Revert "Fix stale bundle error when cluster is offline after bad commit (#4780) (#4823)"

This reverts commit 9b1d80b.

Author: thardeck

e2e/assets/single-cluster/offline-bundle-stuck/Chart.yaml

@@ -9,7 +9,6 @@ import (
 	"path"
 	"strings"
 
-	gogit "github.com/go-git/go-git/v5"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
@@ -263,190 +262,6 @@ var _ = Describe("Checks that template errors are shown in bundles and gitrepos"
 	})
 })
 
-// Checks that once a cluster goes offline after a failed deployment, the bundle
-// status does not permanently show the stale error after a fix commit is applied
-// (issue https://github.com/rancher/fleet/issues/594).
-var _ = Describe("Bundle status does not retain stale error for offline cluster after fix", Ordered, Label("infra-setup"), func() {
-	const (
-		localAgentNS     = "cattle-fleet-local-system"
-		fleetAgentDeploy = "fleet-agent"
-	)
-
-	var (
-		tmpDir           string
-		cloneDir         string
-		k                kubectl.Command
-		kAgent           kubectl.Command
-		gh               *githelper.Git
-		inClusterRepoURL string
-		gitrepoName      string
-		clone            *gogit.Repository
-		targetNamespace  string
-		r                = rand.New(rand.NewSource(GinkgoRandomSeed()))
-	)
-
-	BeforeEach(func() {
-		k = env.Kubectl.Namespace(env.Namespace)
-		kAgent = env.Kubectl.Namespace(localAgentNS)
-	})
-
-	JustBeforeEach(func() {
-		host := githelper.BuildGitHostname()
-		addr, err := githelper.GetExternalRepoAddr(env, port, "repo")
-		Expect(err).ToNot(HaveOccurred())
-		gh = githelper.NewHTTP(addr)
-
-		inClusterRepoURL = gh.GetInClusterURL(host, port, "repo")
-
-		tmpDir, _ = os.MkdirTemp("", "fleet-")
-		cloneDir = path.Join(tmpDir, "repo")
-
-		gitrepoName = testenv.RandomFilename("offline-stuck", r)
-		targetNamespace = testenv.NewNamespaceName("offline-stuck", r)
-
-		clone, err = gh.Create(cloneDir, testenv.AssetPath("single-cluster/offline-bundle-stuck"), "examples")
-		Expect(err).ToNot(HaveOccurred())
-
-		err = testenv.ApplyTemplate(k, testenv.AssetPath("status/gitrepo.yaml"), struct {
-			Name            string
-			Repo            string
-			Branch          string
-			TargetNamespace string
-		}{
-			gitrepoName,
-			inClusterRepoURL,
-			gh.Branch,
-			targetNamespace,
-		})
-		Expect(err).ToNot(HaveOccurred())
-	})
-
-	AfterAll(func() {
-		// Ensure fleet-agent is always restored, even when the test fails midway.
-		_, _ = kAgent.Run("scale", "deployment", fleetAgentDeploy, "--replicas=1", "--timeout=60s")
-
-		_ = os.RemoveAll(tmpDir)
-		_, _ = k.Delete("gitrepo", gitrepoName)
-
-		Eventually(func(g Gomega) {
-			out, _ := k.Get(
-				"bundledeployments",
-				"-A",
-				"-l",
-				fmt.Sprintf("fleet.cattle.io/repo-name=%s", gitrepoName),
-			)
-			g.Expect(out).To(ContainSubstring("No resources found"))
-		}).Should(Succeed())
-
-		_, _ = k.Delete("ns", targetNamespace, "--wait=false")
-	})
-
-	It("clears the stale error from an offline cluster once a fix commit is present", func() {
-		bundleName := gitrepoName + "-examples"
-
-		By("waiting for the initial deployment to be Ready")
-		Eventually(func(g Gomega) {
-			status := getBundleStatus(g, k, bundleName)
-			g.Expect(status.Summary.Ready).To(Equal(1))
-		}).Should(Succeed())
-
-		By("pushing a commit that introduces a YAML parse error")
-		badContent := `apiVersion: v1
-kind: ConfigMap
-metadata:
-  name: offline-bundle-stuck-cm
-data:
-  broken: {unclosed
-`
-		err := os.WriteFile(path.Join(cloneDir, "examples", "templates", "configmap.yaml"), []byte(badContent), 0644)
-		Expect(err).ToNot(HaveOccurred())
-		_, err = gh.Update(clone)
-		Expect(err).ToNot(HaveOccurred())
-
-		By("waiting for the bundle to reflect the YAML parse error")
-		Eventually(func(g Gomega) {
-			status := getBundleStatus(g, k, bundleName)
-			g.Expect(status.Summary.Ready).To(Equal(0))
-			found := false
-			for _, cond := range status.Conditions {
-				if cond.Type == string(fleet.Ready) && strings.Contains(cond.Message, "did not find expected") {
-					found = true
-					break
-				}
-			}
-			g.Expect(found).To(BeTrue(), "expected YAML parse error in bundle conditions, got: %v", status.Conditions)
-		}, testenv.MediumTimeout, testenv.PollingInterval).Should(Succeed())
-
-		By("scaling down the fleet-agent to simulate an offline cluster")
-		out, err := kAgent.Run("scale", "deployment", fleetAgentDeploy, "--replicas=0", "--timeout=60s")
-		Expect(err).ToNot(HaveOccurred(), out)
-
-		// Wait until the agent pod is gone so it cannot apply any further commits.
-		Eventually(func(g Gomega) {
-			out, _ := kAgent.Get("pods", "-l", "app=fleet-agent")
-			g.Expect(out).To(ContainSubstring("No resources found"))
-		}).Should(Succeed())
-
-		By("pushing an intermediate commit that does not fix the YAML error")
-		intermediateContent := `apiVersion: v1
-kind: ConfigMap
-metadata:
-  name: offline-bundle-stuck-cm
-  labels:
-    version: "2"
-data:
-  broken: {unclosed
-`
-		err = os.WriteFile(path.Join(cloneDir, "examples", "templates", "configmap.yaml"), []byte(intermediateContent), 0644)
-		Expect(err).ToNot(HaveOccurred())
-		_, err = gh.Update(clone)
-		Expect(err).ToNot(HaveOccurred())
-
-		By("pushing a fix commit (valid YAML)")
-		fixContent := `apiVersion: v1
-kind: ConfigMap
-metadata:
-  name: offline-bundle-stuck-cm
-data:
-  key: fixed
-`
-		err = os.WriteFile(path.Join(cloneDir, "examples", "templates", "configmap.yaml"), []byte(fixContent), 0644)
-		Expect(err).ToNot(HaveOccurred())
-		_, err = gh.Update(clone)
-		Expect(err).ToNot(HaveOccurred())
-
-		By("verifying the error message does not persist after the fix commit is pushed")
-		// After the controller picks up the fix commit it updates the BD spec.
-		// Even though the offline agent cannot apply the fix yet, the bundle
-		// should no longer surface the stale error from the previous apply attempt.
-		Eventually(func(g Gomega) {
-			status := getBundleStatus(g, k, bundleName)
-			found := false
-			for _, cond := range status.Conditions {
-				if cond.Type == string(fleet.Ready) {
-					found = true
-					g.Expect(cond.Message).NotTo(
-						ContainSubstring("did not find expected"),
-						"bundle Ready condition still shows stale YAML error after fix commit was pushed",
-					)
-					break
-				}
-			}
-			g.Expect(found).To(BeTrue(), "expected Ready condition to be present, got: %v", status.Conditions)
-		}, testenv.LongTimeout, testenv.PollingInterval).Should(Succeed())
-
-		By("scaling the fleet-agent back up")
-		out, err = kAgent.Run("scale", "deployment", fleetAgentDeploy, "--replicas=1", "--timeout=60s")
-		Expect(err).ToNot(HaveOccurred(), out)
-
-		By("waiting for the bundle to become Ready after the agent recovers")
-		Eventually(func(g Gomega) {
-			status := getBundleStatus(g, k, bundleName)
-			g.Expect(status.Summary.Ready).To(Equal(1))
-		}, testenv.LongTimeout, testenv.PollingInterval).Should(Succeed())
-	})
-})
-
 // getBundleStatus retrieves the status of the bundle with the provided name.
 func getBundleStatus(g Gomega, k kubectl.Command, name string) fleet.BundleStatus {
 	gr, err := k.Get("bundle", name, "-o=json")

e2e/assets/single-cluster/offline-bundle-stuck/fleet.yaml

@@ -186,25 +186,12 @@ var _ = Describe("Bundle Status Fields", func() {
 				return k8sClient.Update(ctx, cluster)
 			}).ShouldNot(HaveOccurred())
 
-			By("validating that the bundle deployment spec is updated")
-			Eventually(func() bool {
-				err = k8sClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: "name"}, bd)
-				Expect(err).NotTo(HaveOccurred())
-				return bd.Spec.DeploymentID != deplIDBefore
-			}).Should(BeTrue())
-
-			By("simulating the agent applying the updated bundle deployment")
-			Eventually(func() error {
+			// Change in cluster state results in a bundle deployment update
+			Eventually(func(g Gomega) {
 				err = k8sClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: "name"}, bd)
-				if err != nil {
-					return err
-				}
-				bd.Status.AppliedDeploymentID = bd.Spec.DeploymentID
-				return k8sClient.Status().Update(ctx, bd)
-			}).ShouldNot(HaveOccurred())
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(bd.Spec.DeploymentID).ToNot(Equal(deplIDBefore))
 
-			By("verifying the bundle is ready again")
-			Eventually(func(g Gomega) {
 				err = k8sClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: "name"}, bundle)
 				g.Expect(err).NotTo(HaveOccurred())
 				g.Expect(bundle.Status.Summary.WaitApplied).To(Equal(0))

e2e/assets/single-cluster/offline-bundle-stuck/templates/configmap.yaml

@@ -285,8 +285,7 @@ func deployErrToStatus(err error, status fleet.BundleDeploymentStatus) (bool, fl
 			"(chart requires kubeVersion)|" + // kubeVersion mismatch
 			"(annotation validation error)|" + // annotations fail to pass validation
 			"(failed, and has been rolled back due to atomic being set)|" + // atomic is set and a rollback occurs
-			"(YAML parse error)|" + // YAML is broken in source files (Helm v3)
-			"(MalformedYAMLError)|" + // YAML is broken in source files (Helm v4)
+			"(YAML parse error)|" + // YAML is broken in source files
 			"(Forbidden: updates to [0-9A-Za-z]+ spec for fields other than [0-9A-Za-z ']+ are forbidden)|" + // trying to update fields that cannot be updated
 			"(Forbidden: spec is immutable after creation)|" + // trying to modify immutable spec
 			"(chart requires kubeVersion: [0-9A-Za-z\\.\\-<>=]+ which is incompatible with Kubernetes)", // trying to deploy to incompatible Kubernetes

e2e/single-cluster/status_test.go

@@ -137,13 +137,7 @@ func MessageFromDeployment(deployment *fleet.BundleDeployment) string {
 	}
 	message := MessageFromCondition("Deployed", deployment.Status.Conditions)
 	if message == "" {
-		// Only surface the Installed error when it belongs to the current
-		// deployment. If the deployment IDs differ, the Installed condition
-		// was set by a previous (now superseded) apply attempt and the
-		// message is stale.
-		if deployment.Status.AppliedDeploymentID == deployment.Spec.DeploymentID {
-			message = MessageFromCondition("Installed", deployment.Status.Conditions)
-		}
+		message = MessageFromCondition("Installed", deployment.Status.Conditions)
 	}
 	if message == "" {
 		message = MessageFromCondition("Monitored", deployment.Status.Conditions)

integrationtests/controller/bundle/status_test.go

@@ -1,7 +1,6 @@
 package summary_test
 
 import (
-	"errors"
 	"fmt"
 	"testing"
 
@@ -114,56 +113,3 @@ func TestSetReadyConditions_ReasonClearedWhenBecomingReady(t *testing.T) {
 			c.GetReason(bundleStatus))
 	}
 }
-
-func setCondition(bd *fleet.BundleDeployment, condType, message string) {
-	condition.Cond(condType).SetError(bd, "", errors.New(message))
-}
-
-func TestMessageFromDeployment(t *testing.T) {
-	t.Run("nil deployment returns empty string", func(t *testing.T) {
-		if msg := summary.MessageFromDeployment(nil); msg != "" {
-			t.Errorf("expected empty string, got %q", msg)
-		}
-	})
-
-	t.Run("Deployed condition takes priority over Installed", func(t *testing.T) {
-		bd := &fleet.BundleDeployment{}
-		bd.Spec.DeploymentID = "id1"
-		bd.Status.AppliedDeploymentID = "id1"
-		setCondition(bd, "Deployed", "deploy error")
-		setCondition(bd, "Installed", "install error")
-		if msg := summary.MessageFromDeployment(bd); msg != "deploy error" {
-			t.Errorf("expected deploy error to take priority, got %q", msg)
-		}
-	})
-
-	t.Run("Installed shown when deployment IDs match", func(t *testing.T) {
-		bd := &fleet.BundleDeployment{}
-		bd.Spec.DeploymentID = "id1"
-		bd.Status.AppliedDeploymentID = "id1"
-		setCondition(bd, "Installed", "install error")
-		if msg := summary.MessageFromDeployment(bd); msg != "install error" {
-			t.Errorf("expected install error, got %q", msg)
-		}
-	})
-
-	t.Run("Installed suppressed when deployment IDs differ", func(t *testing.T) {
-		bd := &fleet.BundleDeployment{}
-		bd.Spec.DeploymentID = "id2"
-		bd.Status.AppliedDeploymentID = "id1"
-		setCondition(bd, "Installed", "stale install error")
-		if msg := summary.MessageFromDeployment(bd); msg != "" {
-			t.Errorf("expected stale Installed message to be suppressed, got %q", msg)
-		}
-	})
-
-	t.Run("Monitored used as fallback when Deployed and Installed are absent", func(t *testing.T) {
-		bd := &fleet.BundleDeployment{}
-		bd.Spec.DeploymentID = "id1"
-		bd.Status.AppliedDeploymentID = "id1"
-		setCondition(bd, "Monitored", "monitor error")
-		if msg := summary.MessageFromDeployment(bd); msg != "monitor error" {
-			t.Errorf("expected monitor error as fallback, got %q", msg)
-		}
-	})
-}

internal/cmd/agent/deployer/deployer.go

@@ -136,26 +136,13 @@ func (t *Target) state() fleet.BundleState {
 	case nil:
 		return fleet.Pending
 	default:
-		return summary.GetDeploymentState(t.effectiveDeployment())
+		return summary.GetDeploymentState(t.Deployment)
 	}
 }
 
 // message returns a relevant message from the target (pure function)
 func (t *Target) message() string {
-	return summary.MessageFromDeployment(t.effectiveDeployment())
-}
-
-// effectiveDeployment returns t.Deployment with Spec.DeploymentID overridden
-// to t.DeploymentID, or t.Deployment unchanged when the IDs already match.
-// Status is computed (via state and message) before the reconciler writes the
-// new BD spec, so the cached Spec.DeploymentID lags at that point.
-func (t *Target) effectiveDeployment() *fleet.BundleDeployment {
-	if t.Deployment == nil || t.DeploymentID == t.Deployment.Spec.DeploymentID {
-		return t.Deployment
-	}
-	bd := *t.Deployment
-	bd.Spec.DeploymentID = t.DeploymentID
-	return &bd
+	return summary.MessageFromDeployment(t.Deployment)
 }
 
 // initialiseOptionsMaps initialises options and staged options maps of namespace labels and annotations on bd, if