drpolicy controller define #200

hatfieldbrian · 2021-08-11T20:38:41Z

- vrg and pv cluster roles manifestworks unify
- drpolicy create/update:
	- finalizer add if absent
	- cluster roles manifestwork create, if absent, for each cluster in drpolicy
- drpolicy delete:
	- cluster roles manifestwork delete for each cluster not in any other drpolicy
	- finalizer remove if present
- ginkgo version increase from 1.16.1 to 1.16.4
- gomega version increase from 1.11.0 to 1.15.0

Signed-off-by: bhatfiel bhatfiel@redhat.com

BenamarMk · 2021-08-12T18:13:15Z

controllers/drpolicy_controller.go

+	case true:
+		log.Info("create/update")
+
+		controllerutil.AddFinalizer(drpolicy, finalizerName)


It is better to check if the finalizer exists.

if !controllerutil.ContainsFinalizer(drpolicy, finalizerName) { controllerutil.AddFinalizer(drpolicy, finalizerName) if err := r.Update(ctx, drpolicy); err != nil { return ctrl.Result{}, fmt.Errorf("finalizer add update: %w", err) }

AddFinalizer checks if it exists before adding it

I understand, but I am talking about the whole block. Your reconciler can be called several times, and it will try to update and createRoles several for as many times as it is called for as long as the object is not being deleted.

I think I understand: an optimization to skip the Client.Update() if finalizer already present.

Oh, actually more than that, an infinite loop!

Optimization to update only if finalizer added has been added. Role creation seems optimal already.

BenamarMk · 2021-08-12T18:13:53Z

controllers/drpolicy_controller.go

+			return ctrl.Result{}, fmt.Errorf("cluster roles delete: %w", err)
+		}
+
+		controllerutil.RemoveFinalizer(drpolicy, finalizerName)


RemoveFinalizer checks if it exists before removing it

Same as above

Optimization to update only if finalizer removed has been added.

BenamarMk · 2021-08-12T18:21:33Z

controllers/util/mw_util.go

+var clusterRolesMutex sync.Mutex
+
+func (mwu *MWUtil) ClusterRolesCreate(drpolicy *rmn.DRPolicy) error {
+	clusterRolesMutex.Lock()


why do you need a lock? ClusterRoles are created from the reconciler and I believe that thread safe

It is my understanding that a hub can have multiple DRPolicies each with at most one reconciler running at a time. The lock serializes concurrent creates, deletes, or creates and deletes.

Right, but I am not sure what the lock is protecting. I have to think about it. Ignore my comment for now.

Suppose drpolicyA specifies cluster1 and is being deleted, while drpolicyB also specifying cluster1 is being created. drpolicyA's reconciler reads all drpolicies and finds no other drpolicies include cluster1 because drpolicyB has not yet been created, so it determines cluster1's roles can be deleted. But before it does so, drpolicyB is created and its reconciler determines cluster1's roles need not be created because it already exists, then drpolicyA's reconciler deletes the roles that drpolicyB expects to exist. The mutex makes the read-modify-write operation atomic.

BenamarMk · 2021-08-12T18:28:29Z

controllers/util/mw_util.go

+	for i := range manifestworks.Items {
+		manifestwork := &manifestworks.Items[i]
+		if manifestwork.ObjectMeta.Name == ClusterRolesManifestWorkName {
+			*clusterNames = clusterNames.Insert(manifestwork.ObjectMeta.Namespace)


What the heck this is doing? I can wrap my head around it. It inserting a cluster name (which is MW namespace) into a map of strings but the return value of the Insert is a string which is assigned to *clusterNames???

It outputs a set of namespace names, i.e. cluster names, that have a manifest work named "ramendr-roles". It uses kubernetes' sets.String which is a set of strings, implemented as a map with string keys and empty values.

Okay. So I was looking at this Insert. So you are not using that one then??? I can double-check, I am just being lazy.
https://github.com/kubernetes/apimachinery/blob/v0.22.0/pkg/util/sets/string.go#L49

Ah got it. The confusion the name String in the return value that threw me off. I thought it is a simple string, but it is typedefed to a map.

This routine is currently only used by unit test to confirm that only expected cluster roles manifestworks exist.

controllers/util/mw_util.go

BenamarMk · 2021-08-14T08:07:39Z

controllers/drplacementcontrol_controller_test.go

@@ -595,7 +597,7 @@ func verifyVRGManifestWorkCreatedAsPrimary(managedCluster string) {
 		return err == nil
 	}, timeout, interval).Should(BeTrue())

-	Expect(len(createdVRGRolesManifest.Spec.Workload.Manifests)).To(Equal(2))
+	Expect(len(createdVRGRolesManifest.Spec.Workload.Manifests)).To(BeNumerically(">=", 2))


Why isn't the "ramendr-vrg-roles" count exact? So it should be 2 per managed cluster right?

The vrg and pvc cluster roles and role bindings are now in the same manifestwork (to simplify management since they have the same lifetime) for a total of 4 manifests.

So, test against 4 then. IOW, if the len of Manifests is 6, is that OK? But the expectation will succeed.

I think more manifests are ok. This particular test applies only the first two manifests and then verifies a vrg can be created.

BenamarMk · 2021-08-14T08:09:12Z

controllers/drpolicy_controller.go

+type DRPolicyReconciler struct {
+	client.Client
+	Scheme *runtime.Scheme
+	// Log    logr.Logger


Remove this

BenamarMk · 2021-08-14T08:20:20Z

controllers/drpolicy_controller.go

+	finalizerCount := len(drpolicy.ObjectMeta.Finalizers)
+	controllerutil.AddFinalizer(drpolicy, finalizerName)
+
+	if len(drpolicy.ObjectMeta.Finalizers) != finalizerCount {


(this is kind of nit) Can we stick to one pattern when adding and checking finalizers? The following is kind of the same pattern that the DRPC and the VRG are using:

if !controllerutil.ContainsFinalizer(drpolicy, finalizerName) { controllerutil.AddFinalizer(drpolicy, finalizerName) client.Update(ctx, drpolicy) ... }

I am not saying this is the same pattern, but I am suggesting sticking to one.

I agree. I chose this method because len() of a slice seems to be a constant time operation, whereas Add, Remove, and Contains Finalizer are each linear. It'd be nice if Add and Remove would return a boolean.

I don't think that performance is an issue here.

I am going to suggest we stick to the ContainsFinalizer pattern, not because this is incorrect, it is just devaiant from the pattern, and any fixes/updates to controller-util should retain the interface if we follow the pattern. (and as stated this is a nit)

I agree. I chose this method because len() of a slice seems to be a constant time operation, whereas Add, Remove, and Contains Finalizer are each linear. It'd be nice if Add and Remove would return a boolean.

Pull request to address this: kubernetes-sigs/controller-runtime#1636

BenamarMk · 2021-08-14T08:58:25Z

api/v1alpha1/drpolicy_types.go

@@ -74,6 +74,15 @@ type DRPolicy struct {
 	Status *DRPolicyStatus `json:"status,omitempty"`
 }

+func (drpolicy *DRPolicy) ClusterNames() []string {


Move this out of the *types.go file

This seems, to me, like a logical place for it. Is it a matter of not including it in an external interface? Users may also find it useful and it seems trivial to support.

Correct. Put that function in the implementation file.

It has been moved to ramen's private util package.

vdeenadh · 2021-08-14T00:44:31Z

api/v1alpha1/drpolicy_types.go

+
+	return clusterNames
+}
+


I added two similar DRPolicy methods, which raised the following best-practice related questions:

Benamar: Should the *types.go file contains implementation other than the Init function?

Shyam felt that DRPolicy methods are in deepCopy file and did not want any repurcussions due to more methods getting added to the API itself.

Given the above concerns, I changed them from methods to helper functions and placed them in util/drpolicy_util.go.

vdeenadh · 2021-08-14T00:45:24Z

controllers/drplacementcontrol_controller_test.go

@@ -595,7 +597,7 @@ func verifyVRGManifestWorkCreatedAsPrimary(managedCluster string) {
 		return err == nil
 	}, timeout, interval).Should(BeTrue())

-	Expect(len(createdVRGRolesManifest.Spec.Workload.Manifests)).To(Equal(2))
+	Expect(len(createdVRGRolesManifest.Spec.Workload.Manifests)).To(BeNumerically(">=", 2))


It is not clear why the ">=" is required instead of "==". It may help to comment the expectation.

Benamar raised this issue too: #200 (comment). The code immediately following this only references the first two manifests. Perhaps the pv roles manifests ought to be tested like the vrg ones are.

vdeenadh · 2021-08-14T00:47:51Z

controllers/drplacementcontrol_controller_test.go

@@ -760,13 +762,13 @@ var _ = Describe("DRPlacementControl Reconciler", func() {
 				verifyUserPlacementRuleDecision(userPlacementRule.Name, userPlacementRule.Namespace, WestManagedCluster)
 				verifyDRPCStatusPreferredClusterExpectation(rmn.FailedOver)
 				verifyVRGManifestWorkCreatedAsPrimary(WestManagedCluster)
-				Expect(getManifestWorkCount(WestManagedCluster)).Should(Equal(4)) // MWs for VRG+ROLES+PVs
+				Expect(getManifestWorkCount(WestManagedCluster)).Should(Equal(3)) // MWs for VRG+ROLES+PVs


(nit) This commit is not a related to this PR and not addressed to @hatfieldbrian. When we get a chance in future, I suggest that instead of using hard-coded values, can we use a variable that tracks how many manifest work is expected? The variable could be incremented when new mw is created and decremented when a mw is deleted.

vdeenadh · 2021-08-14T00:49:05Z

controllers/drpolicy_controller_test.go

+		It("should create a cluster roles manifest work for each cluster added", func() {
+		})
+		It("should delete a cluster roles manifest work for each cluster removed", func() {
+		})


Are these two Its to be implemented later? If yes, a TODO comment may help.

vdeenadh · 2021-08-14T05:50:22Z

controllers/suite_test.go

+	Expect((&ramencontrollers.DRPolicyReconciler{
+		Client: k8sManager.GetClient(),
+		Scheme: k8sManager.GetScheme(),
+		// Log:    ctrl.Log.WithName("controllers").WithName("DRPolicy"),


Why comment out the log "DRPolicy" qualifier? Won't the qualifier be useful when the log is output during test failures?

These log qualifiers are added in DRPolicyReconciler.Reconcile() instead which is common to main.go and _test.go instances.

hatfieldbrian · 2021-08-17T16:06:49Z

A bug in drpolicy delete unit test was fixed and it now passes.

raghavendrabhat · 2021-08-17T17:46:57Z

controllers/drpolicy_controller.go

+
+		if err := manifestWorkUtil.ClusterRolesCreate(drpolicy); err != nil {
+			return ctrl.Result{}, fmt.Errorf("cluster roles create: %w", err)
+		}


As mentioned here (https://github.com/RamenDR/ramen/blob/main/controllers/drplacementcontrol_controller.go#L332-L335), it may be better to incorporate the task of validating the schedule in DRPolicy instead of DRPC.

Thanks Raghu. #201 tracks this issue.

ShyamsundarR

@hatfieldbrian rebase to main and any comments that you can tackle are good to get this merged.

config/samples/kustomization.yaml

config/rbac/role.yaml

ShyamsundarR · 2021-08-19T15:47:25Z

controllers/drpolicy_controller.go

+	finalizerCount := len(drpolicy.ObjectMeta.Finalizers)
+	controllerutil.AddFinalizer(drpolicy, finalizerName)
+
+	if len(drpolicy.ObjectMeta.Finalizers) != finalizerCount {


I am going to suggest we stick to the ContainsFinalizer pattern, not because this is incorrect, it is just devaiant from the pattern, and any fixes/updates to controller-util should retain the interface if we follow the pattern. (and as stated this is a nit)

controllers/util/mw_util.go

- drpolicy create/update: - finalizer add if absent - cluster roles manifestwork create, if absent, for each cluster in drpolicy - drpolicy delete: - cluster roles manifestwork delete for each cluster not in any other drpolicy - finalizer remove if present - gomega version increase from 1.14.0 to 1.15.0 Signed-off-by: bhatfiel <bhatfiel@redhat.com>

ShyamsundarR

@hatfieldbrian looks like CI is failing due to go.sum updates

@BenamarMk or @vdeenadh if you can take a look and add an approval we can merge this.

BenamarMk

@ShyamsundarR already approved. We'll merge once the build is complete

hatfieldbrian force-pushed the drpolicy_controller branch from ee4fc2a to 9cf5473 Compare August 11, 2021 22:56

This was referenced Aug 11, 2021

use schedule instead of VolumeReplicationClass #182

Merged

Add a reconciler for DRPolicy #79

Closed

BenamarMk reviewed Aug 12, 2021

View reviewed changes

hatfieldbrian force-pushed the drpolicy_controller branch 2 times, most recently from 79498b0 to 208827d Compare August 13, 2021 07:00

BenamarMk reviewed Aug 14, 2021

View reviewed changes

hatfieldbrian force-pushed the drpolicy_controller branch 3 times, most recently from 3408cea to a935e72 Compare August 14, 2021 22:01

BenamarMk approved these changes Aug 16, 2021

View reviewed changes

vdeenadh reviewed Aug 16, 2021

View reviewed changes

hatfieldbrian force-pushed the drpolicy_controller branch 2 times, most recently from e4b57a2 to f5c91be Compare August 17, 2021 08:41

hatfieldbrian force-pushed the drpolicy_controller branch from f5c91be to 84306fe Compare August 17, 2021 16:15

raghavendrabhat reviewed Aug 17, 2021

View reviewed changes

ShyamsundarR reviewed Aug 19, 2021

View reviewed changes

hatfieldbrian force-pushed the drpolicy_controller branch 4 times, most recently from 46b229f to 8267b48 Compare August 20, 2021 10:30

hatfieldbrian force-pushed the drpolicy_controller branch from 8267b48 to 30c4bfb Compare August 20, 2021 10:44

ShyamsundarR approved these changes Aug 20, 2021

View reviewed changes

BenamarMk reviewed Aug 20, 2021

View reviewed changes

BenamarMk merged commit 6867e44 into RamenDR:main Aug 20, 2021

hatfieldbrian deleted the drpolicy_controller branch August 20, 2021 11:13

drpolicy controller define #200

drpolicy controller define #200

Conversation

hatfieldbrian commented Aug 11, 2021

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

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

BenamarMk Aug 12, 2021 • edited

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

BenamarMk Aug 14, 2021 • edited

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

hatfieldbrian Aug 14, 2021 • edited

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

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

hatfieldbrian commented Aug 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShyamsundarR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShyamsundarR left a comment

Choose a reason for hiding this comment

BenamarMk left a comment

Choose a reason for hiding this comment

BenamarMk Aug 12, 2021 •

edited

BenamarMk Aug 14, 2021 •

edited

hatfieldbrian Aug 14, 2021 •

edited