Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions pkg/store/k8s/k8s.go
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down
149 changes: 149 additions & 0 deletions pkg/store/k8s/store_cache_miss_fallback_internal_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
34 changes: 30 additions & 4 deletions pkg/store/k8s/volume_definitions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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
}