cluster: delete PVCs after decom if flag is set #206

birdayz · 2024-08-29T17:44:26Z

To scale down and up again, we need to delete PVCs after a broker/pod
has been removed.

if auto-delete-pvcs is set, use PersistentVolumeClaimRetentionPolicy to
auto delete PVCs at scale down and delete.

this allows us to scale down and up; PVCs are worthless after scaling
down because we decomission before scaling down. A decomissioned broker
can not rejoin the cluster.

birdayz · 2024-08-30T12:43:08Z

src/go/k8s/pkg/resources/statefulset_scale.go

-	if err := c.Status().Update(ctx, pandaCluster); err != nil {
-		return fmt.Errorf("could not scale cluster %s to %d replicas: %w", pandaCluster.Name, replicas, err)
+
+	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {


i'm seeing often issues with conflicts in this update. now using retry as we should

Rather than using retry why not use a patch here? Patches, by default, won't check resource version and will only change the value that you've changed since the last read (Assuming you're using a client side patch).

.Status is the one exception from my preference to rely on resource version checking. You've already done the work, so you're the one that know the actual state of the status. Statuses, despite being a subresource, still rely on the checking the resource version of the whole object. So you might be getting conflicts due to someone making a change to the Spec which doesn't impact your change on the status.

chrisseto

LGTM though I'd encourage you to use a patch instead of a retry loop for the status update before merging.

chrisseto · 2024-08-30T14:35:38Z

src/go/k8s/pkg/resources/statefulset.go

+	var pvcReclaimRetentionPolicy appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy
+	if r.autoDeletePVCs {
+		pvcReclaimRetentionPolicy.WhenScaled = appsv1.DeletePersistentVolumeClaimRetentionPolicyType
+		pvcReclaimRetentionPolicy.WhenDeleted = appsv1.DeletePersistentVolumeClaimRetentionPolicyType


The paranoia in me says we may want to keep when deleted as retain? I can't really imagine someone scaling the cluster manually but I could see cases where the STS needs to be deleted with orphan = true to work around it's immutability.

Oh you're so right. GOOD CALL!
Delete on orphan DOES happen, and is actively used by the operator. Will change When deleted to RETAIN.

actually need to test if orphan delete triggers this behavior..

it looks like deleting with orphan does not get PVCs deleted. obviously, since pods are retained as well. i can add a test. then it's fine to keep using delete on delete, what do you think?

switched to Patch call. moved the patch helper func from cluster_controller to pkg/patch (pkg/utils does not work because of import conflict w/ types package)

chrisseto · 2024-08-30T14:40:59Z

src/go/k8s/pkg/resources/statefulset_scale.go

-	if err := c.Status().Update(ctx, pandaCluster); err != nil {
-		return fmt.Errorf("could not scale cluster %s to %d replicas: %w", pandaCluster.Name, replicas, err)
+
+	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {


Rather than using retry why not use a patch here? Patches, by default, won't check resource version and will only change the value that you've changed since the last read (Assuming you're using a client side patch).

.Status is the one exception from my preference to rely on resource version checking. You've already done the work, so you're the one that know the actual state of the status. Statuses, despite being a subresource, still rely on the checking the resource version of the whole object. So you might be getting conflicts due to someone making a change to the Spec which doesn't impact your change on the status.

birdayz · 2024-08-30T15:43:23Z

Rather than using retry why not use a patch here? Patches, by default, won't check resource version and will only change the value that you've changed since the last read (Assuming you're using a client side patch).

OK, i will change it. I followed patterns existing in the operator already, where Update was used. But happy to use Patch.

if auto-delete-pvcs is set, use PersistentVolumeClaimRetentionPolicy to auto delete PVCs at scale down and delete. this allows us to scale down and up; PVCs are worthless after scaling down because we decomission before scaling down. A decomissioned broker can not rejoin the cluster.

birdayz · 2024-09-02T15:16:20Z

since this feature requires k8s 1.28, i did the following additional changes:

Add separate KUTTL suite w/ k8s 1.28 - it also has the new flag in the operator enabled
Upgrade KIND to 0.19 (KIND) / k8s 1.28 for that suite. It requires to use some old KIND image using KIND <0.20, because our CI machines do not yet have CGroups v2.

ready for re-review @RafalKorepta @chrisseto - thank you!

it's not possible to toggle feature flags on a per test case basis. therefore, as per advice from Rafal, we're adding a new test suite. it also uses k8s 1.28, as this is what cloud uses, and is required by the new code under test.

RafalKorepta

LGTM

RafalKorepta · 2024-09-02T16:12:58Z

src/go/k8s/tests/e2e-with-flags/decommission/03-assert.yaml

+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+commands:
+- timeout: 300
+  script: |
+    kubectl wait --for=delete pvc/datadir-decommission-2 --timeout 0s -n redpanda --namespace $NAMESPACE


Nice! I would default with TestStep, but then it hard to assert. You approach is better.

206 got merged, even with red tests. this should not have happened. this PR fixes the problem.

birdayz force-pushed the jb/delete-pvcs-after-decommission branch from 898cce6 to 954dc9c Compare August 29, 2024 17:53

birdayz marked this pull request as ready for review August 29, 2024 17:54

birdayz requested review from RafalKorepta and chrisseto as code owners August 29, 2024 17:54

birdayz force-pushed the jb/delete-pvcs-after-decommission branch 5 times, most recently from 97fe759 to f1114b7 Compare August 30, 2024 12:29

birdayz commented Aug 30, 2024

View reviewed changes

birdayz force-pushed the jb/delete-pvcs-after-decommission branch 3 times, most recently from f38cc49 to 96164cf Compare August 30, 2024 13:04

chrisseto approved these changes Aug 30, 2024

View reviewed changes

birdayz force-pushed the jb/delete-pvcs-after-decommission branch 2 times, most recently from cc98b0f to b3e09b6 Compare August 30, 2024 15:33

birdayz force-pushed the jb/delete-pvcs-after-decommission branch 9 times, most recently from 9f6d670 to 4883964 Compare September 2, 2024 13:08

birdayz force-pushed the jb/delete-pvcs-after-decommission branch 3 times, most recently from 6e476a7 to 824cea8 Compare September 2, 2024 15:12

birdayz force-pushed the jb/delete-pvcs-after-decommission branch from 824cea8 to 7d440ba Compare September 2, 2024 15:38

operator v1: add separate test suite with feature flags

8550d70

it's not possible to toggle feature flags on a per test case basis. therefore, as per advice from Rafal, we're adding a new test suite. it also uses k8s 1.28, as this is what cloud uses, and is required by the new code under test.

birdayz force-pushed the jb/delete-pvcs-after-decommission branch from 7d440ba to 8550d70 Compare September 2, 2024 15:59

RafalKorepta approved these changes Sep 2, 2024

View reviewed changes

RafalKorepta enabled auto-merge (rebase) September 2, 2024 16:17

RafalKorepta merged commit 30ee31b into main Sep 2, 2024
4 checks passed

birdayz mentioned this pull request Sep 2, 2024

Fixup for #206 #209

Merged

birdayz added a commit that referenced this pull request Sep 2, 2024

fixup for #206

d170c26

206 got merged, even with red tests. this should not have happened. this PR fixes the problem.

birdayz added a commit that referenced this pull request Sep 2, 2024

fixup for #206

e8ac88c

206 got merged, even with red tests. this should not have happened. this PR fixes the problem.

birdayz added a commit that referenced this pull request Sep 2, 2024

fixup for #206

a9aa804

206 got merged, even with red tests. this should not have happened. this PR fixes the problem.

birdayz added a commit that referenced this pull request Sep 2, 2024

fixup for #206

c81a8ad

206 got merged, even with red tests. this should not have happened. this PR fixes the problem.

RafalKorepta pushed a commit that referenced this pull request Sep 3, 2024

fixup for #206

5787777

206 got merged, even with red tests. this should not have happened. this PR fixes the problem.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster: delete PVCs after decom if flag is set #206

cluster: delete PVCs after decom if flag is set #206

birdayz commented Aug 29, 2024 •

edited

Loading

birdayz Aug 30, 2024

chrisseto Aug 30, 2024

chrisseto left a comment

chrisseto Aug 30, 2024

birdayz Aug 30, 2024

birdayz Aug 30, 2024

birdayz Aug 30, 2024

birdayz Sep 2, 2024

chrisseto Aug 30, 2024

birdayz commented Aug 30, 2024

birdayz commented Sep 2, 2024 •

edited

Loading

RafalKorepta left a comment

RafalKorepta Sep 2, 2024

cluster: delete PVCs after decom if flag is set #206

cluster: delete PVCs after decom if flag is set #206

Conversation

birdayz commented Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisseto left a 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

birdayz commented Aug 30, 2024

birdayz commented Sep 2, 2024 • edited Loading

RafalKorepta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birdayz commented Aug 29, 2024 •

edited

Loading

birdayz commented Sep 2, 2024 •

edited

Loading