Repository: rancher/fleet

Issue:
Orphaned Bundledeployment Lifecycle Secrets Block Installation
I am installing 50 helm-style bundles, deleting the gitrepo and re-creating the gitrepo. The second time the installation will get stuck, unless I wait for all lifecycle secrets to disappear. I am using Fleet `v0.13.2-rc.2`. I am testing with 500 clusters (25k bundledeployments&secrets).
 
```
kubectl apply -n fleet-default -f - <<EOF
---
kind: GitRepo
apiVersion: fleet.cattle.io/v1alpha1
metadata:
  name: scale-50-single
spec:
  repo: https://github.com/manno/fleet-experiments
  branch: main
  paths:
    - scale-50-bundles/ten
    - scale-50-bundles/twenty
    - scale-50-bundles/thirty
    - scale-50-bundles/fourty
    - scale-50-bundles/fifty
  targetNamespace: scale-50-single
  targets:
    - clusterSelector: {}
EOF
```

After removing a GitRepo:
* the gitrepo is removed
* fleet-controller removes finalizers  from content resources, then it removes content resources
  * there are lot's of conflicts in the logs, but it works.
  * example only creates three (!) content resources total (`s-9d774f37b4a...` is used by 48 bundledeployments)
  * `fleet-controller-76cb85cf48-qshmp fleet-controller {"level":"error","msg":"Reconciler error","controller":"bundledeployment","controllerGroup":"fleet.cattle.io","controllerKind":"BundleDeployment","BundleDeployment":{"name":"scale-50-single-scale-50-bundles-twenty-seventeen","namespace":"cluster-fleet-default-d0-k3k-downstream002-downstream0161-5314b"},"namespace":"cluster-fleet-default-d0-k3k-downstream002-downstream0161-5314b","name":"scale-50-single-scale-50-bundles-twenty-seventeen","error":"Content.fleet.cattle.io \"s-9d774f37b4a7562f0f91c3acd6409fdee41209cb35d55f76aa1f381ebbb1f\" not found",...}`
* fleet-controller removes finalizers from bundledeployments, then it removes bundledeployments
* fleet-controller removes finalizer from bundles, then bundles are removed
* lifecycle secrets are left behind so that k8s garbage collection can remove them (orphaned owner?)
  * `kubectl get secrets -A --field-selector type=fleet.cattle.io/bundle-deployment/v1alpha1`
  * this takes easily 30 minutes for 25000 secrets



Ideas:
- [x] Installation should deal with existing lifecycle secrets and update the owner?
- [x] Deinstallation/clean up should not rely on k8s garbage selection
- [ ]  Removing finalizers should not print that many errors
- [ ] GitRepo finalizer should not be removed until deletion cascade (bundles, bundledeployments) are gone

Hints from issue comments:
@weyfonk: Created #4192 for remaining work, to be done as a follow-up. Closing this.

Solution patch:
diff --git a/README.md b/README.md
index 8855414a..371385f6 100644
--- a/README.md
+++ b/README.md
@@ -37,9 +37,9 @@ Install the Fleet Helm charts (there's two because we separate out CRDs for ulti
 
 ```shell
 helm -n cattle-fleet-system install --create-namespace --wait \
-    fleet-crd https://github.com/rancher/fleet/releases/download/v0.13.1/fleet-crd-0.13.1.tgz
+    fleet-crd https://github.com/rancher/fleet/releases/download/v0.13.2/fleet-crd-0.13.2.tgz
 helm -n cattle-fleet-system install --create-namespace --wait \
-    fleet https://github.com/rancher/fleet/releases/download/v0.13.1/fleet-0.13.1.tgz
+    fleet https://github.com/rancher/fleet/releases/download/v0.13.2/fleet-0.13.2.tgz
 ```
 
 ## Add a Git Repo to watch
diff --git a/charts/fleet-agent/Chart.yaml b/charts/fleet-agent/Chart.yaml
index 215427c4..a84826c6 100644
--- a/charts/fleet-agent/Chart.yaml
+++ b/charts/fleet-agent/Chart.yaml
@@ -1,11 +1,11 @@
 annotations:
   catalog.cattle.io/certified: rancher
   catalog.cattle.io/hidden: "true"
-  catalog.cattle.io/kube-version: '>= 1.28.0-0 < 1.34.0-0'
+  catalog.cattle.io/kube-version: '>= 1.32.0-0 < 1.35.0-0'
   catalog.cattle.io/namespace: cattle-fleet-system
   catalog.cattle.io/os: linux
   catalog.cattle.io/permits-os: linux,windows
-  catalog.cattle.io/rancher-version: '>= 2.12.0-0 < 2.13.0-0'
+  catalog.cattle.io/rancher-version: '>= 2.13.0-0 < 2.14.0-0'
   catalog.cattle.io/release-name: fleet-agent
 apiVersion: v2
 appVersion: 0.0.0
diff --git a/charts/fleet/Chart.yaml b/charts/fleet/Chart.yaml
index 71f31f92..ad77e6f9 100644
--- a/charts/fleet/Chart.yaml
+++ b/charts/fleet/Chart.yaml
@@ -3,12 +3,12 @@ annotations:
   catalog.cattle.io/certified: rancher
   catalog.cattle.io/experimental: "true"
   catalog.cattle.io/hidden: "true"
-  catalog.cattle.io/kube-version: '>= 1.28.0-0 < 1.34.0-0'
+  catalog.cattle.io/kube-version: '>= 1.32.0-0 < 1.35.0-0'
   catalog.cattle.io/namespace: cattle-fleet-system
   catalog.cattle.io/os: linux
   catalog.cattle.io/permits-os: linux,windows
   catalog.cattle.io/provides-gvr: clusters.fleet.cattle.io/v1alpha1
-  catalog.cattle.io/rancher-version: '>= 2.12.0-0 < 2.13.0-0'
+  catalog.cattle.io/rancher-version: '>= 2.13.0-0 < 2.14.0-0'
   catalog.cattle.io/release-name: fleet
 apiVersion: v2
 appVersion: 0.0.0
diff --git a/charts/fleet/templates/deployment.yaml b/charts/fleet/templates/deployment.yaml
index bdcdf376..35918e3e 100644
--- a/charts/fleet/templates/deployment.yaml
+++ b/charts/fleet/templates/deployment.yaml
@@ -251,6 +251,10 @@ spec:
 {{- if $.Values.tolerations }}
 {{ toYaml $.Values.tolerations | indent 8 }}
 {{- end }}
+      {{- with $.Values.affinity }}
+      affinity:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
       {{- if $.Values.priorityClassName }}
       priorityClassName: "{{$.Values.priorityClassName}}"
       {{- end }}
diff --git a/charts/fleet/templates/deployment_gitjob.yaml b/charts/fleet/templates/deployment_gitjob.yaml
index b73d3148..2b90a45e 100644
--- a/charts/fleet/templates/deployment_gitjob.yaml
+++ b/charts/fleet/templates/deployment_gitjob.yaml
@@ -134,6 +134,10 @@ spec:
 {{- if $.Values.tolerations }}
 {{ toYaml $.Values.tolerations | indent 8 }}
 {{- end }}
+      {{- with $.Values.affinity }}
+      affinity:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
       {{- if $.Values.priorityClassName }}
       priorityClassName: "{{$.Values.priorityClassName}}"
       {{- end }}
diff --git a/charts/fleet/templates/deployment_helmops.yaml b/charts/fleet/templates/deployment_helmops.yaml
index 998653f1..1b4dc45d 100644
--- a/charts/fleet/templates/deployment_helmops.yaml
+++ b/charts/fleet/templates/deployment_helmops.yaml
@@ -121,6 +121,10 @@ spec:
 {{- if $.Values.tolerations }}
 {{ toYaml $.Values.tolerations | indent 8 }}
 {{- end }}
+      {{- with $.Values.affinity }}
+      affinity:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
       {{- if $.Values.priorityClassName }}
       priorityClassName: "{{$.Values.priorityClassName}}"
       {{- end }}
diff --git a/charts/fleet/values.yaml b/charts/fleet/values.yaml
index ed704e40..c1efaf44 100644
--- a/charts/fleet/values.yaml
+++ b/charts/fleet/values.yaml
@@ -69,9 +69,13 @@ global:
 ## Ref: https://kubernetes.io/docs/user-guide/node-selection/
 ##
 nodeSelector: {}
+
 ## List of node taints to tolerate (requires Kubernetes >= 1.6)
 tolerations: []
 
+## Pod affinity for the controllers.
+affinity: {}
+
 ## PriorityClassName assigned to deployment.
 priorityClassName: ""
 
diff --git a/internal/cmd/cli/apply/apply.go b/internal/cmd/cli/apply/apply.go
index 0f3c27c4..857250ba 100644
--- a/internal/cmd/cli/apply/apply.go
+++ b/internal/cmd/cli/apply/apply.go
@@ -449,6 +449,7 @@ func writeBundle(ctx context.Context, c client.Client, r record.EventRecorder, b
 		valuesSecret := newValuesSecret(bundle, data)
 		updated := valuesSecret.DeepCopy()
 		_, err = controllerutil.CreateOrUpdate(ctx, c, valuesSecret, func() error {
+			valuesSecret.OwnerReferences = updated.OwnerReferences
 			valuesSecret.Labels = updated.Labels
 			valuesSecret.Data = updated.Data
 			valuesSecret.Type = updated.Type
diff --git a/internal/cmd/controller/agentmanagement/scheduling/scheduling.go b/internal/cmd/controller/agentmanagement/scheduling/scheduling.go
index 846cda96..d85cfeb4 100644
--- a/internal/cmd/controller/agentmanagement/scheduling/scheduling.go
+++ b/internal/cmd/controller/agentmanagement/scheduling/scheduling.go
@@ -29,25 +29,25 @@ func PriorityClass(priorityClass *fleet.PriorityClassSpec) *schedulingv1.Priorit
 	}
 }
 
-func PodDisruptionBudget(agentNamespace string, podDisruptionBudgetSpec *fleet.PodDisruptionBudgetSpec) (*policyv1.PodDisruptionBudget, error) {
+func PodDisruptionBudget(agentNamespace string, pdbs *fleet.PodDisruptionBudgetSpec) (*policyv1.PodDisruptionBudget, error) {
 	pdbSpec := policyv1.PodDisruptionBudgetSpec{
 		Selector: &metav1.LabelSelector{
 			MatchLabels: map[string]string{"app": "fleet-agent"},
 		},
 	}
 
-	if podDisruptionBudgetSpec.MaxUnavailable == "" && podDisruptionBudgetSpec.MinAvailable == "" {
+	if pdbs.MaxUnavailable == "" && pdbs.MinAvailable == "" {
 		logrus.Warnf("Neither MaxUnavailable nor MinAvailable is set, defaulting to 0 for MaxUnavailable")
 		pdbSpec.MaxUnavailable = &intstr.IntOrString{IntVal: 0}
-	} else if podDisruptionBudgetSpec.MaxUnavailable != "" && podDisruptionBudgetSpec.MinAvailable != "" {
-		return &policyv1.PodDisruptionBudget{},
-			fmt.Errorf("both MaxUnavailable (%s) and MinAvailable (%s) are set, not creating PDB", podDisruptionBudgetSpec.MaxUnavailable, podDisruptionBudgetSpec.MinAvailable)
-	} else if podDisruptionBudgetSpec.MaxUnavailable != "" {
-		mu := intstr.Parse(podDisruptionBudgetSpec.MaxUnavailable)
+	} else if pdbs.MaxUnavailable != "" && (pdbs.MinAvailable == "" || pdbs.MinAvailable == "0") {
+		mu := intstr.Parse(pdbs.MaxUnavailable)
 		pdbSpec.MaxUnavailable = &mu
-	} else if podDisruptionBudgetSpec.MinAvailable != "" {
-		mu := intstr.Parse(podDisruptionBudgetSpec.MinAvailable)
-		pdbSpec.MinAvailable = &mu
+	} else if pdbs.MinAvailable != "" && (pdbs.MaxUnavailable == "" || pdbs.MaxUnavailable == "0") {
+		ma := intstr.Parse(pdbs.MinAvailable)
+		pdbSpec.MinAvailable = &ma
+	} else if pdbs.MaxUnavailable != "" && pdbs.MinAvailable != "" {
+		return &policyv1.PodDisruptionBudget{},
+			fmt.Errorf("both MaxUnavailable (%s) and MinAvailable (%s) are set, not creating PDB", pdbs.MaxUnavailable, pdbs.MinAvailable)
 	}
 
 	return &policyv1.PodDisruptionBudget{
diff --git a/internal/cmd/controller/reconciler/bundle_controller.go b/internal/cmd/controller/reconciler/bundle_controller.go
index 3a97a43c..f2a008a7 100644
--- a/internal/cmd/controller/reconciler/bundle_controller.go
+++ b/internal/cmd/controller/reconciler/bundle_controller.go
@@ -38,6 +38,7 @@ import (
 	errutil "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
+	"k8s.io/utils/ptr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -347,7 +348,7 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		// and copy labels from Bundle as they might have changed.
 		// However, matchedTargets target.Deployment contains existing BundleDeployments.
 		bd := target.BundleDeployment()
-		logger = logger.WithValues("bundledeployment", bd.Name)
+		logger := logger.WithValues("bundledeployment", bd.Name)
 
 		// No need to check the deletion timestamp here before adding a finalizer, since the bundle has just
 		// been created.
@@ -460,6 +461,11 @@ func (r *BundleReconciler) handleDelete(ctx context.Context, logger logr.Logger,
 		return ctrl.Result{}, err
 	}
 
+	// pro-actively delete the bundle's secret. k8s owner garbage collection will handle remaining orphans.
+	if err := r.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: bundle.Name, Namespace: bundle.Namespace}}); err != nil && !apierrors.IsNotFound(err) {
+		logger.V(1).Info("Cannot delete bundle's values secret, owner garbage collection will remove it")
+	}
+
 	return ctrl.Result{}, r.maybeDeleteOCIArtifact(ctx, bundle)
 }
 
@@ -475,13 +481,13 @@ func (r *BundleReconciler) ensureFinalizer(ctx context.Context, bundle *fleet.Bu
 
 func (r *BundleReconciler) createBundleDeployment(
 	ctx context.Context,
-	logger logr.Logger,
+	l logr.Logger,
 	bd *fleet.BundleDeployment,
 	contentsInOCI bool,
 	contentsInHelmChart bool,
 	manifestID string,
 ) (*fleet.BundleDeployment, error) {
-	logger = logger.WithValues("deploymentID", bd.Spec.DeploymentID)
+	logger := l.WithValues("deploymentID", bd.Spec.DeploymentID)
 
 	// When content resources are stored in etcd, we need to add finalizers.
 	if !contentsInOCI && !contentsInHelmChart {
@@ -565,12 +571,19 @@ func (r *BundleReconciler) createOptionsSecret(ctx context.Context, bd *fleet.Bu
 			Namespace: bd.Namespace,
 		},
 	}
-
-	if err := controllerutil.SetControllerReference(bd, secret, r.Scheme); err != nil {
-		return err
+	owners := []metav1.OwnerReference{
+		{
+			APIVersion:         fleet.SchemeGroupVersion.String(),
+			Kind:               "BundleDeployment",
+			Name:               bd.GetName(),
+			UID:                bd.GetUID(),
+			BlockOwnerDeletion: ptr.To(true),
+			Controller:         ptr.To(true),
+		},
 	}
 
 	if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, secret, func() error {
+		secret.OwnerReferences = owners
 		secret.Data = map[string][]byte{
 			helmvalues.ValuesKey:       options,
 			helmvalues.StagedValuesKey: stagedOptions,
@@ -756,6 +769,10 @@ func batchDeleteBundleDeployments(ctx context.Context, c client.Client, list []f
 		if err := c.Delete(ctx, &bd); client.IgnoreNotFound(err) != nil {
 			errs = append(errs, err)
 		}
+
+		// k8s ownership garbage collection can take a long time, so we explicitly delete the secrets here.
+		// GC will delete any remaining orphaned secrets, no need to add an error.
+		_ = c.Delete(ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: bd.Name, Namespace: bd.Namespace}})
 	}
 
 	return errors.Join(errs...)



Test patch:
diff --git a/charts/fleet/tests/affinity_test.yaml b/charts/fleet/tests/affinity_test.yaml
new file mode 100644
index 00000000..c1cb2da9
--- /dev/null
+++ b/charts/fleet/tests/affinity_test.yaml
@@ -0,0 +1,52 @@
+suite: test affinity rules
+templates:
+  - deployment.yaml
+  - deployment_gitjob.yaml
+  - deployment_helmops.yaml
+tests:
+  - it: should set affinity rules
+    set:
+      affinity:
+        nodeAffinity:
+          requiredDuringSchedulingIgnoredDuringExecution:
+            nodeSelectorTerms:
+            - matchExpressions:
+                - key: disktype
+                  operator: In
+                  values:
+                    - ssd
+        podAffinity:
+          requiredDuringSchedulingIgnoredDuringExecution:
+            - labelSelector:
+                matchExpressions:
+                  - key: security
+                    operator: In
+                    values:
+                      - S1
+              topologyKey: "kubernetes.io/hostname"
+        podAntiAffinity:
+          requiredDuringSchedulingIgnoredDuringExecution:
+            - labelSelector:
+                matchExpressions:
+                  - key: app
+                    operator: In
+                    values:
+                      - fleet
+              topologyKey: "kubernetes.io/hostname"
+    asserts:
+      - equal:
+          path: spec.template.spec.affinity.nodeAffinity.requiredDuringSchedulingIgnoredDuringExecution.nodeSelectorTerms[0].matchExpressions[0].key
+          value: disktype
+      - equal:
+          path: spec.template.spec.affinity.podAffinity.requiredDuringSchedulingIgnoredDuringExecution[0].labelSelector.matchExpressions[0].key
+          value: security
+      - equal:
+          path: spec.template.spec.affinity.podAntiAffinity.requiredDuringSchedulingIgnoredDuringExecution[0].labelSelector.matchExpressions[0].key
+          value: app
+
+  - it: should work without affinity rules
+    set:
+      affinity: {}
+    asserts:
+      - notExists:
+          path: spec.template.spec.affinity
diff --git a/internal/cmd/controller/agentmanagement/scheduling/scheduling_test.go b/internal/cmd/controller/agentmanagement/scheduling/scheduling_test.go
index c4607150..5fe1814f 100644
--- a/internal/cmd/controller/agentmanagement/scheduling/scheduling_test.go
+++ b/internal/cmd/controller/agentmanagement/scheduling/scheduling_test.go
@@ -50,6 +50,31 @@ func TestPodDisruptionBudget(t *testing.T) {
 			spec:    &fleet.PodDisruptionBudgetSpec{MinAvailable: "1", MaxUnavailable: "1"},
 			wantErr: true,
 		},
+		{
+			name:   "having both values, MinAvailable as zero, should disable the zero value",
+			spec:   &fleet.PodDisruptionBudgetSpec{MinAvailable: "0", MaxUnavailable: "1"},
+			wantMU: ptr(intstr.FromInt(1)),
+		},
+		{
+			name:   "having both values, MaxUnavailable as zero, should disable the zero value",
+			spec:   &fleet.PodDisruptionBudgetSpec{MinAvailable: "1", MaxUnavailable: "0"},
+			wantMA: ptr(intstr.FromInt(1)),
+		},
+		{
+			name:   "having a percent value while MinAvailable is zero should disable the zero value",
+			spec:   &fleet.PodDisruptionBudgetSpec{MinAvailable: "0", MaxUnavailable: "10%"},
+			wantMU: ptr(intstr.FromString("10%")),
+		},
+		{
+			name:   "having a percent value while MaxUnavailable is zero should disable the zero value",
+			spec:   &fleet.PodDisruptionBudgetSpec{MinAvailable: "10%", MaxUnavailable: "0"},
+			wantMA: ptr(intstr.FromString("10%")),
+		},
+		{
+			name:    "two non-zero values shouldn't work",
+			spec:    &fleet.PodDisruptionBudgetSpec{MinAvailable: "1", MaxUnavailable: "1"},
+			wantErr: true,
+		},
 	}
 
 	for _, test := range tests {