Add RemoteKubernetesCluster finalizer #2272
Conversation
zimnx
added
kind/feature
|
@zimnx: GitHub didn't allow me to request PR reviews from the following users: zimnx. Note that only scylladb members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
scylla-operator-bot
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zimnx 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:
Approvers can indicate their approval by writing |
scylla-operator-bot
bot
added
approved
Controller reconciles finalizer on RemoteKubernetesCluster, preventing from premature deletion. It waits until all ScyllaDBClusters using particular RemoteKubernetesCluster are deleted.
zimnx
force-pushed
the
remotekubernetescluster-finalizer
branch
from
c422af5 to
846eef0
Compare
zimnx
changed the title
scylla-operator-bot
bot
removed
the
do-not-merge/work-in-progress
|
Ready to review /cc rzetelskik |
zimnx
changed the title
| var _ = g.Describe("RemoteKubernetesCluster finalizer", func() { | ||
| f := framework.NewFramework("remotekubernetescluster") | ||
|
|
||
| g.It("should block deletion when there are referencing ScyllaDBClusters", func() { |
There was a problem hiding this comment.
| g.It("should block deletion when there are referencing ScyllaDBClusters", func() { | |
| g.It("should block deletion when there are ScyllaDBClusters referencing the object", func() { |
or ScyllaDBCluster referents?
nit
| rkc, err := cluster.ScyllaAdminClient().ScyllaV1alpha1().RemoteKubernetesClusters().Create(ctx, originalRKC, metav1.CreateOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| framework.By("Waiting for the RemoteKubernetesCluster %q to rollout (RV=%s)", rkc.Name, rkc.ResourceVersion) |
There was a problem hiding this comment.
| framework.By("Waiting for the RemoteKubernetesCluster %q to rollout (RV=%s)", rkc.Name, rkc.ResourceVersion) | |
| framework.By("Waiting for the RemoteKubernetesCluster %q to roll out (RV=%s)", rkc.Name, rkc.ResourceVersion) |
applies globally
| framework.By("Deleting RemoteKubernetesCluster %q", rkc.Name) | ||
| err = cluster.ScyllaAdminClient().ScyllaV1alpha1().RemoteKubernetesClusters().Delete(ctx, rkcName, metav1.DeleteOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| framework.By("Awaiting RemoteKubernetesCluster %q deletion", rkc.Name) | ||
| err = framework.WaitForObjectDeletion(ctx, f.DynamicAdminClient(), scyllav1alpha1.GroupVersion.WithResource("remotekubernetesclusters"), rkc.Namespace, rkc.Name, nil) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| framework.By("Recreating RemoteKubernetesCluster %q", rkcName) | ||
| rkc = originalRKC.DeepCopy() | ||
| rkc, err = cluster.ScyllaAdminClient().ScyllaV1alpha1().RemoteKubernetesClusters().Create(ctx, rkc, metav1.CreateOptions{}) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| framework.By("Waiting for the RemoteKubernetesCluster %q to rollout (RV=%s)", rkc.Name, rkc.ResourceVersion) |
| cond := meta.FindStatusCondition(rkc.Status.Conditions, "RemoteKubernetesClusterFinalizerProgressing") | ||
| return cond != nil && cond.Reason == "RemoteKubernetesClusterFinalizerProgressing_IsBeingUsed", nil | ||
| } | ||
| framework.By("Awaiting is being used condition on RemoteKubernetesCluster %q", rkc.Name) |
There was a problem hiding this comment.
| framework.By("Awaiting is being used condition on RemoteKubernetesCluster %q", rkc.Name) | |
| framework.By("Awaiting 'IsBeingUsed' condition on RemoteKubernetesCluster %q", rkc.Name) |
nit, difficult to read now
| cond := meta.FindStatusCondition(rkc.Status.Conditions, "RemoteKubernetesClusterFinalizerProgressing") | ||
| return cond != nil && cond.Reason == "RemoteKubernetesClusterFinalizerProgressing_IsBeingUsed", nil |
There was a problem hiding this comment.
Shouldn't you check for generation and status as well?
There was a problem hiding this comment.
also, looking at sync_finalizer implementation, shouldn't the reason just say IsBeingUsed?
| @@ -0,0 +1,82 @@ | |||
| // Copyright (c) 2024 ScyllaDB. | |||
There was a problem hiding this comment.
nit: shouldn't the file be named finalizer.go, as we already have controllerhelpers/annotation.go doing a very similar thing?
| "k8s.io/klog/v2" | ||
| ) | ||
|
|
||
| func (rkcc *Controller) syncFinalizer(ctx context.Context, rkc *scyllav1alpha1.RemoteKubernetesCluster) ([]metav1.Condition, error) { |
There was a problem hiding this comment.
I recall your previous PRs in the enterprise repo having a separate protection controller in a dedicated file subtree - why the change?
| ) | ||
| } | ||
|
|
||
| func (rkcc *Controller) enqueueScyllaDBCluster(depth int, obj kubeinterfaces.ObjectInterface, op controllerhelpers.HandlerOperationType) { |
There was a problem hiding this comment.
the name of the func is misleading, in the end you are enqueuing an RKC - should it say enqueueRemoteKubernetesClustersReferencedByScyllaDBCluster or something along these lines?
| for _, dc := range sc.Spec.Datacenters { | ||
| rkc, err := rkcc.remoteKubernetesClusterLister.Get(dc.RemoteKubernetesClusterName) | ||
| if err != nil { | ||
| utilruntime.HandleError(fmt.Errorf("couldn't find RemoteKubernetesCluster with name %q", dc.RemoteKubernetesClusterName)) |
There was a problem hiding this comment.
| utilruntime.HandleError(fmt.Errorf("couldn't find RemoteKubernetesCluster with name %q", dc.RemoteKubernetesClusterName)) | |
| utilruntime.HandleError(fmt.Errorf("couldn't get RemoteKubernetesCluster %q: %w", dc.RemoteKubernetesClusterName, err)) |
| rkc, err := rkcc.remoteKubernetesClusterLister.Get(dc.RemoteKubernetesClusterName) | ||
| if err != nil { | ||
| utilruntime.HandleError(fmt.Errorf("couldn't find RemoteKubernetesCluster with name %q", dc.RemoteKubernetesClusterName)) | ||
| return |
Description of your changes:
Controller reconciles finalizer on RemoteKubernetesCluster, preventing from premature deletion. It waits until all ScyllaDBClusters using particular RemoteKubernetesCluster are deleted.
Which issue is resolved by this Pull Request:
Resolves #2277