From 29e71d5ee82ee54badffbfae1bc8cf9de3e4f16e Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Thu, 2 Jul 2026 00:10:08 +0200 Subject: [PATCH] fix(apiserver): resolve VD GET cache-miss via live RD re-read (aligns with RD GET) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Volume-definitions are embedded in the RD CRD spec, so after the v0.1.15 RD-404 fix the SAME committed RD answered differently by path on a multi-replica apiserver: `GET /v1/resource-definitions/{rd}` resolved through the RD store's uncached fallback while `GET .../volume-definitions/{vn}` read only the lagging informer cache and 404'd — observed live on the cozystack e2e stand (`GET .../volume-definitions/0 -> 404`, REST cache-retry budget exhausted). volumeDefinitions.Get now does one live re-read via the existing fetchRDLive on a cache miss OR a stale cached RD revision, mirroring resourceDefinitions.Get. Deliberately NOT extended to the other stores: raw store Gets are cache-only by design — REST existence probes rely on a fast cached NotFound, and get*WithCacheRetry absorbs cross-replica lag at the REST layer while preserving read-your-writes for subsequent cached Lists. A store-level fallback short-circuits that convergence wait: trying it across all stores made the CI snapshot-pagination test regress (the create flow's read-back resolved uncached while the cached List still under-reported). NewWithAPIReader's doc now pins this boundary. Regression tests (L1, fail-on-bug proven — both FAIL on the pre-fix tree): the stale-RD embedded-VD mode, the RD-cache-miss mode, and nil-reader NotFound preservation. Co-Authored-By: Claude Signed-off-by: Andrei Kvapil --- pkg/store/k8s/k8s.go | 19 ++- ...store_cache_miss_fallback_internal_test.go | 149 ++++++++++++++++++ pkg/store/k8s/volume_definitions.go | 34 +++- 3 files changed, 195 insertions(+), 7 deletions(-) create mode 100644 pkg/store/k8s/store_cache_miss_fallback_internal_test.go diff --git a/pkg/store/k8s/k8s.go b/pkg/store/k8s/k8s.go index 46083c57..f5c85693 100644 --- a/pkg/store/k8s/k8s.go +++ b/pkg/store/k8s/k8s.go @@ -71,9 +71,22 @@ func New(c ctrlclient.Client) *Store { // BUG-048 atomic VolumeNumber allocation, where retrying an optimistic- // lock conflict against a stale informer cache re-derives the SAME // number and the create-loop never converges (the second of two -// concurrent `vd c` is dropped). Pass mgr.GetAPIReader() in production; -// nil is accepted and every path falls back to the cached client -// (in-memory / unit harnesses that have no informer). +// concurrent `vd c` is dropped), and the RD/VD GET cache-miss fallback +// (a multi-replica apiserver GET can land on a replica whose cache has +// not observed a just-committed RD yet; VDs are embedded in the RD, so +// both paths must answer consistently). Pass mgr.GetAPIReader() in +// production; nil is accepted and every path falls back to the cached +// client (in-memory / unit harnesses that have no informer). +// +// Do NOT extend the uncached fallback to the other stores' Gets: raw +// store Gets are cache-only by design. REST existence probes rely on a +// fast cached NotFound, and cross-replica lag on read endpoints is +// absorbed at the REST layer by get*WithCacheRetry, which POLLS the +// cached read so subsequent cached Lists stay read-your-writes +// coherent. A store-level fallback short-circuits that convergence +// wait (a CI snapshot-pagination regression demonstrated this when it +// was tried: the create flow's read-back resolved uncached while the +// cached List still under-reported). func NewWithAPIReader(c ctrlclient.Client, apiReader ctrlclient.Reader) *Store { s := &Store{c: c} s.nodes = &nodes{c: c} diff --git a/pkg/store/k8s/store_cache_miss_fallback_internal_test.go b/pkg/store/k8s/store_cache_miss_fallback_internal_test.go new file mode 100644 index 00000000..048fcce6 --- /dev/null +++ b/pkg/store/k8s/store_cache_miss_fallback_internal_test.go @@ -0,0 +1,149 @@ +// 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 k8s + +import ( + "context" + "errors" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + crdv1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + "github.com/cozystack/blockstor/pkg/store" +) + +// These tests align volumeDefinitions.Get with the released +// resourceDefinitions.Get cache-miss behaviour (multi-replica +// apiserver, no leader election). VDs are EMBEDDED in the RD CRD +// spec, so before this fix the SAME committed RD answered differently +// by path: `GET /v1/resource-definitions/{rd}` resolved through the +// RD store's uncached fallback while +// `GET .../volume-definitions/{vn}` read only the lagging cache and +// 404'd (observed live on the cozystack e2e stand as +// `GET .../volume-definitions/0 -> 404` with the REST-layer +// cache-retry budget exhausted). +// +// Deliberately NOT extended to the other stores: raw store Gets are +// cache-only by design — REST existence probes rely on a fast cached +// NotFound, and the REST layer's get*WithCacheRetry helpers already +// absorb cross-replica lag while preserving read-your-writes for +// subsequent cached Lists. A store-level uncached fallback +// short-circuits that convergence wait (demonstrated by the CI +// snapshot-pagination regression when it was tried). + +func cacheMissScheme(t *testing.T) *runtime.Scheme { + t.Helper() + + scheme := runtime.NewScheme() + if err := crdv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + + return scheme +} + +func emptyClient(t *testing.T, scheme *runtime.Scheme) ctrlclient.Client { + t.Helper() + + return fake.NewClientBuilder().WithScheme(scheme).Build() +} + +func clientWith(t *testing.T, scheme *runtime.Scheme, objs ...ctrlclient.Object) ctrlclient.Client { + t.Helper() + + return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() +} + +// TestVolumeDefinitionsGetStaleRDFallsBackToLiveRead pins the subtle VD +// staleness mode: the lagging replica's cache can hold the RD itself +// while still missing a just-committed volume-definition. The cached +// read alone can not distinguish "VD absent" from "RD revision stale" +// — Get must re-read the RD live before answering 404. +func TestVolumeDefinitionsGetStaleRDFallsBackToLiveRead(t *testing.T) { + t.Parallel() + + scheme := cacheMissScheme(t) + + stale := &crdv1alpha1.ResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: Name("pvc-stale")}, + } + fresh := &crdv1alpha1.ResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: Name("pvc-stale")}, + Spec: crdv1alpha1.ResourceDefinitionSpec{ + VolumeDefinitions: []crdv1alpha1.ResourceDefinitionVolume{{ + VolumeNumber: 0, + SizeKib: 1 << 20, + }}, + }, + } + + st := &volumeDefinitions{c: clientWith(t, scheme, stale), apiReader: clientWith(t, scheme, fresh)} + + got, err := st.Get(context.Background(), "pvc-stale", 0) + if err != nil { + t.Fatalf("Get with live re-read fallback: unexpected error: %v", err) + } + + if got.SizeKib != 1<<20 { + t.Fatalf("got SizeKib %d, want %d", got.SizeKib, 1<<20) + } + + // Truly-absent VD must still be NotFound after the live re-read. + if _, err := st.Get(context.Background(), "pvc-stale", 7); !errors.Is(err, store.ErrNotFound) { + t.Fatalf("Get of absent VD: got %v, want store.ErrNotFound", err) + } +} + +// TestVolumeDefinitionsGetCacheMissRDFallsBackToLiveRead pins the +// coarser mode: the RD itself has not reached this replica's cache yet. +func TestVolumeDefinitionsGetCacheMissRDFallsBackToLiveRead(t *testing.T) { + t.Parallel() + + scheme := cacheMissScheme(t) + + fresh := &crdv1alpha1.ResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: Name("pvc-vdmiss")}, + Spec: crdv1alpha1.ResourceDefinitionSpec{ + VolumeDefinitions: []crdv1alpha1.ResourceDefinitionVolume{{ + VolumeNumber: 0, + SizeKib: 4096, + }}, + }, + } + + st := &volumeDefinitions{c: emptyClient(t, scheme), apiReader: clientWith(t, scheme, fresh)} + + got, err := st.Get(context.Background(), "pvc-vdmiss", 0) + if err != nil { + t.Fatalf("Get with live re-read fallback: unexpected error: %v", err) + } + + if got.SizeKib != 4096 { + t.Fatalf("got SizeKib %d, want %d", got.SizeKib, 4096) + } + + nilReader := &volumeDefinitions{c: emptyClient(t, scheme)} + if _, err := nilReader.Get(context.Background(), "pvc-vdmiss", 0); !errors.Is(err, store.ErrNotFound) { + t.Fatalf("Get with nil apiReader: got %v, want store.ErrNotFound", err) + } +} diff --git a/pkg/store/k8s/volume_definitions.go b/pkg/store/k8s/volume_definitions.go index 0ba15f79..9b3ef29a 100644 --- a/pkg/store/k8s/volume_definitions.go +++ b/pkg/store/k8s/volume_definitions.go @@ -72,13 +72,27 @@ func (s *volumeDefinitions) List(ctx context.Context, rdName string) ([]apiv1.Vo func (s *volumeDefinitions) Get(ctx context.Context, rdName string, volumeNumber int32) (apiv1.VolumeDefinition, error) { rd, err := s.fetchRD(ctx, rdName) - if err != nil { + if err == nil { + if vd := vdByNumber(rd, volumeNumber); vd != nil { + return crdToWireVD(vd), nil + } + } else if !errors.Is(err, store.ErrNotFound) || s.apiReader == nil { return apiv1.VolumeDefinition{}, err } - for i := range rd.Spec.VolumeDefinitions { - if rd.Spec.VolumeDefinitions[i].VolumeNumber == volumeNumber { - return crdToWireVD(&rd.Spec.VolumeDefinitions[i]), nil + // Cache miss — or a STALE cached RD revision: volume-definitions are + // embedded in the RD spec, so a lagging replica's cache can hold the + // RD while still missing a just-committed VD (observed live as + // `GET .../volume-definitions/0 -> 404` on the cozystack e2e stand). + // One live re-read before concluding 404. + if s.apiReader != nil { + rd, err = s.fetchRDLive(ctx, rdName) + if err != nil { + return apiv1.VolumeDefinition{}, err + } + + if vd := vdByNumber(rd, volumeNumber); vd != nil { + return crdToWireVD(vd), nil } } @@ -441,3 +455,15 @@ func wireToCRDVD(vd *apiv1.VolumeDefinition) crdv1alpha1.ResourceDefinitionVolum Flags: vd.Flags, } } + +// vdByNumber returns the embedded volume-definition with the given +// VolumeNumber, or nil when the RD spec does not carry it. +func vdByNumber(rd *crdv1alpha1.ResourceDefinition, volumeNumber int32) *crdv1alpha1.ResourceDefinitionVolume { + for i := range rd.Spec.VolumeDefinitions { + if rd.Spec.VolumeDefinitions[i].VolumeNumber == volumeNumber { + return &rd.Spec.VolumeDefinitions[i] + } + } + + return nil +}