From 5521e1e3a6cb977ecdc63c401682699616d5fc0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20St=C3=A4bler?= Date: Fri, 10 Apr 2026 09:51:22 +0200 Subject: [PATCH 1/3] Add StatusTracker for intermediate status updates Provides immediate status visibility during long-running reconciliation operations by flushing updates at key checkpoints. Updates only when status changes and retries on conflict with exponential backoff. --- internal/controller/function_controller.go | 28 ++++-- internal/controller/status_tracker.go | 104 +++++++++++++++++++++ 2 files changed, 125 insertions(+), 7 deletions(-) create mode 100644 internal/controller/status_tracker.go diff --git a/internal/controller/function_controller.go b/internal/controller/function_controller.go index 88f4310..7ce331a 100644 --- a/internal/controller/function_controller.go +++ b/internal/controller/function_controller.go @@ -91,15 +91,17 @@ func (r *FunctionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } function := original.DeepCopy() + + // Create tracker and add to context + statusTracker := NewStatusTracker(r.Client, function) + ctx = WithStatusTracker(ctx, statusTracker) + reconcileErr := r.reconcile(ctx, function) - function.CalculateReadyCondition() - // update status if required - if !equality.Semantic.DeepEqual(original.Status, function.Status) { - if err := r.Status().Update(ctx, function); err != nil { - logger.Error(err, "Unable to update Function status") - return ctrl.Result{}, err - } + // Final flush at the end (handles ready condition calculation) + if err := statusTracker.Flush(ctx, function); err != nil { + logger.Error(err, "Unable to update Function status") + return ctrl.Result{}, err } if reconcileErr != nil { @@ -122,12 +124,18 @@ func (r *FunctionReconciler) reconcile(ctx context.Context, function *v1alpha1.F defer repo.Cleanup() r.updateFunctionStatusGit(function, repo) + if err := FlushStatus(ctx, function); err != nil { + return fmt.Errorf("failed to update status: %w", err) + } if err := r.ensureDeployment(ctx, function, repo, metadata); err != nil { return fmt.Errorf("deploying function failed: %w", err) } r.updateFunctionStatus(function, metadata) + if err := FlushStatus(ctx, function); err != nil { + return fmt.Errorf("failed to update status: %w", err) + } return nil } @@ -199,6 +207,12 @@ func (r *FunctionReconciler) handleMiddlewareUpdate(ctx context.Context, functio if !isOnLatestMiddleware { logger.Info("Function is not on latest middleware. Will redeploy") function.MarkMiddlewareNotUpToDate("MiddlewareOutdated", "Middleware is outdated, redeploying") + + // Checkpoint 2: Flush status before long deploy operation + if err := FlushStatus(ctx, function); err != nil { + logger.Error(err, "Failed to update status before redeployment") + } + if err := r.deploy(ctx, function, repo); err != nil { function.MarkDeployNotReady("DeployFailed", "Redeployment failed: %s", err.Error()) return fmt.Errorf("failed to redeploy function: %w", err) diff --git a/internal/controller/status_tracker.go b/internal/controller/status_tracker.go new file mode 100644 index 0000000..0742b1c --- /dev/null +++ b/internal/controller/status_tracker.go @@ -0,0 +1,104 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "fmt" + + "github.com/functions-dev/func-operator/api/v1alpha1" + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// StatusTracker manages incremental status updates during reconciliation +type StatusTracker struct { + client client.Client + original *v1alpha1.Function +} + +// NewStatusTracker creates a new status tracker with a snapshot of the current function state +func NewStatusTracker(client client.Client, function *v1alpha1.Function) *StatusTracker { + return &StatusTracker{ + client: client, + original: function.DeepCopy(), + } +} + +// Flush updates the function status if it has changed since the last flush +func (t *StatusTracker) Flush(ctx context.Context, current *v1alpha1.Function) error { + // Always calculate ready condition before comparing + current.CalculateReadyCondition() + + // Compare and update if changed + if !equality.Semantic.DeepEqual(t.original.Status, current.Status) { + // Retry on conflict with exponential backoff + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + // Get the latest version to ensure we have the most recent resourceVersion + latest := &v1alpha1.Function{} + if err := t.client.Get(ctx, types.NamespacedName{ + Name: current.Name, + Namespace: current.Namespace, + }, latest); err != nil { + return err + } + + // Apply our status changes to the latest version + latest.Status = current.Status + + // Attempt the update + return t.client.Status().Update(ctx, latest) + }) + + if err != nil { + return fmt.Errorf("failed to update status: %w", err) + } + + // Update our snapshot to the new state + t.original = current.DeepCopy() + } + + return nil +} + +// statusTrackerKey is the context key for the status tracker +type statusTrackerKey struct{} + +// WithStatusTracker adds a status tracker to the context +func WithStatusTracker(ctx context.Context, tracker *StatusTracker) context.Context { + return context.WithValue(ctx, statusTrackerKey{}, tracker) +} + +// GetStatusTracker retrieves the tracker from context +func GetStatusTracker(ctx context.Context) *StatusTracker { + tracker, ok := ctx.Value(statusTrackerKey{}).(*StatusTracker) + if !ok { + return nil + } + return tracker +} + +// FlushStatus is a convenience helper that gets tracker from context and flushes +func FlushStatus(ctx context.Context, function *v1alpha1.Function) error { + tracker := GetStatusTracker(ctx) + if tracker == nil { + return nil // gracefully handle missing tracker + } + return tracker.Flush(ctx, function) +} From dfb8d30e2d762e1cc506fd4536d0d0aa189c58b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20St=C3=A4bler?= Date: Fri, 10 Apr 2026 09:56:07 +0200 Subject: [PATCH 2/3] Fix CalculateReadyCondition to handle Unknown conditions Function is now marked as NotReady when any condition is Unknown, preventing false positive Ready status. False conditions take precedence over Unknown when determining reason and message. --- api/v1alpha1/function_lifecycle.go | 24 +++- api/v1alpha1/function_lifecycle_test.go | 156 ++++++++++++++++++++++++ 2 files changed, 175 insertions(+), 5 deletions(-) create mode 100644 api/v1alpha1/function_lifecycle_test.go diff --git a/api/v1alpha1/function_lifecycle.go b/api/v1alpha1/function_lifecycle.go index 1e3cf44..3cc27e5 100644 --- a/api/v1alpha1/function_lifecycle.go +++ b/api/v1alpha1/function_lifecycle.go @@ -52,11 +52,25 @@ func (f *Function) CalculateReadyCondition() { reason := "" message := "" for _, condition := range f.Status.Conditions { - if condition.Type != TypeReady && condition.Status == metav1.ConditionFalse { - allReady = false - reason = condition.Reason - message = condition.Message - continue + if condition.Type != TypeReady { + if condition.Status == metav1.ConditionFalse { + allReady = false + reason = condition.Reason + message = condition.Message + continue + } else if condition.Status == metav1.ConditionUnknown { + allReady = false + + // override reason & message only if not set already + // (e.g. if set by a ConditionFalse as this takes preference) + if reason == "" { + reason = condition.Reason + } + if message == "" { + message = condition.Message + } + continue + } } } diff --git a/api/v1alpha1/function_lifecycle_test.go b/api/v1alpha1/function_lifecycle_test.go new file mode 100644 index 0000000..30bdd68 --- /dev/null +++ b/api/v1alpha1/function_lifecycle_test.go @@ -0,0 +1,156 @@ +package v1alpha1 + +import ( + "testing" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestCalculateReadyCondition(t *testing.T) { + tests := []struct { + name string + conditions []metav1.Condition + expectedStatus metav1.ConditionStatus + expectedReason string + expectedMessage string + }{ + { + name: "all conditions true", + conditions: []metav1.Condition{ + {Type: TypeSourceReady, Status: metav1.ConditionTrue, Reason: "CloneSucceeded"}, + {Type: TypeDeployed, Status: metav1.ConditionTrue, Reason: "DeploySucceeded"}, + {Type: TypeMiddlewareUpToDate, Status: metav1.ConditionTrue, Reason: "UpToDate"}, + }, + expectedStatus: metav1.ConditionTrue, + expectedReason: "ReconcileSucceeded", + }, + { + name: "one condition false", + conditions: []metav1.Condition{ + {Type: TypeSourceReady, Status: metav1.ConditionTrue, Reason: "CloneSucceeded"}, + {Type: TypeDeployed, Status: metav1.ConditionFalse, Reason: "DeployFailed", Message: "deployment failed"}, + {Type: TypeMiddlewareUpToDate, Status: metav1.ConditionTrue, Reason: "UpToDate"}, + }, + expectedStatus: metav1.ConditionFalse, + expectedReason: "DeployFailed", + expectedMessage: "deployment failed", + }, + { + name: "one condition unknown", + conditions: []metav1.Condition{ + {Type: TypeSourceReady, Status: metav1.ConditionTrue, Reason: "CloneSucceeded"}, + {Type: TypeDeployed, Status: metav1.ConditionUnknown, Reason: "NotChecked", Message: "deployment not checked yet"}, + {Type: TypeMiddlewareUpToDate, Status: metav1.ConditionTrue, Reason: "UpToDate"}, + }, + expectedStatus: metav1.ConditionFalse, + expectedReason: "NotChecked", + expectedMessage: "deployment not checked yet", + }, + { + name: "multiple conditions unknown", + conditions: []metav1.Condition{ + {Type: TypeSourceReady, Status: metav1.ConditionUnknown, Reason: "NotCloned", Message: "source not cloned yet"}, + {Type: TypeDeployed, Status: metav1.ConditionUnknown, Reason: "NotDeployed", Message: "not deployed yet"}, + {Type: TypeMiddlewareUpToDate, Status: metav1.ConditionTrue, Reason: "UpToDate"}, + }, + expectedStatus: metav1.ConditionFalse, + expectedReason: "NotCloned", + expectedMessage: "source not cloned yet", + }, + { + name: "false takes precedence over unknown", + conditions: []metav1.Condition{ + {Type: TypeSourceReady, Status: metav1.ConditionUnknown, Reason: "NotCloned", Message: "source not cloned yet"}, + {Type: TypeDeployed, Status: metav1.ConditionFalse, Reason: "DeployFailed", Message: "deployment failed"}, + {Type: TypeMiddlewareUpToDate, Status: metav1.ConditionTrue, Reason: "UpToDate"}, + }, + expectedStatus: metav1.ConditionFalse, + expectedReason: "DeployFailed", + expectedMessage: "deployment failed", + }, + { + name: "all conditions unknown", + conditions: []metav1.Condition{ + {Type: TypeSourceReady, Status: metav1.ConditionUnknown, Reason: "unknown", Message: ""}, + {Type: TypeDeployed, Status: metav1.ConditionUnknown, Reason: "unknown", Message: ""}, + {Type: TypeMiddlewareUpToDate, Status: metav1.ConditionUnknown, Reason: "unknown", Message: ""}, + }, + expectedStatus: metav1.ConditionFalse, + expectedReason: "unknown", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := &Function{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, + } + f.Status.Conditions = tt.conditions + + f.CalculateReadyCondition() + + readyCondition := meta.FindStatusCondition(f.Status.Conditions, TypeReady) + if readyCondition == nil { + t.Fatal("Ready condition not found") + } + + if readyCondition.Status != tt.expectedStatus { + t.Errorf("expected status %v, got %v", tt.expectedStatus, readyCondition.Status) + } + + if readyCondition.Reason != tt.expectedReason { + t.Errorf("expected reason %q, got %q", tt.expectedReason, readyCondition.Reason) + } + + if tt.expectedMessage != "" && readyCondition.Message != tt.expectedMessage { + t.Errorf("expected message %q, got %q", tt.expectedMessage, readyCondition.Message) + } + }) + } +} + +func TestInitializeConditions(t *testing.T) { + f := &Function{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, + } + + // Set some existing conditions + f.Status.Conditions = []metav1.Condition{ + {Type: TypeSourceReady, Status: metav1.ConditionTrue, Reason: "CloneSucceeded"}, + {Type: TypeReady, Status: metav1.ConditionTrue, Reason: "ReconcileSucceeded"}, + } + + f.InitializeConditions() + + // Verify all conditions are reset to Unknown + for _, condType := range FunctionsConditions { + cond := meta.FindStatusCondition(f.Status.Conditions, condType) + if cond == nil { + t.Errorf("condition %s not found", condType) + continue + } + if cond.Status != metav1.ConditionUnknown { + t.Errorf("condition %s: expected status Unknown, got %v", condType, cond.Status) + } + if cond.Reason != "unknown" { + t.Errorf("condition %s: expected reason 'unknown', got %q", condType, cond.Reason) + } + if cond.ObservedGeneration != f.Generation { + t.Errorf("condition %s: expected generation %d, got %d", condType, f.Generation, cond.ObservedGeneration) + } + } + + // Verify Ready condition is also set to Unknown + readyCond := meta.FindStatusCondition(f.Status.Conditions, TypeReady) + if readyCond == nil { + t.Fatal("Ready condition not found") + } + if readyCond.Status != metav1.ConditionUnknown { + t.Errorf("Ready condition: expected status Unknown, got %v", readyCond.Status) + } +} From a3c4bfe3d1abca500894a7f6ceef85b5ffc287bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20St=C3=A4bler?= Date: Fri, 10 Apr 2026 10:00:46 +0200 Subject: [PATCH 3/3] Fix linter issue --- internal/controller/status_tracker.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/controller/status_tracker.go b/internal/controller/status_tracker.go index 0742b1c..dd9f956 100644 --- a/internal/controller/status_tracker.go +++ b/internal/controller/status_tracker.go @@ -29,15 +29,15 @@ import ( // StatusTracker manages incremental status updates during reconciliation type StatusTracker struct { - client client.Client - original *v1alpha1.Function + k8sClient client.Client + original *v1alpha1.Function } // NewStatusTracker creates a new status tracker with a snapshot of the current function state -func NewStatusTracker(client client.Client, function *v1alpha1.Function) *StatusTracker { +func NewStatusTracker(k8sClient client.Client, function *v1alpha1.Function) *StatusTracker { return &StatusTracker{ - client: client, - original: function.DeepCopy(), + k8sClient: k8sClient, + original: function.DeepCopy(), } } @@ -52,7 +52,7 @@ func (t *StatusTracker) Flush(ctx context.Context, current *v1alpha1.Function) e err := retry.RetryOnConflict(retry.DefaultRetry, func() error { // Get the latest version to ensure we have the most recent resourceVersion latest := &v1alpha1.Function{} - if err := t.client.Get(ctx, types.NamespacedName{ + if err := t.k8sClient.Get(ctx, types.NamespacedName{ Name: current.Name, Namespace: current.Namespace, }, latest); err != nil { @@ -63,7 +63,7 @@ func (t *StatusTracker) Flush(ctx context.Context, current *v1alpha1.Function) e latest.Status = current.Status // Attempt the update - return t.client.Status().Update(ctx, latest) + return t.k8sClient.Status().Update(ctx, latest) }) if err != nil {