webhosting-operator: fix edge cases with missing status updates (#640)

* webhosting-operator: fix edge cases with missing status updates

* webhosting-operator: drop special handling for conflict errors

Rely on the usual exponential backoff and accept the noisy logs.

* e2e: allow skipping cleanup on failure

Author: timebertt

webhosting-operator/pkg/controllers/webhosting/common.go

@@ -480,9 +480,13 @@ func (r *WebsiteReconciler) SetupWithManager(mgr manager.Manager, enableSharding
 	}
 
 	var (
-		// trigger on spec change and annotation changes (manual trigger for testing purposes)
-		websitePredicate = predicate.Or(predicate.GenerationChangedPredicate{}, predicate.AnnotationChangedPredicate{})
-		reconciler       = SilenceConflicts(reconcile.AsReconciler[*webhostingv1alpha1.Website](r.Client, r))
+		// trigger on spec, status, and annotation changes (manual trigger for testing purposes)
+		websitePredicate = predicate.Or(
+			predicate.GenerationChangedPredicate{},
+			predicate.AnnotationChangedPredicate{},
+			WebsiteStatusChanged,
+		)
+		reconciler = reconcile.AsReconciler[*webhostingv1alpha1.Website](r.Client, r)
 	)
 
 	if enableSharding {
@@ -551,6 +555,31 @@ func (r *WebsiteReconciler) MapThemeToWebsites(ctx context.Context, theme client
 	return requests
 }
 
+// WebsiteStatusChanged is a predicate that triggers when the Website status changes.
+// The controller skips updating the status if it didn't change the cached object. In fast consecutive retries, this
+// can lead to a Website in Error state, where the controller doesn't observe its own status update, and skips the
+// transition to Ready afterward because the cached object was still in Ready state.
+// To fix this, we trigger the controller one more time when observing its own status updates to ensure a correct
+// status. This shouldn't hurt the standard case, because the controller doesn't cause any API calls if not needed.
+var WebsiteStatusChanged = predicate.Funcs{
+	UpdateFunc: func(e event.UpdateEvent) bool {
+		if e.ObjectOld == nil || e.ObjectNew == nil {
+			return false
+		}
+
+		oldWebsite, ok := e.ObjectOld.(*webhostingv1alpha1.Website)
+		if !ok {
+			return false
+		}
+		newWebsite, ok := e.ObjectNew.(*webhostingv1alpha1.Website)
+		if !ok {
+			return false
+		}
+
+		return !apiequality.Semantic.DeepEqual(oldWebsite.Status, newWebsite.Status)
+	},
+}
+
 // DeploymentAvailabilityChanged is a predicate for filtering relevant Deployment events.
 var DeploymentAvailabilityChanged = predicate.Funcs{
 	UpdateFunc: func(e event.UpdateEvent) bool {

webhosting-operator/pkg/controllers/webhosting/website_controller.go

@@ -18,6 +18,7 @@ package e2e
 
 import (
 	"context"
+	"flag"
 	"maps"
 	"testing"
 	"time"
@@ -42,6 +43,14 @@ import (
 	webhostingv1alpha1 "github.com/timebertt/kubernetes-controller-sharding/webhosting-operator/pkg/apis/webhosting/v1alpha1"
 )
 
+var skipCleanup bool
+
+func TestMain(m *testing.M) {
+	flag.BoolVar(&skipCleanup, "skip-cleanup", false, "Skip cleanup after test failure")
+	flag.Parse()
+	m.Run()
+}
+
 func TestE2E(t *testing.T) {
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "Webhosting Operator E2E Test Suite")
@@ -112,6 +121,10 @@ var _ = BeforeEach(func(ctx SpecContext) {
 	log.Info("Created test Namespace", "namespace", namespace.Name)
 
 	DeferCleanup(func(ctx SpecContext) {
+		if ctx.SpecReport().Failed() && skipCleanup {
+			Skip("Leaving state of test failure because --skip-cleanup is set to true")
+		}
+
 		By("Delete all Websites in test Namespace")
 		Expect(testClient.DeleteAllOf(ctx, &webhostingv1alpha1.Website{}, client.InNamespace(namespace.Name))).To(Succeed())
 
@@ -124,12 +137,16 @@ var _ = BeforeEach(func(ctx SpecContext) {
 	By("Set up test ControllerRing")
 	controllerRing = &shardingv1alpha1.ControllerRing{ObjectMeta: metav1.ObjectMeta{Name: webhostingv1alpha1.WebhostingOperatorName}}
 
-	By("Scaling checksum-controller")
+	By("Scaling webhosting-operator")
 	controllerDeployment = &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: webhostingv1alpha1.WebhostingOperatorName, Namespace: webhostingv1alpha1.NamespaceSystem}}
 	scaleController(ctx, 3)
 
 	DeferCleanup(func(ctx SpecContext) {
-		By("Scaling checksum-controller")
+		if ctx.SpecReport().Failed() && skipCleanup {
+			Skip("Leaving state of test failure because --skip-cleanup is set to true")
+		}
+
+		By("Scaling webhosting-operator")
 		scaleController(ctx, 3)
 	}, NodeTimeout(ShortTimeout))