From 63dfb7b9f4af42b3941f0d77601ef6986fc974ac Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Sat, 15 Aug 2026 17:15:25 +0200 Subject: [PATCH] (fix) four ClusterSummary reconcile bugs found via scale/deletion testing Four separate bug fixes: 1. isClusterPresent: transient GetCluster errors treated as "cluster present" isClusterPresent returned early on apierrors.IsNotFound, but any other error (a transient timeout, an API server hiccup) fell through to `return true, !cluster.GetDeletionTimestamp().IsZero(), err` with cluster still nil - reporting present=true alongside a non-nil error. The caller (reconcileDelete) then silently requeues forever on that error without ever reaching handleDeletedCluster/removeFinalizer, so a ClusterSummary whose SveltosCluster is actually gone can get stuck Terminating indefinitely if a single transient error hits at the wrong moment. Now returns (false, false, err) on any non-NotFound error instead. 2. removeStaleResourceSummary: one cluster's ResourceSummary blocked another's cleanup. The ResourceSummary List is scoped only by ClusterNameLabel/ClusterTypeLabel, not namespace so multiple clusters sharing the same name across different namespaces (a normal setup) all come back in one List. The function correctly filtered which ones to delete by namespace, but the "still present" gate checked the length of the unfiltered list, so an unrelated cluster's still-finalizing ResourceSummary permanently blocked this cluster's own cleanup. Now counts only the entries that actually match this cluster's namespace. 3. LocateChart: no timeout on the chart download call Unlike repo.ChartRepository.DownloadIndexFile a few lines away (already wrapped in a goroutine+timeout because the Helm SDK's repo APIs take no context.Context), action.Install/Upgrade's LocateChart was called directly, twice, with no timeout at all. A repository that accepts the connection but never completes the response wedges the deployer worker processing that request forever. The request only leaves the deployer's inProgress tracking once the handler call returns, so a call that never returns permanently occupies one of the worker pool's fixed slots, and its ClusterSummary/feature is stuck reporting "still being provisioned" with no further retries, indefinitely. Wrapped LocateChart in the same goroutine+channel+90s-timeout pattern as DownloadIndexFile. 4. dependsOn only protected deploy order, not deletion prepareForDeployment already blocks a dependent from deploying until its prerequisites are Provisioned (areDependenciesDeployed), but nothing enforced the reverse on delete: a prerequisite (e.g. cert-manager) and its dependent (e.g. kyverno, via dependsOn) would undeploy concurrently as soon as their own ClusterProfiles were deleted, in random order. Added areDependentsRemoved: a ClusterSummary's undeploy is now blocked while another ClusterSummary for the same cluster still exists and still lists this profile in its own dependsOn - dependsOn is treated as a two-way contract (gates deploy readiness AND protects against deletion while still needed), the same pattern Kubernetes itself uses for a CRD with live CRs or a PVC still mounted by a pod. The blocked ClusterSummary surfaces why via Status.Dependencies. --- controllers/clustersummary_controller.go | 76 ++++++++++- controllers/clustersummary_controller_test.go | 73 +++++++++++ controllers/export_test.go | 2 + controllers/handlers_helm.go | 38 +++++- controllers/handlers_helm_test.go | 24 ++++ controllers/utils.go | 19 ++- controllers/utils_test.go | 90 +++++++++++++ test/fv/dependencies_test.go | 120 +++++++++++++++++- 8 files changed, 433 insertions(+), 9 deletions(-) diff --git a/controllers/clustersummary_controller.go b/controllers/clustersummary_controller.go index 9d8b87ff..f2cac949 100644 --- a/controllers/clustersummary_controller.go +++ b/controllers/clustersummary_controller.go @@ -367,6 +367,18 @@ func (r *ClusterSummaryReconciler) reconcileDelete( } } + // Mirror the dependsOn ordering enforced on deploy: do not undeploy a prerequisite + // while a dependent ClusterSummary for the same cluster still exists and still + // requires it. + allRemoved, dependentMsg, err := r.areDependentsRemoved(ctx, clusterSummaryScope, logger) + if err != nil { + return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil + } + clusterSummaryScope.SetDependenciesMessage(&dependentMsg) + if !allRemoved { + return reconcile.Result{Requeue: true, RequeueAfter: deleteRequeueAfter}, nil + } + // still call undeploy even if cluster is deleted. Sveltos might have deployed resources // in the management cluster and those need to be removed. err = r.undeploy(ctx, clusterSummaryScope, logger) @@ -902,15 +914,15 @@ func (r *ClusterSummaryReconciler) isClusterPresent(ctx context.Context, cs := clusterSummaryScope.ClusterSummary - var cluster client.Object - cluster, err = clusterproxy.GetCluster(ctx, r.Client, cs.Spec.ClusterNamespace, cs.Spec.ClusterName, cs.Spec.ClusterType) + cluster, err := clusterproxy.GetCluster(ctx, r.Client, cs.Spec.ClusterNamespace, cs.Spec.ClusterName, cs.Spec.ClusterType) if err != nil { if apierrors.IsNotFound(err) { return false, false, nil } + return false, false, err } - return true, !cluster.GetDeletionTimestamp().IsZero(), err + return true, !cluster.GetDeletionTimestamp().IsZero(), nil } func (r *ClusterSummaryReconciler) undeploy(ctx context.Context, clusterSummaryScope *scope.ClusterSummaryScope, @@ -1755,6 +1767,64 @@ func (r *ClusterSummaryReconciler) areDependenciesDeployed(ctx context.Context, return true, dependencyMessage, nil } +// areDependentsRemoved is the mirror check of areDependenciesDeployed, run on delete instead of +// deploy. dependsOn is a two-way contract, not just a one-time deploy-ordering hint: +// prepareForDeployment blocks a dependent from deploying until its prerequisites are Provisioned, +// and symmetrically, a prerequisite (e.g. cert-manager) must not be undeployed while a dependent +// that still needs it (e.g. kyverno) is still around - deleting it out from under a live dependent +// would silently break that dependent, with no error at the moment that actually caused it. This +// mirrors how Kubernetes itself protects still-referenced objects (a CRD while CRs of that type +// exist, a PVC while a pod mounts it): the block is deliberate, and lifts once the dependent is +// gone or no longer lists this profile as a dependency. +// +// Returns false, with a message, if another ClusterSummary for the same cluster still exists and +// still lists this ClusterSummary's profile in its own DependsOn. +func (r *ClusterSummaryReconciler) areDependentsRemoved(ctx context.Context, clusterSummaryScope *scope.ClusterSummaryScope, + logger logr.Logger) (allRemoved bool, dependentMessage string, err error) { + + profileReference, err := configv1beta1.GetProfileOwnerReference(clusterSummaryScope.ClusterSummary) + if err != nil { + logger.V(logs.LogInfo).Info(fmt.Sprintf("failed to get profile owner: %v", err)) + return false, "", fmt.Errorf("failed to get profile owner: %w", err) + } + + if profileReference == nil { + return false, "", fmt.Errorf("profile owner not found: %w", err) + } + + clusterSummary := clusterSummaryScope.ClusterSummary + + listOptions := []client.ListOption{ + client.InNamespace(clusterSummary.Spec.ClusterNamespace), + client.MatchingLabels{ + configv1beta1.ClusterNameLabel: clusterSummary.Spec.ClusterName, + configv1beta1.ClusterTypeLabel: string(clusterSummary.Spec.ClusterType), + }, + } + + clusterSummaryList := &configv1beta1.ClusterSummaryList{} + if err := r.List(ctx, clusterSummaryList, listOptions...); err != nil { + return false, "", err + } + + for i := range clusterSummaryList.Items { + other := &clusterSummaryList.Items[i] + if other.Name == clusterSummary.Name { + continue + } + + for j := range other.Spec.ClusterProfileSpec.DependsOn { + if other.Spec.ClusterProfileSpec.DependsOn[j] == profileReference.Name { + msg := fmt.Sprintf("%s still depends on this profile and has not been removed yet", other.Name) + logger.V(logs.LogInfo).Info(msg) + return false, msg, nil + } + } + } + + return true, "", nil +} + func (r *ClusterSummaryReconciler) setFailureMessage(clusterSummaryScope *scope.ClusterSummaryScope, failureMessage string) { if clusterSummaryScope.ClusterSummary.Spec.ClusterProfileSpec.HelmCharts != nil { clusterSummaryScope.SetFailureMessage(libsveltosv1beta1.FeatureHelm, &failureMessage) diff --git a/controllers/clustersummary_controller_test.go b/controllers/clustersummary_controller_test.go index fe85c230..b85d3baa 100644 --- a/controllers/clustersummary_controller_test.go +++ b/controllers/clustersummary_controller_test.go @@ -1108,6 +1108,79 @@ var _ = Describe("ClustersummaryController", func() { Expect(deployed).To(BeTrue()) }) + It("areDependentsRemoved returns false while a dependent ClusterSummary still exists", func() { + // clusterSummary/clusterProfile (from BeforeEach) play the role of the prerequisite + // (e.g. cert-manager). dependentSummary plays the dependent (e.g. kyverno): it still + // lists clusterProfile.Name in its own DependsOn, so the prerequisite must not be + // undeployed while it's still around - dependsOn protects the prerequisite from + // deletion, it's not just a one-time deploy-ordering hint. + dependentProfileName := randomString() + dependentSummaryName := clusterops.GetClusterSummaryName(configv1beta1.ClusterProfileKind, + dependentProfileName, clusterName, false) + dependentSummary := &configv1beta1.ClusterSummary{ + ObjectMeta: metav1.ObjectMeta{ + Name: dependentSummaryName, + Namespace: namespace, + Labels: map[string]string{ + clusterops.ClusterProfileLabelName: dependentProfileName, + configv1beta1.ClusterNameLabel: clusterName, + configv1beta1.ClusterTypeLabel: string(libsveltosv1beta1.ClusterTypeCapi), + }, + }, + Spec: configv1beta1.ClusterSummarySpec{ + ClusterNamespace: cluster.Namespace, + ClusterName: cluster.Name, + ClusterType: libsveltosv1beta1.ClusterTypeCapi, + ClusterProfileSpec: configv1beta1.Spec{ + DependsOn: []string{clusterProfile.Name}, + }, + }, + } + + initObjects := []client.Object{ + dependentSummary, + clusterSummary, + clusterProfile, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(initObjects...).WithObjects(initObjects...).Build() + + addOwnerReference(context.TODO(), c, clusterSummary, clusterProfile) + + deployer := fakedeployer.GetClient(context.TODO(), textlogger.NewLogger(textlogger.NewConfig()), c) + reconciler := &controllers.ClusterSummaryReconciler{ + Client: c, + Scheme: scheme, + Deployer: deployer, + ClusterMap: make(map[corev1.ObjectReference]*libsveltosset.Set), + ReferenceMap: make(map[corev1.ObjectReference]*libsveltosset.Set), + PolicyMux: sync.Mutex{}, + } + + clusterSummaryScope, err := scope.NewClusterSummaryScope(&scope.ClusterSummaryScopeParams{ + Client: c, + Logger: textlogger.NewLogger(textlogger.NewConfig()), + ClusterSummary: clusterSummary, + ControllerName: testControllerNameSummary, + }) + Expect(err).To(BeNil()) + + // because the dependent ClusterSummary still exists and still depends on this profile + removed, msg, err := controllers.AreDependentsRemoved(reconciler, context.TODO(), clusterSummaryScope, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + Expect(removed).To(BeFalse()) + Expect(msg).To(ContainSubstring(dependentSummaryName)) + + // once the dependent ClusterSummary is gone, the prerequisite is free to undeploy + Expect(c.Delete(context.TODO(), dependentSummary)).To(Succeed()) + + removed, _, err = controllers.AreDependentsRemoved(reconciler, context.TODO(), clusterSummaryScope, + textlogger.NewLogger(textlogger.NewConfig())) + Expect(err).To(BeNil()) + Expect(removed).To(BeTrue()) + }) + It("processUndeployError requeues with deleteRequeueAfter and no error for WaitForProfileProcessingError", func() { initObjects := []client.Object{ clusterProfile, diff --git a/controllers/export_test.go b/controllers/export_test.go index 26b82f4c..877c42b1 100644 --- a/controllers/export_test.go +++ b/controllers/export_test.go @@ -70,6 +70,7 @@ var ( CanRemoveFinalizer = (*ClusterSummaryReconciler).canRemoveFinalizer ReconcileDelete = (*ClusterSummaryReconciler).reconcileDelete AreDependenciesDeployed = (*ClusterSummaryReconciler).areDependenciesDeployed + AreDependentsRemoved = (*ClusterSummaryReconciler).areDependentsRemoved SetFailureMessage = (*ClusterSummaryReconciler).setFailureMessage ResetFeatureStatus = (*ClusterSummaryReconciler).resetFeatureStatus @@ -152,6 +153,7 @@ var ( GetInstantiatedChart = getInstantiatedChart GetHelmChartValuesFrom = getHelmChartValuesFrom GetHelmChartInstantiatedValues = getHelmChartInstantiatedValues + LocateChartWithTimeout = locateChartWithTimeout InstantiateTemplateValues = instantiateTemplateValues FetchClusterObjects = fetchClusterObjects diff --git a/controllers/handlers_helm.go b/controllers/handlers_helm.go index f4d4eb9b..41d398c8 100644 --- a/controllers/handlers_helm.go +++ b/controllers/handlers_helm.go @@ -2052,6 +2052,40 @@ func installRelease(ctx context.Context, clusterSummary *configv1beta1.ClusterSu return rel, nil } +const ( + // locateChartTimeout bounds a single LocateChart call. Like repo.ChartRepository.DownloadIndexFile, + // action.Install/Upgrade's LocateChart takes no context.Context - it downloads the chart tarball with + // its own HTTP client, so nothing here can cancel a stalled call. Without this timeout, a repository + // that accepts the connection but never completes the response wedges the deployer worker processing + // this request forever: the request only leaves the deployer's inProgress tracking once the handler + // call returns, so a call that never returns permanently occupies one of the worker pool's fixed + // slots and its ClusterSummary/feature is stuck reporting "still being provisioned" with no further + // retries, indefinitely. + locateChartTimeout = 90 * time.Second +) + +func locateChartWithTimeout(locateFn func(string, *cli.EnvSettings) (string, error), + chartName string, settings *cli.EnvSettings) (string, error) { + + type locateResult struct { + path string + err error + } + + done := make(chan locateResult, 1) + go func() { + cp, err := locateFn(chartName, settings) + done <- locateResult{cp, err} + }() + + select { + case r := <-done: + return r.path, r.err + case <-time.After(locateChartTimeout): + return "", fmt.Errorf("timed out locating chart %s", chartName) + } +} + // locateChartWithCacheRetry calls locateFn to resolve the chart path. If the call fails (e.g. // because the local repository index is stale and does not yet contain a newly published version), // the cache is cleared and the call is retried once so that the current reconciliation can succeed @@ -2062,12 +2096,12 @@ func locateChartWithCacheRetry( requestedChart *configv1beta1.HelmChart, registryOptions *registryClientOptions, logger logr.Logger) (string, error) { - cp, err := locateFn(chartName, settings) + cp, err := locateChartWithTimeout(locateFn, chartName, settings) if err != nil { logger.V(logs.LogInfo).Info(fmt.Sprintf("LocateChart failed: %v; refreshing cache and retrying", err)) removeCachedData(settings, requestedChart.RepositoryName, requestedChart.RepositoryURL, registryOptions, logger) - cp, err = locateFn(chartName, settings) + cp, err = locateChartWithTimeout(locateFn, chartName, settings) } return cp, err } diff --git a/controllers/handlers_helm_test.go b/controllers/handlers_helm_test.go index 2b926f42..299c8e8f 100644 --- a/controllers/handlers_helm_test.go +++ b/controllers/handlers_helm_test.go @@ -32,6 +32,7 @@ import ( . "github.com/onsi/gomega" "helm.sh/helm/v4/pkg/action" + "helm.sh/helm/v4/pkg/cli" releasecommon "helm.sh/helm/v4/pkg/release/common" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -2755,3 +2756,26 @@ var _ = Describe("getHelmUpgradeClient Force", func() { Expect(upgradeClient.ForceConflicts).To(BeTrue()) }) }) + +var _ = Describe("locateChartWithTimeout", func() { + It("returns the result once locateFn returns", func() { + expectedPath := randomString() + locateFn := func(chartName string, settings *cli.EnvSettings) (string, error) { + return expectedPath, nil + } + + cp, err := controllers.LocateChartWithTimeout(locateFn, randomString(), &cli.EnvSettings{}) + Expect(err).To(BeNil()) + Expect(cp).To(Equal(expectedPath)) + }) + + It("propagates the error returned by locateFn", func() { + expectedErr := errors.New(randomString()) + locateFn := func(chartName string, settings *cli.EnvSettings) (string, error) { + return "", expectedErr + } + + _, err := controllers.LocateChartWithTimeout(locateFn, randomString(), &cli.EnvSettings{}) + Expect(err).To(Equal(expectedErr)) + }) +}) diff --git a/controllers/utils.go b/controllers/utils.go index 6285ceb3..70e32b11 100644 --- a/controllers/utils.go +++ b/controllers/utils.go @@ -458,14 +458,27 @@ func removeStaleResourceSummary(ctx context.Context, clusterNamespace, clusterNa return err } + // rsListOptions only scopes by cluster name/type, not namespace - client-go label + // selectors can't match on ClusterSummary namespace, so multiple clusters sharing the + // same name across different namespaces (a normal setup) all come back in one List. + // matchingCount tracks only the ones that are ours to wait on: known to belong to + // clusterNamespace, or - conservatively - ones whose namespace we can't determine at + // all. That keeps an unrelated cluster's still-finalizing ResourceSummary (a namespace + // we can positively rule out) from blocking this one's cleanup, without letting an + // unidentifiable ResourceSummary be ignored outright. + matchingCount := 0 for i := range resourceSummaries.Items { rs := &resourceSummaries.Items[i] ns, ok := getClusterSummaryNamespaceFromResourceSummary(rs, logger) - if !ok { + if ok && ns != clusterNamespace { continue } - if ns != clusterNamespace { + matchingCount++ + + if !ok { + // Can't tell which cluster this belongs to, so don't risk deleting it - just + // count it, so the caller waits rather than proceeding as if it weren't there. continue } @@ -477,7 +490,7 @@ func removeStaleResourceSummary(ctx context.Context, clusterNamespace, clusterNa } } - if len(resourceSummaries.Items) > 0 { + if matchingCount > 0 { // The drift-detection-manager adds a finalizer to ResourceSummary instances and removes // it only after a successful deletion. If ResourceSummary instances still exist, we return // an error to prevent the drift-detection-manager deployment from being deleted, ensuring diff --git a/controllers/utils_test.go b/controllers/utils_test.go index b188c8cd..805c0899 100644 --- a/controllers/utils_test.go +++ b/controllers/utils_test.go @@ -548,6 +548,96 @@ metadata: }, timeout, pollingInterval).Should(BeTrue()) }) + It("removeStaleResourceSummary is not blocked by another cluster's ResourceSummary sharing the same name", + func() { + // Two clusters registered under the same name/type but different namespaces - + // a normal setup, e.g. "prod" registered in multiple tenant namespaces. The + // ClusterNameLabel/ClusterTypeLabel selector used to List ResourceSummary + // instances cannot distinguish between them, so both come back in one List. + clusterName := randomString() + clusterType := libsveltosv1beta1.ClusterTypeSveltos + + targetNamespace := randomString() + otherNamespace := randomString() + + logger := textlogger.NewLogger(textlogger.NewConfig()) + + for _, ns := range []string{targetNamespace, otherNamespace} { + n := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: ns, + }, + } + Expect(testEnv.Create(context.TODO(), n)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, n)).To(Succeed()) + } + + targetRS := fmt.Sprintf(`apiVersion: lib.projectsveltos.io/v1beta1 +kind: ResourceSummary +metadata: + labels: + projectsveltos.io/cluster-summary-namespace: %s + version.projectsveltos.io/clustername: %s + version.projectsveltos.io/clustertype: %s + name: deploy-cert-manager-sveltos-cluster1 + namespace: %s +`, targetNamespace, clusterName, strings.ToLower(string(clusterType)), targetNamespace) + + // otherRS belongs to a different cluster (different namespace) that just + // happens to share the same clusterName/clusterType. It must survive the + // call below untouched: it is not what removeStaleResourceSummary(targetNamespace, ...) + // was asked to clean up, and its mere existence must not block that cleanup either. + otherRS := fmt.Sprintf(`apiVersion: lib.projectsveltos.io/v1beta1 +kind: ResourceSummary +metadata: + labels: + projectsveltos.io/cluster-summary-namespace: %s + version.projectsveltos.io/clustername: %s + version.projectsveltos.io/clustertype: %s + name: deploy-cert-manager-sveltos-cluster1 + namespace: %s +`, otherNamespace, clusterName, strings.ToLower(string(clusterType)), otherNamespace) + + target, err := k8s_utils.GetUnstructured([]byte(targetRS)) + Expect(err).To(BeNil()) + Expect(testEnv.Create(context.TODO(), target)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, target)).To(Succeed()) + + other, err := k8s_utils.GetUnstructured([]byte(otherRS)) + Expect(err).To(BeNil()) + Expect(testEnv.Create(context.TODO(), other)).To(Succeed()) + Expect(waitForObject(context.TODO(), testEnv.Client, other)).To(Succeed()) + + // First call finds and deletes targetNamespace's own ResourceSummary, so - same as + // the sibling test above - it reports "still present" to give that deletion a + // chance to actually complete before the caller (e.g. drift-detection-manager + // removal) proceeds. That's expected here too; it is not what this test is about. + err = controllers.RemoveStaleResourceSummary(context.TODO(), targetNamespace, clusterName, + clusterType, logger) + Expect(err).ToNot(BeNil()) + Expect(err.Error()).To(ContainSubstring("resourceSummary instances still present")) + + // targetNamespace's ResourceSummary had no finalizer, so it should be fully gone. + currentRS := &libsveltosv1beta1.ResourceSummary{} + Eventually(func() bool { + err = testEnv.Get(context.TODO(), + types.NamespacedName{Namespace: target.GetNamespace(), Name: target.GetName()}, currentRS) + return apierrors.IsNotFound(err) + }, timeout, pollingInterval).Should(BeTrue()) + + // otherNamespace's ResourceSummary belongs to a different cluster and must be untouched. + Expect(testEnv.Get(context.TODO(), + types.NamespacedName{Namespace: other.GetNamespace(), Name: other.GetName()}, currentRS)).To(Succeed()) + + // Now targetNamespace has nothing left of its own. This is the actual regression + // this test guards: otherNamespace's still-existing ResourceSummary shares + // clusterName/clusterType and would previously have kept this call reporting "still + // present" forever, even though nothing belonging to targetNamespace remains. + err = controllers.RemoveStaleResourceSummary(context.TODO(), targetNamespace, clusterName, + clusterType, logger) + Expect(err).To(BeNil()) + }) + It("removeStaleDriftDetectionResources deletes drift-detection-manager after all ResourceSummaries are gone", func(ctx SpecContext) { namespace := randomString() diff --git a/test/fv/dependencies_test.go b/test/fv/dependencies_test.go index a7011d2a..c88647ac 100644 --- a/test/fv/dependencies_test.go +++ b/test/fv/dependencies_test.go @@ -18,9 +18,11 @@ package fv_test import ( "context" + "strings" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" @@ -32,6 +34,20 @@ import ( var _ = Describe("Dependencies", func() { const ( namePrefix = "dependencies-" + + // Charts for "prerequisite deletion waits for dependent" below. Deliberately not + // Bitnami (unreliable chart-index downloads in CI) and not cert-manager/kyverno + // (already used by other dependsOn FV tests). + metricsServerRepoURL = "https://kubernetes-sigs.github.io/metrics-server/" + metricsServerRepoName = "metrics-server" + metricsServerChartName = "metrics-server/metrics-server" + metricsServerVersion = "3.13.1" + metricsServerRelease = "metrics-server" + reloaderChartRepoURL = "https://stakater.github.io/stakater-charts" + reloaderChartRepoName = "stakater" + reloaderHelmChartName = "stakater/reloader" + reloaderHelmVersion = "2.2.16" + reloaderHelmReleaseName = "stakater-reloader" ) It("ClusterProfile with dependencies is deployed after dependencies are provisioned", @@ -157,8 +173,110 @@ migrateDatabaseJob: Byf("Verifying ClusterSummary %s status is set to Deployed for Helm feature", clusterSummary.Name) verifyFeatureStatusIsProvisioned(kindWorkloadCluster.GetNamespace(), clusterSummary.Name, libsveltosv1beta1.FeatureHelm) + // dependsOn protects the prerequisite from deletion while its dependent still + // needs it: the dependent must be removed first. + deleteClusterProfile(clusterProfile) + deleteClusterProfile(clusterProfileDependency) + }) - deleteClusterProfile(clusterProfile) + It("Deleting a prerequisite ClusterProfile waits for its dependent to be removed first", + Label("FV", "PULLMODE", "EXTENDED"), func() { + Byf("Create a ClusterProfile matching Cluster %s/%s to be used as a prerequisite", + kindWorkloadCluster.GetNamespace(), kindWorkloadCluster.GetName()) + prerequisiteProfile := getClusterProfile(namePrefix, map[string]string{key: value}) + prerequisiteProfile.Spec.SyncMode = configv1beta1.SyncModeContinuous + prerequisiteProfile.Spec.HelmCharts = []configv1beta1.HelmChart{ + { + RepositoryURL: metricsServerRepoURL, + RepositoryName: metricsServerRepoName, + ChartName: metricsServerChartName, + ChartVersion: metricsServerVersion, + ReleaseName: metricsServerRelease, + ReleaseNamespace: metricsServerRelease, + HelmChartAction: configv1beta1.HelmChartActionInstall, + }, + } + Expect(k8sClient.Create(context.TODO(), prerequisiteProfile)).To(Succeed()) + verifyClusterProfileMatches(prerequisiteProfile) + prerequisiteSummary := verifyClusterSummary(clusterops.ClusterProfileLabelName, prerequisiteProfile.Name, + &prerequisiteProfile.Spec, kindWorkloadCluster.GetNamespace(), kindWorkloadCluster.GetName(), getClusterType()) + + Byf("Create a ClusterProfile matching Cluster %s/%s, depending on %s", + kindWorkloadCluster.GetNamespace(), kindWorkloadCluster.GetName(), prerequisiteProfile.Name) + dependentProfile := getClusterProfile(namePrefix, map[string]string{key: value}) + dependentProfile.Spec.SyncMode = configv1beta1.SyncModeContinuous + dependentProfile.Spec.DependsOn = []string{prerequisiteProfile.Name} + dependentProfile.Spec.HelmCharts = []configv1beta1.HelmChart{ + { + RepositoryURL: reloaderChartRepoURL, + RepositoryName: reloaderChartRepoName, + ChartName: reloaderHelmChartName, + ChartVersion: reloaderHelmVersion, + ReleaseName: reloaderHelmReleaseName, + ReleaseNamespace: reloaderHelmReleaseName, + HelmChartAction: configv1beta1.HelmChartActionInstall, + }, + } + Expect(k8sClient.Create(context.TODO(), dependentProfile)).To(Succeed()) + verifyClusterProfileMatches(dependentProfile) + dependentSummary := verifyClusterSummary(clusterops.ClusterProfileLabelName, dependentProfile.Name, + &dependentProfile.Spec, kindWorkloadCluster.GetNamespace(), kindWorkloadCluster.GetName(), getClusterType()) + + Byf("Verifying ClusterSummary %s status is set to Deployed for Helm feature", prerequisiteSummary.Name) + verifyFeatureStatusIsProvisioned(kindWorkloadCluster.GetNamespace(), prerequisiteSummary.Name, libsveltosv1beta1.FeatureHelm) + + Byf("Verifying ClusterSummary %s status is set to Deployed for Helm feature", dependentSummary.Name) + verifyFeatureStatusIsProvisioned(kindWorkloadCluster.GetNamespace(), dependentSummary.Name, libsveltosv1beta1.FeatureHelm) + + Byf("Deleting prerequisite ClusterProfile %s while dependent ClusterProfile %s is still present", + prerequisiteProfile.Name, dependentProfile.Name) + currentPrerequisiteProfile := &configv1beta1.ClusterProfile{} + Expect(k8sClient.Get(context.TODO(), types.NamespacedName{Name: prerequisiteProfile.Name}, + currentPrerequisiteProfile)).To(Succeed()) + Expect(k8sClient.Delete(context.TODO(), currentPrerequisiteProfile)).To(Succeed()) + + isBlockedOnDependent := func() bool { + currentClusterSummary := &configv1beta1.ClusterSummary{} + err := k8sClient.Get(context.TODO(), + types.NamespacedName{Namespace: prerequisiteSummary.Namespace, Name: prerequisiteSummary.Name}, + currentClusterSummary) + if err != nil { + return false + } + return currentClusterSummary.Status.Dependencies != nil && + strings.Contains(*currentClusterSummary.Status.Dependencies, dependentSummary.Name) + } + + Byf("Verifying ClusterSummary %s reports it's waiting on dependent %s", + prerequisiteSummary.Name, dependentSummary.Name) + // The reconciler needs at least one pass after the Delete above to observe the + // dependent and write this status - Eventually here, not Consistently, which would + // fail on that first, still-stale sample. + Eventually(isBlockedOnDependent, timeout, pollingInterval).Should(BeTrue()) + + Byf("Verifying ClusterSummary %s stays present, reporting dependent %s, while %s exists", + prerequisiteSummary.Name, dependentSummary.Name, dependentProfile.Name) + Consistently(isBlockedOnDependent, timeout/2, pollingInterval).Should(BeTrue()) + + Byf("Deleting dependent ClusterProfile %s", dependentProfile.Name) + deleteClusterProfile(dependentProfile) + + Byf("Verifying ClusterSummary %s is now gone, freed by dependent %s's removal", + prerequisiteSummary.Name, dependentProfile.Name) + Eventually(func() bool { + currentClusterSummary := &configv1beta1.ClusterSummary{} + err := k8sClient.Get(context.TODO(), + types.NamespacedName{Namespace: prerequisiteSummary.Namespace, Name: prerequisiteSummary.Name}, + currentClusterSummary) + return apierrors.IsNotFound(err) + }, timeout, pollingInterval).Should(BeTrue()) + + Byf("Verifying ClusterProfile %s is now gone", prerequisiteProfile.Name) + Eventually(func() bool { + err := k8sClient.Get(context.TODO(), types.NamespacedName{Name: prerequisiteProfile.Name}, + currentPrerequisiteProfile) + return apierrors.IsNotFound(err) + }, timeout, pollingInterval).Should(BeTrue()) }) })