diff --git a/internal/controller/autosnapshot_controller.go b/internal/controller/autosnapshot_controller.go index 9d34d9f4..275926ab 100644 --- a/internal/controller/autosnapshot_controller.go +++ b/internal/controller/autosnapshot_controller.go @@ -28,6 +28,7 @@ import ( "github.com/cockroachdb/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -338,25 +339,44 @@ func (r *AutoSnapshotRunnable) createAutoSnapshot( // uncreated snapshot (bumped without snapshot). The Snapshot CRD is // the source of truth so id-only inconsistency is recoverable on // retry. +// The rd argument comes from Tick's List and is stale by the time we +// write: the Snapshot create right before this wakes reconcilers that +// update the RD concurrently, so a bare Update on the listed object +// 409s (observed deterministically in the L-integration stack). The +// stamp re-reads the RD fresh and retries the conflict away — losing +// the stamp is worse than the extra read: the next tick re-derives +// the SAME id and only createAutoSnapshot's AlreadyExists guard keeps +// the loop idempotent. func (r *AutoSnapshotRunnable) stampRDAfterCreate( ctx context.Context, rd *blockstoriov1alpha1.ResourceDefinition, nextID int64, now time.Time, ) error { - if rd.Spec.Props == nil { - rd.Spec.Props = make(map[string]string) - } + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + var fresh blockstoriov1alpha1.ResourceDefinition - rd.Spec.Props[PropAutoSnapshotNextID] = strconv.FormatInt(nextID, 10) + getErr := r.Client.Get(ctx, client.ObjectKeyFromObject(rd), &fresh) + if getErr != nil { + // Bare error: RetryOnConflict matches on the apierrors type. + return getErr + } - if rd.Annotations == nil { - rd.Annotations = make(map[string]string) - } + if fresh.Spec.Props == nil { + fresh.Spec.Props = make(map[string]string) + } - rd.Annotations[AnnotationAutoSnapshotLastAt] = now.UTC().Format(time.RFC3339Nano) + fresh.Spec.Props[PropAutoSnapshotNextID] = strconv.FormatInt(nextID, 10) - err := r.Client.Update(ctx, rd) + if fresh.Annotations == nil { + fresh.Annotations = make(map[string]string) + } + + fresh.Annotations[AnnotationAutoSnapshotLastAt] = now.UTC().Format(time.RFC3339Nano) + + // Bare error: RetryOnConflict matches on the apierrors type. + return r.Client.Update(ctx, &fresh) + }) if err != nil { return errors.Wrap(err, "update RD") } diff --git a/internal/controller/autosnapshot_stamp_conflict_test.go b/internal/controller/autosnapshot_stamp_conflict_test.go new file mode 100644 index 00000000..60de46a9 --- /dev/null +++ b/internal/controller/autosnapshot_stamp_conflict_test.go @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +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" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" +) + +// TestAutoSnapshotStampSurvivesRDUpdateConflict pins the bookkeeping +// half of the auto-snapshot tick against the reconciler race observed +// in the L-integration stack (TestGroupG/AutoSnapshotPeriodicTick): +// the RD object the runnable stamps comes from Tick's List, and the +// Snapshot create wakes reconcilers that update the RD concurrently — +// so the stamp's bare Update hits a 409 Conflict. Pre-fix the conflict +// aborted the stamp (Tick swallows per-RD errors), leaving NextAutoId +// and the last-at annotation unset: the next tick re-derived the SAME +// id every interval and only the createAutoSnapshot AlreadyExists +// short-circuit kept the loop from duplicating snapshots. The stamp +// must instead re-read the RD fresh and retry the conflict away. +func TestAutoSnapshotStampSurvivesRDUpdateConflict(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := blockstoriov1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + + rd := &blockstoriov1alpha1.ResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "rd-stamp-conflict"}, + Spec: blockstoriov1alpha1.ResourceDefinitionSpec{ + Props: map[string]string{PropAutoSnapshotRunEvery: "1"}, + }, + } + + // Model the racing reconciler: the FIRST RD Update lands on a + // stale resourceVersion and 409s; the retry (against a fresh + // read) succeeds. + conflicted := false + cli := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(rd). + WithInterceptorFuncs(interceptor.Funcs{ + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + _, isRD := obj.(*blockstoriov1alpha1.ResourceDefinition) + if isRD && !conflicted { + conflicted = true + + return apierrors.NewConflict( + schema.GroupResource{Group: "blockstor.cozystack.io", Resource: "resourcedefinitions"}, + obj.GetName(), + nil, + ) + } + + return c.Update(ctx, obj, opts...) + }, + }). + Build() + + runnable := &AutoSnapshotRunnable{Client: cli} + + if err := runnable.Tick(context.Background()); err != nil { + t.Fatalf("Tick: %v", err) + } + + if !conflicted { + t.Fatalf("interceptor never fired — the test no longer models the stamp Update") + } + + var updated blockstoriov1alpha1.ResourceDefinition + if err := cli.Get(context.Background(), types.NamespacedName{Name: "rd-stamp-conflict"}, &updated); err != nil { + t.Fatalf("re-fetch RD: %v", err) + } + + if got := updated.Spec.Props[PropAutoSnapshotNextID]; got != "2" { + t.Errorf("NextAutoId after conflicted stamp: got %q, want \"2\"", got) + } + + if updated.Annotations[AnnotationAutoSnapshotLastAt] == "" { + t.Errorf("AnnotationAutoSnapshotLastAt unset after conflicted stamp; the tick would re-fire and re-derive the same id") + } +} diff --git a/tests/integration/harness/asserts.go b/tests/integration/harness/asserts.go index 5ceff28c..cb41fb04 100644 --- a/tests/integration/harness/asserts.go +++ b/tests/integration/harness/asserts.go @@ -22,6 +22,7 @@ package harness import ( "context" + "os" "testing" "time" @@ -35,12 +36,15 @@ import ( // elapses. Fails the test via t.Fatalf on timeout. The default // poll interval is 100ms — short enough that a 30s budget gives // 300 attempts, long enough that the apiserver isn't hammered. +// +// The budget is stretched by scaledTimeout on CI — see its comment. func Eventually(t *testing.T, timeout time.Duration, predicate func() bool, msg string) { t.Helper() const pollInterval = 100 * time.Millisecond - deadline := time.Now().Add(timeout) + effective := scaledTimeout(timeout) + deadline := time.Now().Add(effective) for { if predicate() { @@ -48,13 +52,32 @@ func Eventually(t *testing.T, timeout time.Duration, predicate func() bool, msg } if time.Now().After(deadline) { - t.Fatalf("Eventually timed out after %s: %s", timeout, msg) + t.Fatalf("Eventually timed out after %s: %s", effective, msg) } time.Sleep(pollInterval) } } +// scaledTimeout stretches every Eventually budget on CI. The per-group +// convergence constants (30s) were tuned on dev machines; GitHub-hosted +// runners under a full suite run are several times slower, so a random +// group blows its budget and the Integration lane rotate-flakes +// (GroupI's ResourceConnectionPathCreate one run, GroupJ's +// CSICreateVolumeFromEmpty the next). Eventually carries only +// positive-convergence asserts — it returns the moment the predicate +// passes — so green runs pay nothing for the stretch; only genuinely +// failing runs report slower, still capped by the job-level -timeout. +func scaledTimeout(timeout time.Duration) time.Duration { + const ciScale = 3 + + if os.Getenv("CI") == "" { + return timeout + } + + return timeout * ciScale +} + // MustList returns the .Items slice of the given list-type. The // caller passes a pointer to an empty list (e.g. &blockstoriov1alpha1.NodeList{}) // and a function that extracts the Items. We keep the interface diff --git a/tests/integration/harness/asserts_ci_scale_test.go b/tests/integration/harness/asserts_ci_scale_test.go new file mode 100644 index 00000000..28d77e8c --- /dev/null +++ b/tests/integration/harness/asserts_ci_scale_test.go @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build integration + +/* +Copyright 2026 Cozystack contributors. + +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 harness + +import ( + "testing" + "time" +) + +// TestScaledTimeoutStretchesOnCI pins the CI budget stretch: the +// per-group convergence constants are tuned for dev machines, and the +// Integration lane rotate-flaked on GitHub runners until Eventually +// budgets were scaled there (GroupI / GroupJ 30s timeouts). +func TestScaledTimeoutStretchesOnCI(t *testing.T) { + t.Setenv("CI", "true") + + if got := scaledTimeout(30 * time.Second); got != 90*time.Second { + t.Fatalf("scaledTimeout on CI: got %s, want 90s", got) + } + + t.Setenv("CI", "") + + if got := scaledTimeout(30 * time.Second); got != 30*time.Second { + t.Fatalf("scaledTimeout off CI: got %s, want 30s", got) + } +}