Implement reconciliation for ManagedCluster and create ManagedClusterView #219

vbnrh · 2024-07-04T11:32:27Z

Added reconciliation logic for ManagedCluster
Added tests for ManagedClusterReconciler reconciliation logic and ensure ManagedClusterViews function.
Reconciliation for ManagedCluster creates ManagedClusterView which pulls the ‘odf-info’ configmap onto the hub.
Updated RBAC rules in config/rbac/role.yaml to include permissions for ManagedClusterView resources. (ManagedCluster RBAC already there)
Plumbing to make the controllers work like the items above
Updated go.mod and go.sum to include github.com/stolostron/multicloud-operators-foundation.

config/rbac/role.yaml

controllers/managedcluster_controller.go

controllers/utils/acm.go

controllers/managedcluster_controller.go

rewantsoni · 2024-07-09T08:54:25Z

controllers/manager.go

@@ -140,6 +142,15 @@ func (o *ManagerOptions) runManager() {
 		os.Exit(1)
 	}

+	if err = (&ManagedClusterReconciler{
+		Client: mgr.GetClient(),
+		Scheme: mgr.GetScheme(),


We don't need to pass the Scheme here, we can get it from client using Client.Scheme()

What would be the difference between both ways ?

We could probably remove the Scheme from the ManagedClusterReconciler, we arent using it anywhere.

controllers/managedcluster_controller.go

vbnrh · 2024-07-09T09:26:23Z

/test integration-test

rewantsoni · 2024-07-09T09:55:11Z

controllers/manager.go

@@ -140,6 +142,15 @@ func (o *ManagerOptions) runManager() {
 		os.Exit(1)
 	}

+	if err = (&ManagedClusterReconciler{
+		Client: mgr.GetClient(),
+		Scheme: mgr.GetScheme(),


We could probably remove the Scheme from the ManagedClusterReconciler, we arent using it anywhere.

controllers/utils/acm.go

SanjalKatiyar · 2024-07-11T15:36:46Z

controllers/mirrorpeer_controller.go

@@ -71,6 +71,7 @@ const spokeClusterRoleBindingName = "spoke-clusterrole-bindings"
 //+kubebuilder:rbac:groups=addon.open-cluster-management.io,resources=managedclusteraddons/status,verbs=*
 //+kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;create;update;watch
 //+kubebuilder:rbac:groups=console.openshift.io,resources=consoleplugins,verbs=get;list;create;update;watch
+// +kubebuilder:rbac:groups=view.open-cluster-management.io,resources=managedclusterviews,verbs=get;list;watch;create;update


just a question: shouldn't this be added to managedcluster_controller file ?? instead of here...

kubebuilder will add the required permissions for the pod to the role manifest automatically. All controllers run in the same container.

yeah, but I assumed it was more of a good practice to add RBACs on top of respective reconciler, rather than spreading it across multiple unrelated files...

I agree. It is good practice to have RBACs close to the controllers that require it. We haven't followed that well enough in this repo, but I think we should start doing it. End result is the same, but this makes it easy to argue about why a particular RBAC was added.

controllers/managedcluster_controller.go

umangachapagain · 2024-07-12T10:23:19Z

controllers/mirrorpeer_controller.go

@@ -71,6 +71,7 @@ const spokeClusterRoleBindingName = "spoke-clusterrole-bindings"
 //+kubebuilder:rbac:groups=addon.open-cluster-management.io,resources=managedclusteraddons/status,verbs=*
 //+kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;create;update;watch
 //+kubebuilder:rbac:groups=console.openshift.io,resources=consoleplugins,verbs=get;list;create;update;watch
+// +kubebuilder:rbac:groups=view.open-cluster-management.io,resources=managedclusterviews,verbs=get;list;watch;create;update


I agree. It is good practice to have RBACs close to the controllers that require it. We haven't followed that well enough in this repo, but I think we should start doing it. End result is the same, but this makes it easy to argue about why a particular RBAC was added.

controllers/utils/acm.go

vbnrh · 2024-07-16T05:41:22Z

Added initialization for ManagedClusterViewReconciler in manager.go to setup the ManagedClusterView controller.
Creates or updates configMap odf-client-info which maps client to it provider cluster
Created comprehensive unit tests to cover the creation and update scenarios of the ConfigMap.

controllers/managedclusterview_controller.go

rewantsoni · 2024-07-16T05:47:42Z

controllers/managedclusterview_controller.go

+		DeleteFunc: func(e event.DeleteEvent) bool {
+			return false
+		},
+		GenericFunc: func(e event.GenericEvent) bool {
+			return false
+		},


Do we need to explicitly specify the Delete and Generic? won't it default to false?

rewantsoni · 2024-07-16T05:51:40Z

controllers/managedclusterview_controller.go

+		ownerExists := false
+		for _, ownerRef := range configMap.OwnerReferences {
+			if ownerRef.UID == managedClusterView.UID {
+				ownerExists = true
+				break
+			}
+		}
+
+		if !ownerExists {
+			ownerRef := *metav1.NewControllerRef(&managedClusterView, viewv1beta1.GroupVersion.WithKind("ManagedClusterView"))
+			logger.Info("OwnerRef added", "UID", string(ownerRef.UID))
+			configMap.OwnerReferences = append(configMap.OwnerReferences, ownerRef)
+		}
+		return nil


Maybe we can replace it with controller-util to make it simpler?

I don't have access to scheme which is required for setOwnerReference function

can we use c.Scheme() ?

I used it but it was not getting set somehow. The tests were failing. I will look into this later

rewantsoni · 2024-07-16T05:52:07Z

controllers/managedclusterview_controller.go

+			Name:      ClientInfoConfigMapName,
+			Namespace: operatorNamespace,
+		},
+		Data: configMapData,


We don't need to set the data here, we can do it inside the CreateOrUpdate directly

rewantsoni · 2024-07-16T05:53:32Z

controllers/managedclusterview_controller.go

+		if strings.HasSuffix(key, ".config.yaml") {
+			var config StorageClusterConfig
+			err := json.Unmarshal([]byte(value), &config)
+			if err != nil {
+				return fmt.Errorf("failed to unmarshal config data for key %s: %v", key, err)
+			}
+
+			providerInfo := config
+			providerInfo.Clients = nil
+			providerInfo.ProviderClusterName = managedClusterView.Namespace
+
+			providerInfoJSON, err := json.Marshal(providerInfo)
+			if err != nil {
+				return fmt.Errorf("failed to marshal provider info: %v", err)
+			}
+
+			for _, client := range config.Clients {
+				reverseLookup[client] = string(providerInfoJSON)
+			}
+		}


this should change once we have changed the struct

vbnrh · 2024-07-16T07:40:30Z

controllers/managedclusterview_controller.go

+	NamespacedName          types.NamespacedName `yaml:"namespacedName"`
+	StorageProviderEndpoint string               `yaml:"storageProviderEndpoint"`
+	CephClusterFSID         string               `yaml:"cephClusterFSID"`
+}


This is a temporary addition till the api is exported on OCS operator

https://github.com/red-hat-storage/ocs-operator/blob/fbe323d325111ca4c871982159149cb05c6b5fc1/controllers/storagecluster/odfinfoconfig.go#L33

rewantsoni · 2024-07-17T06:24:44Z

controllers/managedclusterview_controller.go

+		for _, client := range odfInfo.Clients {
+			clientInfo := ClientInfo{
+				ClusterID:    client.ClusterID,
+				Name:         client.Name,
+				ProviderInfo: providerInfo,
+			}
+			clientInfoMap[client.Name] = clientInfo
+		}
+	}
+
+	configMapData := make(map[string]string)
+	for clientName, clientInfo := range clientInfoMap {
+		clientInfoJSON, err := json.Marshal(clientInfo)
+		if err != nil {
+			return fmt.Errorf("failed to marshal client info: %v", err)
+		}
+		configMapData[clientName] = string(clientInfoJSON)
+	}


The key we use in the configMap for reverse lookup is storageClient Name, two different managed clusters can have the storageClient Name. What do you think about using ClientName/ManagedClusterName (of the client cluster) as the key for the configMap?
We can get the managedClusterName of the client cluster here by referencing the openshift Cluster Version UUID provided in the odfInfo .clients.clusterId

This makes sense.
Added func to fetch ManagedCluster via the clusterId.
Added another field called clientManagedClusterName to ClientInfo
The key is changed from client.Name to clientManagedClusterName/client.Name

vbnrh · 2024-07-17T11:59:39Z

/test unit-test

vbnrh · 2024-07-17T12:16:54Z

/test unit-test

vbnrh · 2024-07-17T12:17:50Z

/test integration-test

controllers/managedcluster_controller.go

controllers/managedclusterview_controller.go

controllers/managedclusterview_controller_test.go

controllers/managedclusterview_controller.go

umangachapagain

Squash the first 2 commits and last 2 commits. Everything else looks good to me.

openshift-ci · 2024-07-19T10:09:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: umangachapagain, vbnrh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [umangachapagain,vbnrh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

controllers/managedcluster_controller.go

controllers/managedclusterview_controller.go

controllers/managedcluster_controller.go

controllers/mirrorpeer_controller.go

…lusterView - Added reconciliation logic for `ManagedCluster` - Added tests for `ManagedClusterReconciler` reconciliation logic and ensure ManagedClusterViews function. - Reconciliation for ManagedCluster creates ManagedClusterView which pulls the ‘odf-info’ configmap onto the hub. - Updated RBAC rules in `config/rbac/role.yaml` to include permissions for ManagedClusterView resources. (ManagedCluster RBAC already there) - Plumbing to make the controllers work like the items above - Updated go.mod and go.sum to include `github.com/stolostron/multicloud-operators-foundation`. - Fixes to functions, tests and adding new predicate Signed-off-by: vbadrina <[email protected]>

- Added initialization for ManagedClusterViewReconciler in manager.go to setup the ManagedClusterView controller. - Creates or updates configMap odf-client-info which maps client to it provider cluster - Created comprehensive unit tests to cover the creation and update scenarios of the ConfigMap. Signed-off-by: vbadrina <[email protected]>

rewantsoni · 2024-07-19T11:39:26Z

/lgtm

SanjalKatiyar · 2024-07-19T13:56:53Z

controllers/utils/managedclusterview.go

+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+)
+
+const MCVLabelKey = "multicluster.odf.openshift.io/cluster"


I don't think this is used anywhere...

Yeah, we replaced with the createdBy label for MCO , i will remove it later

SanjalKatiyar · 2024-07-19T14:18:23Z

controllers/managedcluster_controller.go

 	}

 	return ctrl.NewControllerManagedBy(mgr).
 		For(&clusterv1.ManagedCluster{}, builder.WithPredicates(managedClusterPredicate, predicate.ResourceVersionChangedPredicate{})).
 		Owns(&viewv1beta1.ManagedClusterView{}).
+		Owns(&corev1.ConfigMap{}).


Is there any need to reconcile this on the owned ConfigMaps ?? It's not like we are managing cleanup of MCV or anything here, was there some other reason ??

It is done to ensure that MC reconcile is triggered whenever there are events for the ConfigMap. To ensure the desired ConfigMap is always present..

ConfigMap is created by MCV controller, not by MC controller, right ?? my question was, if any event occurs on that ConfigMap, why we need to reconcile MC controller (which creates MCV)... which edge case am I missing ??

Configmap cannot be owned by MCV as they will be in different namespaces. It can be owned by cluster scoped resource like MC. Which will propagate events to eventually reconcile the configmap to desired state

controllers/managedclusterview_controller.go

SanjalKatiyar · 2024-07-19T14:31:05Z

controllers/managedclusterview_controller.go

+		for _, client := range odfInfo.Clients {
+			managedCluster, err := utils.GetManagedClusterById(ctx, c, client.ClusterID)
+			if err != nil {
+				return err
+			}
+			clientInfo := ClientInfo{
+				ClusterID:                client.ClusterID,
+				Name:                     client.Name,
+				ProviderInfo:             providerInfo,
+				ClientManagedClusterName: managedCluster.Name,
+			}
+			clientInfoMap[fmt.Sprintf("%s/%s", managedCluster.Name, client.Name)] = clientInfo
+		}
+	}


What will happen in case of Multiple StorageCluster scenario (normal 1 internal + 1 external on same OCP) (no Provider-Client) ??

Won't fmt.Sprintf("%s/%s", managedCluster.Name, client.Name) give exact same "key" in both cases ??

Assuming that there will always be one client for internal (somehow), what happens to external mode cluster ?? This ConfigMap won't store info about external cluster, right ??

Let me know if this ConfigMap is only for Provider-Client use case and not for other deployments. Because it make sense if so, but I am not sure why to make it so specific.

vbnrh · 2024-07-22T11:12:54Z

Isn't it conceptually incorrect to call it client-info ConfigMap ?? There can be non-Provider deployments as well (eg: normal internal, external), this flow will remain same for that.
Same is true for the entity which will consume this ConfigMap (say console), used while creating a DRPolicy, which is unrelated to just Provider-Client. Am I missing anything ??

 or purpose of this ConfigMap is to only be used by Provider-Client cases ?? in which case I don't think it is very useful for console.

Let me know if this ConfigMap is only for Provider-Client use case and not for other deployments. Because it make sense if so, but I am not sure why to make it so specific.

@SanjalKatiyar
For the above queries, the purpose of the current configmap was to serve as a quick everse lookup for clients for provider consumer scenarios. The current configmap could not be used due to the way data is structured, there will be lot of redundant data. We will add and extend the design later releases to add to the same or a different configmap for non provider mode deployments

For now, UI may need to consume MCV's created by MCO directly.

SanjalKatiyar · 2024-07-22T11:37:30Z

@SanjalKatiyar For the above queries, the purpose of the current configmap was to serve as a quick everse lookup for clients for provider consumer scenarios. The current configmap could not be used due to the way data is structured, there will be lot of redundant data. We will add and extend the design later releases to add to the same or a different configmap for non provider mode deployments

For now, UI may need to consume MCV's created by MCO directly.

Thanks for the confirmation (that was my guess too).
But, is Provider specific current ConfigMap needed by any operator ?? I mean why are we creating it at all ?? and if we are then we should have created it for all use cases instead of specific case, it's just a re-arrangement of data.

Keeping key as clusterName/storageClusterName/storageClusterNamespace should have reduced the current redundancy as well IIUC.

But anyway, this is just a question. My main concern has already been answered. Console will rely on MCV, instead of the ConfigMap.

vbnrh · 2024-07-22T11:47:39Z

@SanjalKatiyar For the above queries, the purpose of the current configmap was to serve as a quick everse lookup for clients for provider consumer scenarios. The current configmap could not be used due to the way data is structured, there will be lot of redundant data. We will add and extend the design later releases to add to the same or a different configmap for non provider mode deployments
For now, UI may need to consume MCV's created by MCO directly.

Thanks for the confirmation (that was my guess too). But, is Provider specific current ConfigMap needed by any operator ?? I mean why are we creating it at all ?? and if we are then we should have created it for all use cases instead of specific case, it's just a re-arrangement of data.

Keeping key as clusterName/storageClusterName/storageClusterNamespace should have reduced the current redundancy as well IIUC.

But anyway, this is just a question. My main concern has already been answered. Console will rely on MCV, instead of the ConfigMap.

This won't function as reverse lookup for client information as we may need to look through all keys. Your concern is valid though, we will work upon the design to cover all scenarios.

openshift-ci bot added the approved label Jul 4, 2024

umangachapagain requested changes Jul 8, 2024

View reviewed changes

openshift-ci bot assigned umangachapagain Jul 8, 2024

vbnrh requested a review from umangachapagain July 9, 2024 08:24

rewantsoni suggested changes Jul 9, 2024

View reviewed changes

openshift-ci bot assigned rewantsoni Jul 9, 2024

vbnrh force-pushed the odfinfo-pull-configmap branch from 924779d to 6bfaed3 Compare July 9, 2024 09:46

rewantsoni reviewed Jul 9, 2024

View reviewed changes

vbnrh force-pushed the odfinfo-pull-configmap branch 3 times, most recently from 409a44a to 35359fa Compare July 9, 2024 11:42

SanjalKatiyar reviewed Jul 11, 2024

View reviewed changes

controllers/managedcluster_controller.go Outdated Show resolved Hide resolved

vbnrh force-pushed the odfinfo-pull-configmap branch from 35359fa to 0a9f688 Compare July 12, 2024 08:16

umangachapagain requested changes Jul 12, 2024

View reviewed changes

vbnrh force-pushed the odfinfo-pull-configmap branch from 0a9f688 to 1d772d8 Compare July 12, 2024 13:15

openshift-merge-robot added the needs-rebase label Jul 15, 2024

vbnrh force-pushed the odfinfo-pull-configmap branch from 1d772d8 to df4b7a4 Compare July 15, 2024 09:35

openshift-merge-robot removed the needs-rebase label Jul 15, 2024

SanjalKatiyar mentioned this pull request Jul 15, 2024

Bug 2297447: Use ManagedClusterView for ODF info - DRPolicy creation red-hat-storage/odf-console#1470

Merged

vbnrh force-pushed the odfinfo-pull-configmap branch from 2988e81 to 428f7f2 Compare July 16, 2024 05:39

rewantsoni suggested changes Jul 16, 2024

View reviewed changes

vbnrh force-pushed the odfinfo-pull-configmap branch from 428f7f2 to e45de1f Compare July 16, 2024 07:37

vbnrh commented Jul 16, 2024

View reviewed changes

vbnrh force-pushed the odfinfo-pull-configmap branch from e45de1f to 150f216 Compare July 16, 2024 07:42

rewantsoni reviewed Jul 17, 2024

View reviewed changes

vbnrh force-pushed the odfinfo-pull-configmap branch 2 times, most recently from 50cb833 to d5f5de3 Compare July 17, 2024 10:41

vbnrh force-pushed the odfinfo-pull-configmap branch from d5f5de3 to c49600c Compare July 17, 2024 11:54

umangachapagain requested changes Jul 18, 2024

View reviewed changes

umangachapagain reviewed Jul 19, 2024

View reviewed changes

umangachapagain requested a review from rewantsoni July 19, 2024 09:36

vbnrh force-pushed the odfinfo-pull-configmap branch from b9dbed4 to 7e269a5 Compare July 19, 2024 09:52

umangachapagain approved these changes Jul 19, 2024

View reviewed changes

openshift-ci bot added the lgtm label Jul 19, 2024

rewantsoni suggested changes Jul 19, 2024

View reviewed changes

openshift-ci bot removed the lgtm label Jul 19, 2024

vbnrh force-pushed the odfinfo-pull-configmap branch from 7e269a5 to ef0af41 Compare July 19, 2024 10:47

vbnrh force-pushed the odfinfo-pull-configmap branch from ef0af41 to 3ed8293 Compare July 19, 2024 10:59

openshift-ci bot added the lgtm label Jul 19, 2024

openshift-merge-bot bot merged commit 7cfca25 into red-hat-storage:main Jul 19, 2024
11 checks passed

SanjalKatiyar reviewed Jul 19, 2024

View reviewed changes

controllers/managedclusterview_controller.go Show resolved Hide resolved

SanjalKatiyar reviewed Jul 19, 2024

View reviewed changes

Implement reconciliation for ManagedCluster and create ManagedClusterView #219

Implement reconciliation for ManagedCluster and create ManagedClusterView #219

Conversation

vbnrh commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbnrh commented Jul 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanjalKatiyar Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbnrh commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbnrh commented Jul 17, 2024

vbnrh commented Jul 17, 2024

vbnrh commented Jul 17, 2024

umangachapagain left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 19, 2024

rewantsoni commented Jul 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanjalKatiyar Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanjalKatiyar Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

SanjalKatiyar Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

vbnrh commented Jul 22, 2024

SanjalKatiyar commented Jul 22, 2024 • edited Loading

vbnrh commented Jul 22, 2024

SanjalKatiyar Jul 12, 2024 •

edited

Loading

SanjalKatiyar Jul 22, 2024 •

edited

Loading

SanjalKatiyar Jul 19, 2024 •

edited

Loading

SanjalKatiyar Jul 19, 2024 •

edited

Loading

SanjalKatiyar commented Jul 22, 2024 •

edited

Loading