From ffb863df3194b990e2ebdbecad504d7070cfaffc Mon Sep 17 00:00:00 2001 From: Tim Lange Date: Mon, 5 Oct 2026 14:00:25 +0200 Subject: [PATCH 1/5] feat: make cleanup of StoreExecs configurable in CR StoreExec cleanup was driven by a single operator-wide grace period and only applied to successful executions, so every store shared one retention and failed executions were never collected. StoreExecSpec gains cleanupPeriodSuccessfulExec (default 5m) and cleanupPeriodErrorExec (default 1h). reconcileStoreExecCleanup picks the period from the CR's own state; a period of zero disables cleanup for that state, which keeps the previous opt-out per execution. Both fields are pointers: omitempty does not drop a zero metav1.Duration, so a value field would send an explicit "0s" from typed Go clients and bypass the CRD defaults. An unset field falls back to the same default in code, which also covers an older CRD that dropped the field. storeExecFinishedAt no longer guesses. It answers only for a terminal state and only from a condition that carries a LastTransitionTime, instead of falling back to the creation time or time.Now(). CleanupGracePeriod is dropped from StoreExecReconciler; StoreDebugInstance keeps using it. --- api/v1/exec.go | 17 ++ api/v1/zz_generated.deepcopy.go | 11 + cmd/main.go | 9 +- helm/values.yaml | 4 +- internal/controller/storeexec_controller.go | 82 +++++--- .../controller/successful_cleanup_test.go | 192 ++++++++++++++---- 6 files changed, 243 insertions(+), 72 deletions(-) diff --git a/api/v1/exec.go b/api/v1/exec.go index f2670fd4..a1cfc189 100644 --- a/api/v1/exec.go +++ b/api/v1/exec.go @@ -26,6 +26,23 @@ type StoreExecSpec struct { // +kubebuilder:default=3 MaxRetries int32 `json:"maxRetries,omitempty"` + // The two cleanup periods are pointers rather than values because `omitempty` + // does not drop a zero metav1.Duration: a value field would send an explicit + // "0s" from typed clients and defeat the defaults. Keep those defaults in sync + // with defaultCleanupPeriod* in internal/controller/storeexec_controller.go. + + // How long a successfully finished StoreExec is kept before it is deleted. + // Zero disables cleanup for successful executions. + // +optional + // +kubebuilder:default="5m" + CleanupPeriodSuccessfulExec *metav1.Duration `json:"cleanupPeriodSuccessfulExec,omitempty"` + + // How long a failed StoreExec is kept before it is deleted. + // Zero disables cleanup for failed executions. + // +optional + // +kubebuilder:default="1h" + CleanupPeriodErrorExec *metav1.Duration `json:"cleanupPeriodErrorExec,omitempty"` + ExtraEnvs []corev1.EnvVar `json:"extraEnvs,omitempty"` Container ContainerSpec `json:"container,omitempty"` diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 6d129793..dbf2f176 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -23,6 +23,7 @@ package v1 import ( "k8s.io/api/autoscaling/v2" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -977,6 +978,16 @@ func (in *StoreExecList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *StoreExecSpec) DeepCopyInto(out *StoreExecSpec) { *out = *in + if in.CleanupPeriodSuccessfulExec != nil { + in, out := &in.CleanupPeriodSuccessfulExec, &out.CleanupPeriodSuccessfulExec + *out = new(metav1.Duration) + **out = **in + } + if in.CleanupPeriodErrorExec != nil { + in, out := &in.CleanupPeriodErrorExec, &out.CleanupPeriodErrorExec + *out = new(metav1.Duration) + **out = **in + } if in.ExtraEnvs != nil { in, out := &in.ExtraEnvs, &out.ExtraEnvs *out = make([]corev1.EnvVar, len(*in)) diff --git a/cmd/main.go b/cmd/main.go index 8be275ff..a8ef3c4d 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -240,11 +240,10 @@ func main() { os.Exit(1) } if err = (&controller.StoreExecReconciler{ - Client: nsClient, - Logger: logger.With(zapz.String("component", "store-exec-reconciler")), - Scheme: mgr.GetScheme(), - Recorder: mgr.GetEventRecorderFor(fmt.Sprintf("shopware-controller-%s", cfg.Namespace)), - CleanupGracePeriod: cfg.SuccessfulCRCleanupGracePeriod, + Client: nsClient, + Logger: logger.With(zapz.String("component", "store-exec-reconciler")), + Scheme: mgr.GetScheme(), + Recorder: mgr.GetEventRecorderFor(fmt.Sprintf("shopware-controller-%s", cfg.Namespace)), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create exec controller", "controller", "StoreExec") os.Exit(1) diff --git a/helm/values.yaml b/helm/values.yaml index e6eb0db5..b3a03655 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -105,7 +105,9 @@ logFormat: json # Disable check for s3/database/fastly and Opensearch checks. Useful if network access is not given for one of the services. # This is a global level. You can also control this per store. disableChecks: false -# Grace period before successful StoreExec and StoreDebugInstance CRs are deleted. Set to "0" to disable cleanup. +# Grace period before successful StoreDebugInstance CRs are deleted. Set to "0" to disable cleanup. +# StoreExec cleanup is configured per CR via spec.cleanupPeriodSuccessfulExec and +# spec.cleanupPeriodErrorExec, and is not affected by this value. successfulCRCleanupGracePeriod: 1h # keda: when enabled, the operator creates a KEDA ScaledObject for every store diff --git a/internal/controller/storeexec_controller.go b/internal/controller/storeexec_controller.go index 77ec9ff4..80127ab3 100644 --- a/internal/controller/storeexec_controller.go +++ b/internal/controller/storeexec_controller.go @@ -3,6 +3,7 @@ package controller import ( "context" "fmt" + "slices" "time" v1 "github.com/shopware/shopware-operator/api/v1" @@ -11,6 +12,7 @@ import ( "github.com/shopware/shopware-operator/internal/logging" "go.uber.org/zap" k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" @@ -20,10 +22,9 @@ import ( type StoreExecReconciler struct { client.Client - Scheme *runtime.Scheme - Recorder record.EventRecorder - Logger *zap.SugaredLogger - CleanupGracePeriod time.Duration + Scheme *runtime.Scheme + Recorder record.EventRecorder + Logger *zap.SugaredLogger } // +kubebuilder:rbac:groups=shop.shopware.com,namespace=default,resources=storeexecs,verbs=get;list;watch;create;update;patch;delete @@ -63,9 +64,9 @@ func (r *StoreExecReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return rr, nil } - if result, handled, cleanupErr := r.reconcileSuccessfulStoreExecCleanup(ctx, ex); handled || cleanupErr != nil { + if result, handled, cleanupErr := r.reconcileStoreExecCleanup(ctx, ex); handled || cleanupErr != nil { if cleanupErr != nil { - log.Errorw("failed to cleanup successful store-exec", zap.Error(cleanupErr)) + log.Errorw("failed to cleanup finished store-exec", zap.Error(cleanupErr)) skipStatusUpdate = true return rr, nil } @@ -147,47 +148,80 @@ func (r *StoreExecReconciler) reconcileJob(ctx context.Context, store *v1.Store, return nil } -func (r *StoreExecReconciler) reconcileSuccessfulStoreExecCleanup( +func (r *StoreExecReconciler) reconcileStoreExecCleanup( ctx context.Context, ex *v1.StoreExec, ) (ctrl.Result, bool, error) { - if r.CleanupGracePeriod <= 0 || - ex.DeletionTimestamp != nil || - ex.Spec.CronSchedule != "" || - !ex.IsState(v1.ExecStateDone) { + period, ok := cleanupPeriodFor(ex) + if !ok { return ctrl.Result{}, false, nil } - deleteAfter := storeExecFinishedAt(ex).Add(r.CleanupGracePeriod) + finishedAt, ok := storeExecFinishedAt(ex) + if !ok { + return ctrl.Result{}, false, nil + } + + deleteAfter := finishedAt.Add(period) if remaining := time.Until(deleteAfter); remaining > 0 { return ctrl.Result{RequeueAfter: remaining}, true, nil } if err := r.Delete(ctx, ex); err != nil && !k8serrors.IsNotFound(err) { - return ctrl.Result{}, false, fmt.Errorf("delete successful StoreExec: %w", err) + return ctrl.Result{}, false, fmt.Errorf("delete finished StoreExec: %w", err) } return ctrl.Result{}, true, nil } -func storeExecFinishedAt(ex *v1.StoreExec) time.Time { - for i := len(ex.Status.Conditions) - 1; i >= 0; i-- { - if !ex.Status.Conditions[i].LastTransitionTime.IsZero() { - return ex.Status.Conditions[i].LastTransitionTime.Time - } +func storeExecFinishedAt(ex *v1.StoreExec) (time.Time, bool) { + if !ex.IsState(v1.ExecStateDone, v1.ExecStateError) { + return time.Time{}, false } - for i := len(ex.Status.Conditions) - 1; i >= 0; i-- { - if !ex.Status.Conditions[i].LastUpdateTime.IsZero() { - return ex.Status.Conditions[i].LastUpdateTime.Time + for _, v := range slices.Backward(ex.Status.Conditions) { + if t := v.LastTransitionTime; !t.IsZero() { + return t.Time, true } } - if !ex.CreationTimestamp.IsZero() { - return ex.CreationTimestamp.Time + return time.Time{}, false +} + +// Mirror of the kubebuilder defaults on StoreExecSpec. They apply when the field +// is unset in the object we hold, which happens if the CRD in the cluster is +// older than this operator and dropped the field before defaulting could run. +const ( + defaultCleanupPeriodSuccessfulExec = 5 * time.Minute + defaultCleanupPeriodErrorExec = time.Hour +) + +func cleanupPeriodFor(ex *v1.StoreExec) (time.Duration, bool) { + if ex.DeletionTimestamp != nil || ex.Spec.CronSchedule != "" { + return 0, false + } + + var period time.Duration + switch { + case ex.IsState(v1.ExecStateDone): + period = cleanupPeriodOrDefault(ex.Spec.CleanupPeriodSuccessfulExec, defaultCleanupPeriodSuccessfulExec) + case ex.IsState(v1.ExecStateError): + period = cleanupPeriodOrDefault(ex.Spec.CleanupPeriodErrorExec, defaultCleanupPeriodErrorExec) + default: + return 0, false + } + + return period, period > 0 +} + +// An explicit zero stays zero and disables cleanup for that state; only an unset +// field falls back to the default. +func cleanupPeriodOrDefault(period *metav1.Duration, fallback time.Duration) time.Duration { + if period == nil { + return fallback } - return time.Now() + return period.Duration } func (r *StoreExecReconciler) reconcileCronJob(ctx context.Context, store *v1.Store, exec *v1.StoreExec) (err error) { diff --git a/internal/controller/successful_cleanup_test.go b/internal/controller/successful_cleanup_test.go index 010107cb..9a4f90f5 100644 --- a/internal/controller/successful_cleanup_test.go +++ b/internal/controller/successful_cleanup_test.go @@ -17,54 +17,127 @@ import ( const cleanupTestNamespace = "default" -func TestStoreExecSuccessfulCleanupDeletesDoneOneShotAfterGracePeriod(t *testing.T) { +func TestStoreExecCleanupDeletesFinishedExecAfterCleanupPeriod(t *testing.T) { ctx := context.Background() - ex := storeExecForCleanup("done-old", shopv1.ExecStateDone, time.Now().Add(-2*time.Hour)) - cl := fake.NewClientBuilder(). - WithScheme(cleanupTestScheme(t)). - WithObjects(ex). - Build() + scheme := cleanupTestScheme(t) - reconciler := StoreExecReconciler{ - Client: cl, - CleanupGracePeriod: time.Hour, + tests := []struct { + name string + state shopv1.StatefulState + successfulIn time.Duration + errorIn time.Duration + }{ + {name: "done", state: shopv1.ExecStateDone, successfulIn: time.Hour}, + {name: "error", state: shopv1.ExecStateError, errorIn: time.Hour}, } - result, handled, err := reconciler.reconcileSuccessfulStoreExecCleanup(ctx, ex) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ex := storeExecForCleanup(tt.name+"-old", tt.state, time.Now().Add(-2*time.Hour)) + ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: tt.successfulIn} + ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: tt.errorIn} - require.NoError(t, err) - assert.True(t, handled) - assert.Zero(t, result) + cl := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(ex). + Build() - got := &shopv1.StoreExec{} - err = cl.Get(ctx, types.NamespacedName{Namespace: cleanupTestNamespace, Name: ex.Name}, got) - assert.True(t, k8serrors.IsNotFound(err)) + reconciler := StoreExecReconciler{Client: cl} + + result, handled, err := reconciler.reconcileStoreExecCleanup(ctx, ex) + + require.NoError(t, err) + assert.True(t, handled) + assert.Zero(t, result) + + got := &shopv1.StoreExec{} + err = cl.Get(ctx, types.NamespacedName{Namespace: cleanupTestNamespace, Name: ex.Name}, got) + assert.True(t, k8serrors.IsNotFound(err)) + }) + } } -func TestStoreExecSuccessfulCleanupRetainsDoneOneShotBeforeGracePeriod(t *testing.T) { +func TestStoreExecCleanupUsesDefaultPeriodsWhenUnset(t *testing.T) { ctx := context.Background() - ex := storeExecForCleanup("done-new", shopv1.ExecStateDone, time.Now().Add(-30*time.Minute)) - cl := fake.NewClientBuilder(). - WithScheme(cleanupTestScheme(t)). - WithObjects(ex). - Build() + scheme := cleanupTestScheme(t) - reconciler := StoreExecReconciler{ - Client: cl, - CleanupGracePeriod: time.Hour, + tests := []struct { + name string + state shopv1.StatefulState + finishedAt time.Time + wantGone bool + }{ + {name: "done past default", state: shopv1.ExecStateDone, finishedAt: time.Now().Add(-10 * time.Minute), wantGone: true}, + {name: "done within default", state: shopv1.ExecStateDone, finishedAt: time.Now().Add(-time.Minute)}, + {name: "error past default", state: shopv1.ExecStateError, finishedAt: time.Now().Add(-2 * time.Hour), wantGone: true}, + {name: "error within default", state: shopv1.ExecStateError, finishedAt: time.Now().Add(-10 * time.Minute)}, } - result, handled, err := reconciler.reconcileSuccessfulStoreExecCleanup(ctx, ex) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Both periods stay nil: an object whose defaults never ran must still + // be cleaned up rather than silently retained forever. + ex := storeExecForCleanup(tt.name, tt.state, tt.finishedAt) - require.NoError(t, err) - assert.True(t, handled) - assert.Greater(t, result.RequeueAfter, time.Duration(0)) + cl := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(ex). + Build() + + reconciler := StoreExecReconciler{Client: cl} + + _, handled, err := reconciler.reconcileStoreExecCleanup(ctx, ex) + + require.NoError(t, err) + assert.True(t, handled) + + got := &shopv1.StoreExec{} + err = cl.Get(ctx, types.NamespacedName{Namespace: cleanupTestNamespace, Name: ex.Name}, got) + assert.Equal(t, tt.wantGone, k8serrors.IsNotFound(err)) + }) + } +} + +func TestStoreExecCleanupRetainsFinishedExecBeforeCleanupPeriod(t *testing.T) { + ctx := context.Background() + scheme := cleanupTestScheme(t) + + tests := []struct { + name string + state shopv1.StatefulState + successfulIn time.Duration + errorIn time.Duration + }{ + {name: "done", state: shopv1.ExecStateDone, successfulIn: time.Hour}, + {name: "error", state: shopv1.ExecStateError, errorIn: time.Hour}, + } - got := &shopv1.StoreExec{} - require.NoError(t, cl.Get(ctx, types.NamespacedName{Namespace: cleanupTestNamespace, Name: ex.Name}, got)) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ex := storeExecForCleanup(tt.name+"-new", tt.state, time.Now().Add(-30*time.Minute)) + ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: tt.successfulIn} + ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: tt.errorIn} + + cl := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(ex). + Build() + + reconciler := StoreExecReconciler{Client: cl} + + result, handled, err := reconciler.reconcileStoreExecCleanup(ctx, ex) + + require.NoError(t, err) + assert.True(t, handled) + assert.Greater(t, result.RequeueAfter, time.Duration(0)) + + got := &shopv1.StoreExec{} + require.NoError(t, cl.Get(ctx, types.NamespacedName{Namespace: cleanupTestNamespace, Name: ex.Name}, got)) + }) + } } -func TestStoreExecSuccessfulCleanupRetainsCronAndFailedResources(t *testing.T) { +func TestStoreExecCleanupSkipsUnaffectedResources(t *testing.T) { ctx := context.Background() scheme := cleanupTestScheme(t) @@ -77,12 +150,48 @@ func TestStoreExecSuccessfulCleanupRetainsCronAndFailedResources(t *testing.T) { ex: func() *shopv1.StoreExec { ex := storeExecForCleanup("cron-done", shopv1.ExecStateDone, time.Now().Add(-2*time.Hour)) ex.Spec.CronSchedule = "*/5 * * * *" + ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: time.Hour} return ex }(), }, { - name: "error", - ex: storeExecForCleanup("error", shopv1.ExecStateError, time.Now().Add(-2*time.Hour)), + name: "still running", + ex: func() *shopv1.StoreExec { + ex := storeExecForCleanup("running", shopv1.ExecStateRunning, time.Now().Add(-2*time.Hour)) + ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: time.Hour} + ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: time.Hour} + return ex + }(), + }, + { + name: "done with successful period disabled", + ex: func() *shopv1.StoreExec { + ex := storeExecForCleanup("done-no-period", shopv1.ExecStateDone, time.Now().Add(-2*time.Hour)) + ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: 0} + ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: time.Hour} + return ex + }(), + }, + { + name: "error with error period disabled", + ex: func() *shopv1.StoreExec { + ex := storeExecForCleanup("error-no-period", shopv1.ExecStateError, time.Now().Add(-2*time.Hour)) + ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: time.Hour} + ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: 0} + return ex + }(), + }, + { + name: "done without usable finish time", + ex: func() *shopv1.StoreExec { + ex := storeExecForCleanup("done-no-timestamp", shopv1.ExecStateDone, time.Now().Add(-2*time.Hour)) + ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: time.Hour} + for i := range ex.Status.Conditions { + ex.Status.Conditions[i].LastTransitionTime = metav1.Time{} + ex.Status.Conditions[i].LastUpdateTime = metav1.Time{} + } + return ex + }(), }, } @@ -93,12 +202,9 @@ func TestStoreExecSuccessfulCleanupRetainsCronAndFailedResources(t *testing.T) { WithObjects(tt.ex). Build() - reconciler := StoreExecReconciler{ - Client: cl, - CleanupGracePeriod: time.Hour, - } + reconciler := StoreExecReconciler{Client: cl} - result, handled, err := reconciler.reconcileSuccessfulStoreExecCleanup(ctx, tt.ex) + result, handled, err := reconciler.reconcileStoreExecCleanup(ctx, tt.ex) require.NoError(t, err) assert.False(t, handled) @@ -166,11 +272,13 @@ func TestStoreDebugInstanceSuccessfulCleanupUsesDurationAndGracePeriod(t *testin } } -func TestSuccessfulCleanupDisabledWithZeroGracePeriod(t *testing.T) { +func TestSuccessfulCleanupDisabledWithZeroPeriods(t *testing.T) { ctx := context.Background() scheme := cleanupTestScheme(t) ex := storeExecForCleanup("disabled-exec", shopv1.ExecStateDone, time.Now().Add(-2*time.Hour)) + ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: 0} + ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: 0} debugInstance := storeDebugInstanceForCleanup("disabled-debug", time.Now().Add(-3*time.Hour)) cl := fake.NewClientBuilder(). WithScheme(scheme). @@ -178,7 +286,7 @@ func TestSuccessfulCleanupDisabledWithZeroGracePeriod(t *testing.T) { Build() execReconciler := StoreExecReconciler{Client: cl} - execResult, execHandled, err := execReconciler.reconcileSuccessfulStoreExecCleanup(ctx, ex) + execResult, execHandled, err := execReconciler.reconcileStoreExecCleanup(ctx, ex) require.NoError(t, err) assert.False(t, execHandled) assert.Equal(t, time.Duration(0), execResult.RequeueAfter) @@ -220,7 +328,7 @@ func storeExecForCleanup(name string, state shopv1.StatefulState, finishedAt tim State: state, Conditions: []shopv1.ExecCondition{ { - Type: shopv1.ExecStateRunning, + Type: state, LastTransitionTime: metav1.NewTime(finishedAt), LastUpdateTime: metav1.NewTime(finishedAt), Message: "Command finished", From 4d0f90d8ad1f45b50b99dfae94060450508c16c9 Mon Sep 17 00:00:00 2001 From: Tim Lange Date: Wed, 7 Oct 2026 14:56:35 +0200 Subject: [PATCH 2/5] fix: Review comments --- api/v1/exec.go | 11 +-- api/v1/zz_generated.deepcopy.go | 13 +--- internal/controller/storeexec_controller.go | 23 +------ .../controller/successful_cleanup_test.go | 69 ++++--------------- 4 files changed, 20 insertions(+), 96 deletions(-) diff --git a/api/v1/exec.go b/api/v1/exec.go index a1cfc189..8f662751 100644 --- a/api/v1/exec.go +++ b/api/v1/exec.go @@ -26,22 +26,15 @@ type StoreExecSpec struct { // +kubebuilder:default=3 MaxRetries int32 `json:"maxRetries,omitempty"` - // The two cleanup periods are pointers rather than values because `omitempty` - // does not drop a zero metav1.Duration: a value field would send an explicit - // "0s" from typed clients and defeat the defaults. Keep those defaults in sync - // with defaultCleanupPeriod* in internal/controller/storeexec_controller.go. - // How long a successfully finished StoreExec is kept before it is deleted. // Zero disables cleanup for successful executions. - // +optional // +kubebuilder:default="5m" - CleanupPeriodSuccessfulExec *metav1.Duration `json:"cleanupPeriodSuccessfulExec,omitempty"` + CleanupPeriodSuccessfulExec metav1.Duration `json:"cleanupPeriodSuccessfulExec,omitempty"` // How long a failed StoreExec is kept before it is deleted. // Zero disables cleanup for failed executions. - // +optional // +kubebuilder:default="1h" - CleanupPeriodErrorExec *metav1.Duration `json:"cleanupPeriodErrorExec,omitempty"` + CleanupPeriodErrorExec metav1.Duration `json:"cleanupPeriodErrorExec,omitempty"` ExtraEnvs []corev1.EnvVar `json:"extraEnvs,omitempty"` diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index dbf2f176..56814e4d 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -23,7 +23,6 @@ package v1 import ( "k8s.io/api/autoscaling/v2" corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ) @@ -978,16 +977,8 @@ func (in *StoreExecList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *StoreExecSpec) DeepCopyInto(out *StoreExecSpec) { *out = *in - if in.CleanupPeriodSuccessfulExec != nil { - in, out := &in.CleanupPeriodSuccessfulExec, &out.CleanupPeriodSuccessfulExec - *out = new(metav1.Duration) - **out = **in - } - if in.CleanupPeriodErrorExec != nil { - in, out := &in.CleanupPeriodErrorExec, &out.CleanupPeriodErrorExec - *out = new(metav1.Duration) - **out = **in - } + out.CleanupPeriodSuccessfulExec = in.CleanupPeriodSuccessfulExec + out.CleanupPeriodErrorExec = in.CleanupPeriodErrorExec if in.ExtraEnvs != nil { in, out := &in.ExtraEnvs, &out.ExtraEnvs *out = make([]corev1.EnvVar, len(*in)) diff --git a/internal/controller/storeexec_controller.go b/internal/controller/storeexec_controller.go index 80127ab3..82c84540 100644 --- a/internal/controller/storeexec_controller.go +++ b/internal/controller/storeexec_controller.go @@ -12,7 +12,6 @@ import ( "github.com/shopware/shopware-operator/internal/logging" "go.uber.org/zap" k8serrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" @@ -188,14 +187,6 @@ func storeExecFinishedAt(ex *v1.StoreExec) (time.Time, bool) { return time.Time{}, false } -// Mirror of the kubebuilder defaults on StoreExecSpec. They apply when the field -// is unset in the object we hold, which happens if the CRD in the cluster is -// older than this operator and dropped the field before defaulting could run. -const ( - defaultCleanupPeriodSuccessfulExec = 5 * time.Minute - defaultCleanupPeriodErrorExec = time.Hour -) - func cleanupPeriodFor(ex *v1.StoreExec) (time.Duration, bool) { if ex.DeletionTimestamp != nil || ex.Spec.CronSchedule != "" { return 0, false @@ -204,9 +195,9 @@ func cleanupPeriodFor(ex *v1.StoreExec) (time.Duration, bool) { var period time.Duration switch { case ex.IsState(v1.ExecStateDone): - period = cleanupPeriodOrDefault(ex.Spec.CleanupPeriodSuccessfulExec, defaultCleanupPeriodSuccessfulExec) + period = ex.Spec.CleanupPeriodSuccessfulExec.Duration case ex.IsState(v1.ExecStateError): - period = cleanupPeriodOrDefault(ex.Spec.CleanupPeriodErrorExec, defaultCleanupPeriodErrorExec) + period = ex.Spec.CleanupPeriodErrorExec.Duration default: return 0, false } @@ -214,16 +205,6 @@ func cleanupPeriodFor(ex *v1.StoreExec) (time.Duration, bool) { return period, period > 0 } -// An explicit zero stays zero and disables cleanup for that state; only an unset -// field falls back to the default. -func cleanupPeriodOrDefault(period *metav1.Duration, fallback time.Duration) time.Duration { - if period == nil { - return fallback - } - - return period.Duration -} - func (r *StoreExecReconciler) reconcileCronJob(ctx context.Context, store *v1.Store, exec *v1.StoreExec) (err error) { var changed bool obj := job.CommandCronJob(*store, *exec) diff --git a/internal/controller/successful_cleanup_test.go b/internal/controller/successful_cleanup_test.go index 9a4f90f5..a2633505 100644 --- a/internal/controller/successful_cleanup_test.go +++ b/internal/controller/successful_cleanup_test.go @@ -34,8 +34,8 @@ func TestStoreExecCleanupDeletesFinishedExecAfterCleanupPeriod(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ex := storeExecForCleanup(tt.name+"-old", tt.state, time.Now().Add(-2*time.Hour)) - ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: tt.successfulIn} - ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: tt.errorIn} + ex.Spec.CleanupPeriodSuccessfulExec = metav1.Duration{Duration: tt.successfulIn} + ex.Spec.CleanupPeriodErrorExec = metav1.Duration{Duration: tt.errorIn} cl := fake.NewClientBuilder(). WithScheme(scheme). @@ -57,47 +57,6 @@ func TestStoreExecCleanupDeletesFinishedExecAfterCleanupPeriod(t *testing.T) { } } -func TestStoreExecCleanupUsesDefaultPeriodsWhenUnset(t *testing.T) { - ctx := context.Background() - scheme := cleanupTestScheme(t) - - tests := []struct { - name string - state shopv1.StatefulState - finishedAt time.Time - wantGone bool - }{ - {name: "done past default", state: shopv1.ExecStateDone, finishedAt: time.Now().Add(-10 * time.Minute), wantGone: true}, - {name: "done within default", state: shopv1.ExecStateDone, finishedAt: time.Now().Add(-time.Minute)}, - {name: "error past default", state: shopv1.ExecStateError, finishedAt: time.Now().Add(-2 * time.Hour), wantGone: true}, - {name: "error within default", state: shopv1.ExecStateError, finishedAt: time.Now().Add(-10 * time.Minute)}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Both periods stay nil: an object whose defaults never ran must still - // be cleaned up rather than silently retained forever. - ex := storeExecForCleanup(tt.name, tt.state, tt.finishedAt) - - cl := fake.NewClientBuilder(). - WithScheme(scheme). - WithObjects(ex). - Build() - - reconciler := StoreExecReconciler{Client: cl} - - _, handled, err := reconciler.reconcileStoreExecCleanup(ctx, ex) - - require.NoError(t, err) - assert.True(t, handled) - - got := &shopv1.StoreExec{} - err = cl.Get(ctx, types.NamespacedName{Namespace: cleanupTestNamespace, Name: ex.Name}, got) - assert.Equal(t, tt.wantGone, k8serrors.IsNotFound(err)) - }) - } -} - func TestStoreExecCleanupRetainsFinishedExecBeforeCleanupPeriod(t *testing.T) { ctx := context.Background() scheme := cleanupTestScheme(t) @@ -115,8 +74,8 @@ func TestStoreExecCleanupRetainsFinishedExecBeforeCleanupPeriod(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ex := storeExecForCleanup(tt.name+"-new", tt.state, time.Now().Add(-30*time.Minute)) - ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: tt.successfulIn} - ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: tt.errorIn} + ex.Spec.CleanupPeriodSuccessfulExec = metav1.Duration{Duration: tt.successfulIn} + ex.Spec.CleanupPeriodErrorExec = metav1.Duration{Duration: tt.errorIn} cl := fake.NewClientBuilder(). WithScheme(scheme). @@ -150,7 +109,7 @@ func TestStoreExecCleanupSkipsUnaffectedResources(t *testing.T) { ex: func() *shopv1.StoreExec { ex := storeExecForCleanup("cron-done", shopv1.ExecStateDone, time.Now().Add(-2*time.Hour)) ex.Spec.CronSchedule = "*/5 * * * *" - ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: time.Hour} + ex.Spec.CleanupPeriodSuccessfulExec = metav1.Duration{Duration: time.Hour} return ex }(), }, @@ -158,8 +117,8 @@ func TestStoreExecCleanupSkipsUnaffectedResources(t *testing.T) { name: "still running", ex: func() *shopv1.StoreExec { ex := storeExecForCleanup("running", shopv1.ExecStateRunning, time.Now().Add(-2*time.Hour)) - ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: time.Hour} - ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: time.Hour} + ex.Spec.CleanupPeriodSuccessfulExec = metav1.Duration{Duration: time.Hour} + ex.Spec.CleanupPeriodErrorExec = metav1.Duration{Duration: time.Hour} return ex }(), }, @@ -167,8 +126,8 @@ func TestStoreExecCleanupSkipsUnaffectedResources(t *testing.T) { name: "done with successful period disabled", ex: func() *shopv1.StoreExec { ex := storeExecForCleanup("done-no-period", shopv1.ExecStateDone, time.Now().Add(-2*time.Hour)) - ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: 0} - ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: time.Hour} + ex.Spec.CleanupPeriodSuccessfulExec = metav1.Duration{Duration: 0} + ex.Spec.CleanupPeriodErrorExec = metav1.Duration{Duration: time.Hour} return ex }(), }, @@ -176,8 +135,8 @@ func TestStoreExecCleanupSkipsUnaffectedResources(t *testing.T) { name: "error with error period disabled", ex: func() *shopv1.StoreExec { ex := storeExecForCleanup("error-no-period", shopv1.ExecStateError, time.Now().Add(-2*time.Hour)) - ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: time.Hour} - ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: 0} + ex.Spec.CleanupPeriodSuccessfulExec = metav1.Duration{Duration: time.Hour} + ex.Spec.CleanupPeriodErrorExec = metav1.Duration{Duration: 0} return ex }(), }, @@ -185,7 +144,7 @@ func TestStoreExecCleanupSkipsUnaffectedResources(t *testing.T) { name: "done without usable finish time", ex: func() *shopv1.StoreExec { ex := storeExecForCleanup("done-no-timestamp", shopv1.ExecStateDone, time.Now().Add(-2*time.Hour)) - ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: time.Hour} + ex.Spec.CleanupPeriodSuccessfulExec = metav1.Duration{Duration: time.Hour} for i := range ex.Status.Conditions { ex.Status.Conditions[i].LastTransitionTime = metav1.Time{} ex.Status.Conditions[i].LastUpdateTime = metav1.Time{} @@ -277,8 +236,8 @@ func TestSuccessfulCleanupDisabledWithZeroPeriods(t *testing.T) { scheme := cleanupTestScheme(t) ex := storeExecForCleanup("disabled-exec", shopv1.ExecStateDone, time.Now().Add(-2*time.Hour)) - ex.Spec.CleanupPeriodSuccessfulExec = &metav1.Duration{Duration: 0} - ex.Spec.CleanupPeriodErrorExec = &metav1.Duration{Duration: 0} + ex.Spec.CleanupPeriodSuccessfulExec = metav1.Duration{Duration: 0} + ex.Spec.CleanupPeriodErrorExec = metav1.Duration{Duration: 0} debugInstance := storeDebugInstanceForCleanup("disabled-debug", time.Now().Add(-3*time.Hour)) cl := fake.NewClientBuilder(). WithScheme(scheme). From e6fa02e1b337f7c87715e934a5f0b49ac32ee7d0 Mon Sep 17 00:00:00 2001 From: Tim Lange Date: Wed, 7 Oct 2026 15:31:41 +0200 Subject: [PATCH 3/5] feat: remove CleanupGracePeriod and streamline behaviour of StoreDebugInstance with StoreExecs --- api/v1/storedebuginstance_types.go | 4 ++-- api/v1/zz_generated.deepcopy.go | 1 + cmd/main.go | 9 ++++----- helm/templates/deployment.yaml | 2 -- helm/values.yaml | 4 ---- internal/config/config.go | 3 --- .../storedebuginstance_controller.go | 19 +++++-------------- .../controller/storedebuginstance_status.go | 4 ++-- .../controller/successful_cleanup_test.go | 18 ++++++++---------- internal/util/labels.go | 5 +---- 10 files changed, 23 insertions(+), 46 deletions(-) diff --git a/api/v1/storedebuginstance_types.go b/api/v1/storedebuginstance_types.go index f661efe9..a6802b5b 100644 --- a/api/v1/storedebuginstance_types.go +++ b/api/v1/storedebuginstance_types.go @@ -29,9 +29,9 @@ type StoreDebugInstanceSpec struct { // StoreRef is the reference to the store to debug StoreRef string `json:"storeRef,omitempty"` // Duration is the duration of the debug instance after which it will be deleted - // e.g. 1h or 30m + // e.g. 1h or 30m. Zero keeps the instance until it is deleted manually. // +default="1h" - Duration string `json:"duration,omitempty"` + Duration metav1.Duration `json:"duration,omitempty"` // ExtraLabels is the extra labels to add to the debug instance ExtraLabels map[string]string `json:"extraLabels,omitempty"` // ExtraContainerPorts is the extra ports to add to the debug instance diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 56814e4d..f84f313f 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -869,6 +869,7 @@ func (in *StoreDebugInstanceList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *StoreDebugInstanceSpec) DeepCopyInto(out *StoreDebugInstanceSpec) { *out = *in + out.Duration = in.Duration if in.ExtraLabels != nil { in, out := &in.ExtraLabels, &out.ExtraLabels *out = make(map[string]string, len(*in)) diff --git a/cmd/main.go b/cmd/main.go index a8ef3c4d..c037390e 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -273,11 +273,10 @@ func main() { os.Exit(1) } if err = (&controller.StoreDebugInstanceReconciler{ - Client: nsClient, - Logger: logger.With(zapz.String("component", "store-debug-instance-reconciler")), - Scheme: mgr.GetScheme(), - Recorder: mgr.GetEventRecorderFor(fmt.Sprintf("shopware-controller-%s", cfg.Namespace)), - CleanupGracePeriod: cfg.SuccessfulCRCleanupGracePeriod, + Client: nsClient, + Logger: logger.With(zapz.String("component", "store-debug-instance-reconciler")), + Scheme: mgr.GetScheme(), + Recorder: mgr.GetEventRecorderFor(fmt.Sprintf("shopware-controller-%s", cfg.Namespace)), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create instance controller", "controller", "StoreDebugInstance") os.Exit(1) diff --git a/helm/templates/deployment.yaml b/helm/templates/deployment.yaml index aa3aee1a..b35ae9d5 100644 --- a/helm/templates/deployment.yaml +++ b/helm/templates/deployment.yaml @@ -91,8 +91,6 @@ spec: value: "{{ .Values.keda.enabled | default "false" }}" - name: ENABLE_SERVICE_MONITOR value: "{{ and .Values.metrics.enabled .Values.metrics.serviceMonitor.enabled }}" - - name: SUCCESSFUL_CR_CLEANUP_GRACE_PERIOD - value: "{{ .Values.successfulCRCleanupGracePeriod | default "1h" }}" {{- if .Values.metrics.enabled }} - name: METRICS_BIND_ADDRESS value: ":{{ .Values.metrics.port | default 8080 }}" diff --git a/helm/values.yaml b/helm/values.yaml index b3a03655..dc7a892b 100644 --- a/helm/values.yaml +++ b/helm/values.yaml @@ -105,10 +105,6 @@ logFormat: json # Disable check for s3/database/fastly and Opensearch checks. Useful if network access is not given for one of the services. # This is a global level. You can also control this per store. disableChecks: false -# Grace period before successful StoreDebugInstance CRs are deleted. Set to "0" to disable cleanup. -# StoreExec cleanup is configured per CR via spec.cleanupPeriodSuccessfulExec and -# spec.cleanupPeriodErrorExec, and is not affected by this value. -successfulCRCleanupGracePeriod: 1h # keda: when enabled, the operator creates a KEDA ScaledObject for every store # queue worker deployment. The ScaledObject scales the workers based on the diff --git a/internal/config/config.go b/internal/config/config.go index f599bc84..ea87fece 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "strings" - "time" "github.com/sethvargo/go-envconfig" ) @@ -77,8 +76,6 @@ type StoreConfig struct { EnableServiceMonitor bool `env:"ENABLE_SERVICE_MONITOR, default=false"` DisableChecks bool `env:"DISABLE_CHECKS, default=false"` Namespace string `env:"NAMESPACE, default=default"` - - SuccessfulCRCleanupGracePeriod time.Duration `env:"SUCCESSFUL_CR_CLEANUP_GRACE_PERIOD, default=1h"` } type Config struct { diff --git a/internal/controller/storedebuginstance_controller.go b/internal/controller/storedebuginstance_controller.go index 116d829e..e2b08879 100644 --- a/internal/controller/storedebuginstance_controller.go +++ b/internal/controller/storedebuginstance_controller.go @@ -40,10 +40,9 @@ import ( // StoreDebugInstanceReconciler reconciles a StoreDebugInstance object type StoreDebugInstanceReconciler struct { client.Client - Scheme *runtime.Scheme - Recorder record.EventRecorder - Logger *zap.SugaredLogger - CleanupGracePeriod time.Duration + Scheme *runtime.Scheme + Recorder record.EventRecorder + Logger *zap.SugaredLogger } // +kubebuilder:rbac:groups=shop.shopware.com,namespace=default,resources=storedebuginstances,verbs=get;list;watch;create;update;patch;delete @@ -80,12 +79,6 @@ func (r *StoreDebugInstanceReconciler) Reconcile(ctx context.Context, req ctrl.R log.Errorw("get CR store debug instance", zap.Error(err)) } - // validate duration - _, err = time.ParseDuration(storeDebugInstance.Spec.Duration) - if err != nil { - return rr, fmt.Errorf("invalid duration: %w", err) - } - if result, deleted, cleanupErr := r.deleteSuccessfulStoreDebugInstanceIfCleanupDue(ctx, storeDebugInstance); deleted || cleanupErr != nil { if cleanupErr != nil { log.Errorw("failed to cleanup successful store debug instance", zap.Error(cleanupErr)) @@ -238,7 +231,7 @@ func (r *StoreDebugInstanceReconciler) reconcileSuccessfulStoreDebugInstanceClea func (r *StoreDebugInstanceReconciler) isStoreDebugInstanceCleanupEligible( storeDebugInstance *shopv1.StoreDebugInstance, ) bool { - return r.CleanupGracePeriod > 0 && + return storeDebugInstance.Spec.Duration.Duration > 0 && storeDebugInstance.DeletionTimestamp == nil && storeDebugInstance.IsState(shopv1.StoreDebugInstanceStateDone) } @@ -246,9 +239,7 @@ func (r *StoreDebugInstanceReconciler) isStoreDebugInstanceCleanupEligible( func (r *StoreDebugInstanceReconciler) storeDebugInstanceCleanupRemaining( storeDebugInstance *shopv1.StoreDebugInstance, ) time.Duration { - duration, _ := time.ParseDuration(storeDebugInstance.Spec.Duration) - deleteAfter := storeDebugInstance.CreationTimestamp.Add(duration).Add(r.CleanupGracePeriod) - return time.Until(deleteAfter) + return time.Until(storeDebugInstance.CreationTimestamp.Add(storeDebugInstance.Spec.Duration.Duration)) } func (r *StoreDebugInstanceReconciler) deleteSuccessfulStoreDebugInstance( diff --git a/internal/controller/storedebuginstance_status.go b/internal/controller/storedebuginstance_status.go index 716aae74..c9d5328a 100644 --- a/internal/controller/storedebuginstance_status.go +++ b/internal/controller/storedebuginstance_status.go @@ -99,8 +99,8 @@ func (r *StoreDebugInstanceReconciler) stateRunning(ctx context.Context, store * storeDebugInstance.Status.AddCondition(con) }() - duration, _ := time.ParseDuration(storeDebugInstance.Spec.Duration) - if time.Now().After(storeDebugInstance.CreationTimestamp.Add(duration)) { + duration := storeDebugInstance.Spec.Duration.Duration + if duration > 0 && time.Now().After(storeDebugInstance.CreationTimestamp.Add(duration)) { con.Message = "Store debug instance expired" con.Status = string(v1.StoreDebugInstanceStateDone) return v1.StoreDebugInstanceStateDone diff --git a/internal/controller/successful_cleanup_test.go b/internal/controller/successful_cleanup_test.go index a2633505..f5a02a8e 100644 --- a/internal/controller/successful_cleanup_test.go +++ b/internal/controller/successful_cleanup_test.go @@ -175,7 +175,7 @@ func TestStoreExecCleanupSkipsUnaffectedResources(t *testing.T) { } } -func TestStoreDebugInstanceSuccessfulCleanupUsesDurationAndGracePeriod(t *testing.T) { +func TestStoreDebugInstanceSuccessfulCleanupUsesDuration(t *testing.T) { ctx := context.Background() scheme := cleanupTestScheme(t) @@ -186,15 +186,15 @@ func TestStoreDebugInstanceSuccessfulCleanupUsesDurationAndGracePeriod(t *testin expectDeleted bool }{ { - name: "after duration and grace", + name: "after duration", objectName: "debug-done-old", - creationTime: time.Now().Add(-3 * time.Hour), + creationTime: time.Now().Add(-2 * time.Hour), expectDeleted: true, }, { - name: "before duration and grace", + name: "before duration", objectName: "debug-done-new", - creationTime: time.Now().Add(-90 * time.Minute), + creationTime: time.Now().Add(-30 * time.Minute), expectDeleted: false, }, } @@ -207,10 +207,7 @@ func TestStoreDebugInstanceSuccessfulCleanupUsesDurationAndGracePeriod(t *testin WithObjects(debugInstance). Build() - reconciler := StoreDebugInstanceReconciler{ - Client: cl, - CleanupGracePeriod: time.Hour, - } + reconciler := StoreDebugInstanceReconciler{Client: cl} result, handled, err := reconciler.reconcileSuccessfulStoreDebugInstanceCleanup(ctx, debugInstance) @@ -239,6 +236,7 @@ func TestSuccessfulCleanupDisabledWithZeroPeriods(t *testing.T) { ex.Spec.CleanupPeriodSuccessfulExec = metav1.Duration{Duration: 0} ex.Spec.CleanupPeriodErrorExec = metav1.Duration{Duration: 0} debugInstance := storeDebugInstanceForCleanup("disabled-debug", time.Now().Add(-3*time.Hour)) + debugInstance.Spec.Duration = metav1.Duration{Duration: 0} cl := fake.NewClientBuilder(). WithScheme(scheme). WithObjects(ex, debugInstance). @@ -307,7 +305,7 @@ func storeDebugInstanceForCleanup(name string, creationTime time.Time) *shopv1.S }, Spec: shopv1.StoreDebugInstanceSpec{ StoreRef: "test", - Duration: "1h", + Duration: metav1.Duration{Duration: time.Hour}, }, Status: shopv1.StoreDebugInstanceStatus{ State: shopv1.StoreDebugInstanceStateDone, diff --git a/internal/util/labels.go b/internal/util/labels.go index aea62bca..a097717e 100644 --- a/internal/util/labels.go +++ b/internal/util/labels.go @@ -3,7 +3,6 @@ package util import ( "fmt" "maps" - "time" v1 "github.com/shopware/shopware-operator/api/v1" ) @@ -52,9 +51,7 @@ func GetDefaultStoreSnapshotLabels(store v1.Store, overwrite map[string]string, func GetDefaultStoreInstanceDebugLabels(store v1.Store, storeDebugInstance v1.StoreDebugInstance) map[string]string { labels := GetDefaultContainerStoreLabels(store, storeDebugInstance.Spec.ExtraLabels) - // we don't need to check for errors here, because the duration is validated in the controller - duration, _ := time.ParseDuration(storeDebugInstance.Spec.Duration) - validUntil := storeDebugInstance.CreationTimestamp.Add(duration) + validUntil := storeDebugInstance.CreationTimestamp.Add(storeDebugInstance.Spec.Duration.Duration) labels[ShopwareKey("store.debug")] = "true" labels[ShopwareKey("store.debug.instance")] = storeDebugInstance.Name From a203ad00dd12a21376b99018603e722339d5abc0 Mon Sep 17 00:00:00 2001 From: Tim Lange Date: Wed, 7 Oct 2026 16:21:10 +0200 Subject: [PATCH 4/5] fix: add duration validation to prevent nonsense --- api/v1/exec.go | 2 ++ api/v1/storedebuginstance_types.go | 3 +++ 2 files changed, 5 insertions(+) diff --git a/api/v1/exec.go b/api/v1/exec.go index 8f662751..1d3d6a4f 100644 --- a/api/v1/exec.go +++ b/api/v1/exec.go @@ -29,11 +29,13 @@ type StoreExecSpec struct { // How long a successfully finished StoreExec is kept before it is deleted. // Zero disables cleanup for successful executions. // +kubebuilder:default="5m" + // +kubebuilder:validation:XValidation:rule="self.matches('^(0|([0-9]+([.][0-9]+)?(ns|us|ms|s|m|h))+)$')",message="must be a valid duration, e.g. 30s, 5m or 1h" CleanupPeriodSuccessfulExec metav1.Duration `json:"cleanupPeriodSuccessfulExec,omitempty"` // How long a failed StoreExec is kept before it is deleted. // Zero disables cleanup for failed executions. // +kubebuilder:default="1h" + // +kubebuilder:validation:XValidation:rule="self.matches('^(0|([0-9]+([.][0-9]+)?(ns|us|ms|s|m|h))+)$')",message="must be a valid duration, e.g. 30s, 5m or 1h" CleanupPeriodErrorExec metav1.Duration `json:"cleanupPeriodErrorExec,omitempty"` ExtraEnvs []corev1.EnvVar `json:"extraEnvs,omitempty"` diff --git a/api/v1/storedebuginstance_types.go b/api/v1/storedebuginstance_types.go index a6802b5b..9a1fa2a9 100644 --- a/api/v1/storedebuginstance_types.go +++ b/api/v1/storedebuginstance_types.go @@ -30,7 +30,10 @@ type StoreDebugInstanceSpec struct { StoreRef string `json:"storeRef,omitempty"` // Duration is the duration of the debug instance after which it will be deleted // e.g. 1h or 30m. Zero keeps the instance until it is deleted manually. + // The pattern is enforced by the API server: a value the Go duration parser + // rejects would otherwise break the informer for every StoreDebugInstance. // +default="1h" + // +kubebuilder:validation:XValidation:rule="self.matches('^(0|([0-9]+([.][0-9]+)?(ns|us|ms|s|m|h))+)$')",message="must be a valid duration, e.g. 30s, 5m or 1h" Duration metav1.Duration `json:"duration,omitempty"` // ExtraLabels is the extra labels to add to the debug instance ExtraLabels map[string]string `json:"extraLabels,omitempty"` From a314be66cf76c2d9a73f300328a4578497ba64dc Mon Sep 17 00:00:00 2001 From: Tim Lange Date: Thu, 8 Oct 2026 11:09:03 +0200 Subject: [PATCH 5/5] fix: add debug instances in error state to cleanup cycle --- internal/controller/storedebuginstance_controller.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/controller/storedebuginstance_controller.go b/internal/controller/storedebuginstance_controller.go index e2b08879..fda39591 100644 --- a/internal/controller/storedebuginstance_controller.go +++ b/internal/controller/storedebuginstance_controller.go @@ -233,7 +233,8 @@ func (r *StoreDebugInstanceReconciler) isStoreDebugInstanceCleanupEligible( ) bool { return storeDebugInstance.Spec.Duration.Duration > 0 && storeDebugInstance.DeletionTimestamp == nil && - storeDebugInstance.IsState(shopv1.StoreDebugInstanceStateDone) + (storeDebugInstance.IsState(shopv1.StoreDebugInstanceStateDone) || + storeDebugInstance.IsState(shopv1.StoreDebugInstanceStateError)) } func (r *StoreDebugInstanceReconciler) storeDebugInstanceCleanupRemaining(