Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DFBUGS-1026: Provision to disable DRPC reconsile temporarily - WA of CG #1720

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

DFBUGS-1026: Provision to disable DRPC reconsile temporarily - WA of CG #1720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/v1alpha1/ramenconfig_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,10 @@ type RamenConfig struct {

// RamenOpsNamespace is the namespace where resources for unmanaged apps are created
RamenOpsNamespace string `json:"ramenOpsNamespace,omitempty"`

// StartDRPCReconciliationPaused is a flag to pause all the DRPC reconcile temporarily.
// This flag can be overridden by the annotation drplacementcontrol.ramendr.openshift.io/reconciliationUnpaused
StartDRPCReconciliationPaused bool `json:"startDRPCReconciliationPaused,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the Start from the name, this flag disable reconcile even if was already started. It means that drpc is paused by default and requires unpuasing to start reconciling.

I'm not sure about paused and unpause, maybe we can use more common terms like disabled and enabled?

The option can be:

drpcReconciliationDisabled

and the annotation:

reconciliation-enabled

The logic seems clear, if the option is set, all drpcs are disabled, except the these with the explicit "enabled" annotation. It is common that local flag overrides global flag.

@BenamarMk what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Start... here, is simply an indication that disabling is intended for new DRPCs only. But, we can drop it. I'll let Gowtham decides. Also, I like the word Pause instead of disable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not really certain we need this, to stop reconciliation just scale down the deployment to 0 replicas. That would scale down all controllers (i.e policy/cluster/etc.) but this seems excessive at present.

Once the annotation is removed for CG enabling, if a future use-case arises where a single controller from the set needs to be disabled, this maybe considered.

}

func init() {
Expand Down
18 changes: 18 additions & 0 deletions internal/controller/drplacementcontrol_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@

DoNotDeletePVCAnnotation = "drplacementcontrol.ramendr.openshift.io/do-not-delete-pvc"
DoNotDeletePVCAnnotationVal = "true"

// Force DRPC reconcile to be enabled
ReconciliationUnpaused = "drplacementcontrol.ramendr.openshift.io/reconciliationUnpaused"
)

var ErrInitialWaitTimeForDRPCPlacementRule = errors.New("waiting for DRPC Placement to produces placement decision")
Expand Down Expand Up @@ -152,6 +155,16 @@
return ctrl.Result{}, err
}

// Workaround: to add drplacementcontrol.ramendr.openshift.io/is-cg-enabled annotation
// for the application workload that is DR protected from the UI.
if !schedulingEnabled(drpc, ramenConfig) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the flags are about reconciliation, this helper should be reconciliationEnabled()

logger.Info("DRPC is reconcile is paused, To continue the reconsile, " +
"either add drplacementcontrol.ramendr.openshift.io/reconciliationUnpaused: 'true' " +
"annotation or update StartDRPCReconciliationPaused on the ramen configMap.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log will appear many times - each time a watched resource changes, and it is a very verbose log.

It would be better to set the status to report that the drpc is disabled. In this case this will happen once since the next reconcile the status will not change.


return ctrl.Result{}, nil
}

var placementObj client.Object

placementObj, err = getPlacementOrPlacementRule(ctx, r.Client, drpc, logger)
Expand Down Expand Up @@ -2467,3 +2480,8 @@

return drpolicyClusters.Intersection(otherDrpolicyClusters).Len() > 0, nil
}

func schedulingEnabled(drpc *rmn.DRPlacementControl, ramenConfig *rmn.RamenConfig) bool {
force := drpc.GetAnnotations()[ReconciliationUnpaused]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

force is does not match the new name, it was good for the old name.

return !ramenConfig.StartDRPCReconciliationPaused && force == "true"
}
78 changes: 78 additions & 0 deletions internal/controller/drplacementcontrol_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,10 @@ func createDRPC(placementName, name, namespace, drPolicyName, preferredCluster s
return drpc
}

func updateDRPC(drpc *rmn.DRPlacementControl) {
Expect(k8sClient.Update(context.TODO(), drpc)).Should(Succeed())
}

//nolint:unparam
func deleteUserPlacementRule(name, namespace string) {
userPlacementRule := getLatestUserPlacementRule(name, namespace)
Expand Down Expand Up @@ -2772,6 +2776,80 @@ var _ = Describe("DRPlacementControl Reconciler", func() {
deleteDRClustersAsync()
})
})

Context("Disable DRPlacementControl Reconciler", func() {
var userPlacementRule1 *plrv1.PlacementRule

Specify("DRClusters", func() {
populateDRClusters()
})

When("Application deployed for the first time", func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DRPC created in the first time?

Application is deployment may is not related to the drpc, it can be deployed before the drpc was created.

It("Should not have any drpc status", func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be better to have status reporting that the drpc is disabled.

createNamespacesAsync(getNamespaceObj(DefaultDRPCNamespace))
createManagedClusters(asyncClusters)
createDRClustersAsync()
createDRPolicyAsync()
ramenConfig.StartDRPCReconciliationPaused = true
configMapUpdate()
DeferCleanup(func() {
ramenConfig.StartDRPCReconciliationPaused = false
configMapUpdate()
})

var placementObj client.Object
placementObj, _ = CreatePlacementAndDRPC(
DefaultDRPCNamespace, UserPlacementRuleName, East1ManagedCluster, UsePlacementRule)
userPlacementRule1 = placementObj.(*plrv1.PlacementRule)
Expect(userPlacementRule1).NotTo(BeNil())
drpc := getLatestDRPC(DefaultDRPCNamespace)
Expect(drpc.Status.Phase == "").Should(BeTrue())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This contradicts the test name - we have Status. Maybe you mean that we should not have phase?

Can you share an example drpc status in this case? Adding it to the commit message will be helpful.

})

})

When("Update ramen config to override DRPC reconcile", func() {
It("Should have some drpc status after reconcile", func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that disabling reconciliation will keep the status before the reconciliation was disabled?

It would be more useful if we had disable status in the drpc status, and it would reflect the current status of the drpc. If we disabled the drpc after it was reconciled, the status will be the same as the last status, with Status.disabled: "true".

ramenConfig.StartDRPCReconciliationPaused = true
configMapUpdate()
DeferCleanup(func() {
ramenConfig.StartDRPCReconciliationPaused = false
configMapUpdate()
})
drpc := getLatestDRPC(DefaultDRPCNamespace)
drpc.Annotations = map[string]string{controllers.ReconciliationUnpaused: "true"}
updateDRPC(drpc)
waitForDRPCPhaseAndProgression(DefaultDRPCNamespace, rmn.Deployed)
})
})

When("Deleting DRPolicy with DRPC references", func() {
It("Should retain the deleted DRPolicy in the API server", func() {
By("\n\n*** DELETE drpolicy ***\n\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copied from another test? not sure why we need all these newlines and ***

deleteDRPolicyAsync()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests that drpolicy is not deleted?

})
})

When("Deleting user PlacementRule", func() {
It("Should cleanup DRPC", func() {
By("\n\n*** DELETE User PlacementRule ***\n\n")
deleteUserPlacementRule(UserPlacementRuleName, DefaultDRPCNamespace)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "cleanup DRPC" means? if this delete the drpc, the next test is incorrect since the drpc was already deleted.

But I don't think that deleting drpc will work with the current code, since return from the reconcile very early before we reach the check for deleted drpc.

})
})

When("Deleting DRPC", func() {
It("Should delete all VRGs", func() {
deleteDRPC()
waitForCompletion("deleted")
ensureNamespaceMWsDeletedFromAllClusters(DefaultDRPCNamespace)
})
})

Specify("delete drclusters", func() {
deleteDRClustersAsync()
})
})

})

func getVRGManifestWorkCount() int {
Expand Down
Loading