Skip to content

Commit ac283f8

Browse files
teoincontattoteoincontatto
committed
fix: PersistentVolumeClaim are not cleaned up after cluster deletion
1 parent 7d916c7 commit ac283f8

4 files changed

Lines changed: 114 additions & 43 deletions

File tree

‎stackgres-k8s/e2e/envs/kind‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -434,8 +434,8 @@ EOF
434434
if [ "$(echo "$K8S_VERSION" | tr . '\n' | head -n 2 | xargs -I @ printf '%05d' @)" \
435435
-ge "$(echo "1.22" | tr . '\n' | xargs -I @ printf '%05d' @)" ]
436436
then
437-
kubectl create -f https://raw.githubusercontent.com/projectcalico/calico/v3.26.4/manifests/tigera-operator.yaml
438-
wait_until kubectl create -f https://raw.githubusercontent.com/projectcalico/calico/v3.26.4/manifests/custom-resources.yaml
437+
kubectl replace --force -f https://raw.githubusercontent.com/projectcalico/calico/v3.26.4/manifests/tigera-operator.yaml
438+
wait_until kubectl replace --force -f https://raw.githubusercontent.com/projectcalico/calico/v3.26.4/manifests/custom-resources.yaml
439439
kubectl patch installations.operator.tigera.io default --type json \
440440
-p '[{"op":"replace","path":"/spec/calicoNetwork/ipPools/0/cidr","value":"'"$K8S_POD_CIDR"'"}]'
441441
else
@@ -506,12 +506,12 @@ EOF
506506

507507
{
508508
# Apply VolumeSnapshot CRDs
509-
kubectl create -f "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${SNAPSHOTTER_VERSION}/client/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml"
510-
kubectl create -f "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${SNAPSHOTTER_VERSION}/client/config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml"
511-
kubectl create -f "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${SNAPSHOTTER_VERSION}/client/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml"
509+
kubectl replace --force -f "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${SNAPSHOTTER_VERSION}/client/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml"
510+
kubectl replace --force -f "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${SNAPSHOTTER_VERSION}/client/config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml"
511+
kubectl replace --force -f "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${SNAPSHOTTER_VERSION}/client/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml"
512512
# Create snapshot controller
513-
kubectl create -f "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${SNAPSHOTTER_VERSION}/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml"
514-
kubectl create -f "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${SNAPSHOTTER_VERSION}/deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml"
513+
kubectl replace --force -f "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${SNAPSHOTTER_VERSION}/deploy/kubernetes/snapshot-controller/rbac-snapshot-controller.yaml"
514+
kubectl replace --force -f "https://raw.githubusercontent.com/kubernetes-csi/external-snapshotter/${SNAPSHOTTER_VERSION}/deploy/kubernetes/snapshot-controller/setup-snapshot-controller.yaml"
515515
CSI_DRIVER_HOST_PATH_PATH="$TARGET_PATH/csi-driver-host-path/deploy/kubernetes-$(printf %s "$K8S_VERSION" | cut -d . -f 1-2)"
516516
if [ "$(printf %s "$K8S_VERSION" | cut -d . -f 1-2)" = 1.20 ]
517517
then
@@ -567,7 +567,7 @@ EOF
567567
sed -i "s#kubectl#sh $CSI_DRIVER_HOST_PATH_PATH/kubectlw#" \
568568
"$CSI_DRIVER_HOST_PATH_PATH"/deploy.sh
569569
IMAGE_TAG= bash "$CSI_DRIVER_HOST_PATH_PATH"/deploy.sh
570-
kubectl create -f "$TARGET_PATH/csi-driver-host-path/examples/csi-storageclass.yaml"
570+
kubectl replace --force -f "$TARGET_PATH/csi-driver-host-path/examples/csi-storageclass.yaml"
571571
kubectl get storageclass -o name | xargs -I % kubectl annotate % --overwrite storageclass.kubernetes.io/is-default-class=false
572572
kubectl annotate storageclass csi-hostpath-sc --overwrite storageclass.kubernetes.io/is-default-class=true
573573
kubectl annotate volumesnapshotclass csi-hostpath-snapclass --overwrite snapshot.storage.kubernetes.io/is-default-class="true"

‎stackgres-k8s/src/operator/src/main/java/io/stackgres/operator/conciliation/factory/cluster/ClusterStatefulSet.java‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import io.stackgres.common.crd.sgcluster.StackGresClusterStatus;
3838
import io.stackgres.common.crd.sgcluster.StackGresReplicationInitializationMode;
3939
import io.stackgres.common.labels.LabelFactoryForCluster;
40+
import io.stackgres.operator.conciliation.KubernetesVersionBinder;
4041
import io.stackgres.operator.conciliation.OperatorVersionBinder;
4142
import io.stackgres.operator.conciliation.ResourceGenerator;
4243
import io.stackgres.operator.conciliation.cluster.StackGresClusterContext;
@@ -50,6 +51,7 @@
5051

5152
@Singleton
5253
@OperatorVersionBinder
54+
@KubernetesVersionBinder(from = "1.23")
5355
public class ClusterStatefulSet
5456
implements ResourceGenerator<StackGresClusterContext> {
5557

@@ -159,7 +161,7 @@ public Stream<HasMetadata> generateResource(StackGresClusterContext context) {
159161
instances = Math.max(1, context.getCurrentInstances());
160162
LOGGER.info("Skipping upscale while waiting for a fresh SGBackup to be created");
161163
}
162-
StatefulSet clusterStatefulSet = new StatefulSetBuilder()
164+
StatefulSetBuilder clusterStatefulSetBuilder = new StatefulSetBuilder()
163165
.withNewMetadata()
164166
.withNamespace(namespace)
165167
.withName(name)
@@ -178,10 +180,6 @@ public Stream<HasMetadata> generateResource(StackGresClusterContext context) {
178180
.build())
179181
.withServiceName(name)
180182
.withTemplate(podTemplateSpec.getSpec())
181-
.withNewPersistentVolumeClaimRetentionPolicy()
182-
.withWhenDeleted("Delete")
183-
.withWhenScaled("Retain")
184-
.endPersistentVolumeClaimRetentionPolicy()
185183
.withVolumeClaimTemplates(
186184
new PersistentVolumeClaimBuilder()
187185
.withNewMetadata()
@@ -192,8 +190,9 @@ public Stream<HasMetadata> generateResource(StackGresClusterContext context) {
192190
.withSpec(volumeClaimSpec.build())
193191
.build()
194192
)
195-
.endSpec()
196-
.build();
193+
.endSpec();
194+
applyToStatefulSetBuilder(clusterStatefulSetBuilder);
195+
StatefulSet clusterStatefulSet = clusterStatefulSetBuilder.build();
197196

198197
var volumeDependencies = podTemplateSpec.claimedVolumes().stream()
199198
.map(availableVolumesPairs::get)
@@ -205,4 +204,14 @@ public Stream<HasMetadata> generateResource(StackGresClusterContext context) {
205204
return Stream.concat(Stream.of(clusterStatefulSet), volumeDependencies.stream());
206205
}
207206

207+
protected void applyToStatefulSetBuilder(StatefulSetBuilder clusterStatefulSetBuilder) {
208+
clusterStatefulSetBuilder
209+
.editSpec()
210+
.withNewPersistentVolumeClaimRetentionPolicy()
211+
.withWhenDeleted("Delete")
212+
.withWhenScaled("Retain")
213+
.endPersistentVolumeClaimRetentionPolicy()
214+
.endSpec();
215+
}
216+
208217
}

‎stackgres-k8s/src/operator/src/main/java/io/stackgres/operator/conciliation/factory/cluster/ClusterStatefulSetK8sV1M22.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/*
2+
* Copyright (C) 2019 OnGres, Inc.
3+
* SPDX-License-Identifier: AGPL-3.0-or-later
4+
*/
5+
6+
package io.stackgres.operator.conciliation.factory.cluster;
7+
8+
import io.fabric8.kubernetes.api.model.apps.StatefulSetBuilder;
9+
import io.stackgres.common.labels.LabelFactoryForCluster;
10+
import io.stackgres.operator.conciliation.KubernetesVersionBinder;
11+
import io.stackgres.operator.conciliation.OperatorVersionBinder;
12+
import io.stackgres.operator.conciliation.cluster.StackGresClusterContext;
13+
import io.stackgres.operator.conciliation.factory.VolumeDiscoverer;
14+
import jakarta.inject.Inject;
15+
import jakarta.inject.Singleton;
16+
17+
@Singleton
18+
@OperatorVersionBinder
19+
@KubernetesVersionBinder(to = "1.22")
20+
public class ClusterStatefulSetK8sV1M22 extends ClusterStatefulSet {
21+
22+
@Inject
23+
public ClusterStatefulSetK8sV1M22(
24+
LabelFactoryForCluster labelFactory,
25+
PodTemplateFactoryDiscoverer<ClusterContainerContext>
26+
podTemplateSpecFactoryDiscoverer,
27+
VolumeDiscoverer<StackGresClusterContext> volumeDiscoverer) {
28+
super(labelFactory, podTemplateSpecFactoryDiscoverer, volumeDiscoverer);
29+
}
30+
31+
@Override
32+
protected void applyToStatefulSetBuilder(StatefulSetBuilder clusterStatefulSetBuilder) {
33+
}
34+
35+
}

‎stackgres-k8s/src/operator/src/test/java/io/stackgres/operator/conciliation/cluster/ClusterConciliatorTest.java‎

Lines changed: 55 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,13 @@
2121
import java.util.Optional;
2222
import java.util.Random;
2323
import java.util.function.Predicate;
24+
import java.util.stream.Collectors;
2425

2526
import com.google.common.collect.ImmutableMap;
2627
import io.fabric8.kubernetes.api.model.HasMetadata;
2728
import io.fabric8.kubernetes.api.model.LoadBalancerIngressBuilder;
2829
import io.fabric8.kubernetes.api.model.LoadBalancerStatusBuilder;
30+
import io.fabric8.kubernetes.api.model.Pod;
2931
import io.fabric8.kubernetes.api.model.Service;
3032
import io.fabric8.kubernetes.api.model.ServiceSpecBuilder;
3133
import io.fabric8.kubernetes.api.model.ServiceStatusBuilder;
@@ -177,9 +179,12 @@ void whenThereIsNoChanges_allResourcesShouldBeEmpty() {
177179
foundDeployedResources);
178180

179181
ReconciliationResult result = conciliator.evalReconciliationState(cluster);
180-
assertEquals(0, result.getDeletions().size());
181-
assertEquals(0, result.getCreations().size());
182-
assertEquals(0, result.getPatches().size());
182+
assertEquals(0, result.getDeletions().size(),
183+
result.getDeletions().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
184+
assertEquals(0, result.getCreations().size(),
185+
result.getCreations().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
186+
assertEquals(0, result.getPatches().size(),
187+
result.getPatches().stream().map(t -> t.v2.getKind()).collect(Collectors.joining(", ")));
183188

184189
assertTrue(result.isUpToDate());
185190
}
@@ -323,9 +328,12 @@ void whenThereAreDeployedChangesOnMetadataOwnerReferences_shouldDoNothing() {
323328
foundDeployedResources);
324329

325330
ReconciliationResult result = conciliator.evalReconciliationState(cluster);
326-
assertEquals(0, result.getDeletions().size());
327-
assertEquals(0, result.getCreations().size());
328-
assertEquals(0, result.getPatches().size());
331+
assertEquals(0, result.getDeletions().size(),
332+
result.getDeletions().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
333+
assertEquals(0, result.getCreations().size(),
334+
result.getCreations().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
335+
assertEquals(0, result.getPatches().size(),
336+
result.getPatches().stream().map(t -> t.v2.getKind()).collect(Collectors.joining(", ")));
329337
}
330338

331339
@Test
@@ -342,6 +350,7 @@ void whenThereAreDeployedWithOtherMetadataOwnerReferences_shouldDoNoting() {
342350

343351
var updatedResource = Seq.seq(foundDeployedResources)
344352
.zipWithIndex()
353+
.filter(Predicate.not(t -> t.v1 instanceof Pod))
345354
.filter(t -> hasAnotherOwnerReference(t.v1))
346355
.sorted(shuffle())
347356
.findFirst()
@@ -356,9 +365,12 @@ void whenThereAreDeployedWithOtherMetadataOwnerReferences_shouldDoNoting() {
356365
foundDeployedResources);
357366

358367
ReconciliationResult result = conciliator.evalReconciliationState(cluster);
359-
assertEquals(0, result.getDeletions().size());
360-
assertEquals(0, result.getCreations().size());
361-
assertEquals(0, result.getPatches().size());
368+
assertEquals(0, result.getDeletions().size(),
369+
result.getDeletions().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
370+
assertEquals(0, result.getCreations().size(),
371+
result.getCreations().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
372+
assertEquals(0, result.getPatches().size(),
373+
result.getPatches().stream().map(t -> t.v2.getKind()).collect(Collectors.joining(", ")));
362374
}
363375

364376
@Test
@@ -461,9 +473,12 @@ void whenThereAreDeployedChangesOnMetadataResourceVersion_shouldNotBeDetected()
461473
foundDeployedResources);
462474

463475
ReconciliationResult result = conciliator.evalReconciliationState(cluster);
464-
assertEquals(0, result.getDeletions().size());
465-
assertEquals(0, result.getCreations().size());
466-
assertEquals(0, result.getPatches().size());
476+
assertEquals(0, result.getDeletions().size(),
477+
result.getDeletions().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
478+
assertEquals(0, result.getCreations().size(),
479+
result.getCreations().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
480+
assertEquals(0, result.getPatches().size(),
481+
result.getPatches().stream().map(t -> t.v2.getKind()).collect(Collectors.joining(", ")));
467482
}
468483

469484
@Test
@@ -494,9 +509,12 @@ void whenThereAreDeployedChangesOnStatefulSetStatus_shouldNotBeDetected() {
494509
foundDeployedResources);
495510

496511
ReconciliationResult result = conciliator.evalReconciliationState(cluster);
497-
assertEquals(0, result.getDeletions().size());
498-
assertEquals(0, result.getCreations().size());
499-
assertEquals(0, result.getPatches().size());
512+
assertEquals(0, result.getDeletions().size(),
513+
result.getDeletions().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
514+
assertEquals(0, result.getCreations().size(),
515+
result.getCreations().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
516+
assertEquals(0, result.getPatches().size(),
517+
result.getPatches().stream().map(t -> t.v2.getKind()).collect(Collectors.joining(", ")));
500518
}
501519

502520
@Test
@@ -531,9 +549,12 @@ void whenThereAreDeployedChangesOnServiceStatus_shouldNotBeDetected() {
531549
foundDeployedResources);
532550

533551
ReconciliationResult result = conciliator.evalReconciliationState(cluster);
534-
assertEquals(0, result.getDeletions().size());
535-
assertEquals(0, result.getCreations().size());
536-
assertEquals(0, result.getPatches().size());
552+
assertEquals(0, result.getDeletions().size(),
553+
result.getDeletions().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
554+
assertEquals(0, result.getCreations().size(),
555+
result.getCreations().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
556+
assertEquals(0, result.getPatches().size(),
557+
result.getPatches().stream().map(t -> t.v2.getKind()).collect(Collectors.joining(", ")));
537558
}
538559

539560
@Test
@@ -598,9 +619,12 @@ void conciliation_shouldIgnoreChangesOnResourcesMarkedWithReconciliationPauseAnn
598619

599620
ReconciliationResult result = conciliator.evalReconciliationState(cluster);
600621

601-
assertEquals(0, result.getDeletions().size());
602-
assertEquals(0, result.getCreations().size());
603-
assertEquals(0, result.getPatches().size());
622+
assertEquals(0, result.getDeletions().size(),
623+
result.getDeletions().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
624+
assertEquals(0, result.getCreations().size(),
625+
result.getCreations().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
626+
assertEquals(0, result.getPatches().size(),
627+
result.getPatches().stream().map(t -> t.v2.getKind()).collect(Collectors.joining(", ")));
604628

605629
assertTrue(result.isUpToDate());
606630
}
@@ -647,9 +671,12 @@ void conciliation_shouldIgnoreDeletionsOnResourcesMarkedWithReconciliationPauseA
647671
foundDeployedResources);
648672

649673
ReconciliationResult result = conciliator.evalReconciliationState(cluster);
650-
assertEquals(0, result.getCreations().size());
651-
assertEquals(0, result.getDeletions().size());
652-
assertEquals(0, result.getPatches().size());
674+
assertEquals(0, result.getDeletions().size(),
675+
result.getDeletions().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
676+
assertEquals(0, result.getCreations().size(),
677+
result.getCreations().stream().map(t -> t.getKind()).collect(Collectors.joining(", ")));
678+
assertEquals(0, result.getPatches().size(),
679+
result.getPatches().stream().map(t -> t.v2.getKind()).collect(Collectors.joining(", ")));
653680

654681
assertTrue(result.isUpToDate());
655682
}
@@ -733,16 +760,16 @@ protected ClusterConciliator buildConciliator(
733760
}
734761

735762
private boolean hasAnotherOwnerReference(HasMetadata resource) {
736-
return resource.getMetadata().getOwnerReferences() != null
737-
&& (resource.getMetadata().getOwnerReferences().isEmpty()
763+
return resource.getMetadata().getOwnerReferences() == null
764+
|| resource.getMetadata().getOwnerReferences().isEmpty()
738765
|| resource.getMetadata().getOwnerReferences().stream()
739766
.noneMatch(ownerReference -> ownerReference.getKind()
740767
.equals(HasMetadata.getKind(cluster.getClass()))
741768
&& ownerReference.getApiVersion().equals(HasMetadata.getApiVersion(cluster.getClass()))
742769
&& ownerReference.getName().equals(cluster.getMetadata().getName())
743770
&& ownerReference.getUid().equals(cluster.getMetadata().getUid())
744-
&& ownerReference.getController() != null
745-
&& ownerReference.getController()));
771+
&& ownerReference.getController() != null
772+
&& ownerReference.getController());
746773
}
747774

748775
private boolean hasControllerOwnerReference(HasMetadata resource) {

0 commit comments

Comments
 (0)