From faf5b3ec765943460320e1aac3aa3aaf74b901e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stanislav=20L=C3=A1zni=C4=8Dka?= Date: Mon, 17 Feb 2025 16:01:34 +0100 Subject: [PATCH 1/4] integration:svm: refactor utils - remove unused receiver from getETCDPathForResource() - flatten API discovery loop --- .../storageversionmigrator/util.go | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/test/integration/storageversionmigrator/util.go b/test/integration/storageversionmigrator/util.go index 7b78e673209..49e765a849a 100644 --- a/test/integration/storageversionmigrator/util.go +++ b/test/integration/storageversionmigrator/util.go @@ -389,7 +389,7 @@ func (svm *svmTest) createSecret(ctx context.Context, t *testing.T, name, namesp func (svm *svmTest) getRawSecretFromETCD(t *testing.T, name, namespace string) ([]byte, error) { t.Helper() - secretETCDPath := svm.getETCDPathForResource(t, svm.storageConfig.Prefix, "", "secrets", name, namespace) + secretETCDPath := getETCDPathForResource(t, svm.storageConfig.Prefix, "", "secrets", name, namespace) etcdResponse, err := svm.readRawRecordFromETCD(t, secretETCDPath) if err != nil { return nil, fmt.Errorf("failed to read %s from etcd: %w", secretETCDPath, err) @@ -397,7 +397,7 @@ func (svm *svmTest) getRawSecretFromETCD(t *testing.T, name, namespace string) ( return etcdResponse.Kvs[0].Value, nil } -func (svm *svmTest) getETCDPathForResource(t *testing.T, storagePrefix, group, resource, name, namespaceName string) string { +func getETCDPathForResource(t *testing.T, storagePrefix, group, resource, name, namespaceName string) string { t.Helper() groupResource := resource if group != "" { @@ -433,7 +433,7 @@ func (svm *svmTest) readRawRecordFromETCD(t *testing.T, path string) (*clientv3. func (svm *svmTest) getRawCRFromETCD(t *testing.T, name, namespace, crdGroup, crdName string) ([]byte, error) { t.Helper() - crdETCDPath := svm.getETCDPathForResource(t, svm.storageConfig.Prefix, crdGroup, crdName, name, namespace) + crdETCDPath := getETCDPathForResource(t, svm.storageConfig.Prefix, crdGroup, crdName, name, namespace) etcdResponse, err := svm.readRawRecordFromETCD(t, crdETCDPath) if err != nil { t.Fatalf("failed to read %s from etcd: %v", crdETCDPath, err) @@ -807,29 +807,30 @@ func (svm *svmTest) waitForCRDUpdate( return false, fmt.Errorf("failed to get server groups and resources: %w", err) } for _, api := range apiGroups { - if api.Name == crdGroup { - var servingVersions []string - for _, apiVersion := range api.Versions { - servingVersions = append(servingVersions, apiVersion.Version) - } - sort.Strings(servingVersions) + if api.Name != crdGroup { + continue + } + var servingVersions []string + for _, apiVersion := range api.Versions { + servingVersions = append(servingVersions, apiVersion.Version) + } + sort.Strings(servingVersions) - // Check if the serving versions are as expected - if reflect.DeepEqual(expectedServingVersions, servingVersions) { - expectedHash := endpointsdiscovery.StorageVersionHash(crdGroup, expectedStorageVersion, crdKind) - resourceList, err := svm.discoveryClient.ServerResourcesForGroupVersion(crdGroup + "/" + api.PreferredVersion.Version) - if err != nil { - return false, fmt.Errorf("failed to get server resources for group version: %w", err) - } + // Check if the serving versions are as expected + if !reflect.DeepEqual(expectedServingVersions, servingVersions) { + continue + } - // Check if the storage version is as expected - for _, resource := range resourceList.APIResources { - if resource.Kind == crdKind { - if resource.StorageVersionHash == expectedHash { - return true, nil - } - } - } + expectedHash := endpointsdiscovery.StorageVersionHash(crdGroup, expectedStorageVersion, crdKind) + resourceList, err := svm.discoveryClient.ServerResourcesForGroupVersion(crdGroup + "/" + api.PreferredVersion.Version) + if err != nil { + return false, fmt.Errorf("failed to get server resources for group version: %w", err) + } + + // Check if the storage version is as expected + for _, resource := range resourceList.APIResources { + if resource.Kind == crdKind && resource.StorageVersionHash == expectedHash { + return true, nil } } } From e1557f80a2ac977d669c6efaeeb97f07ad0e8418 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stanislav=20L=C3=A1zni=C4=8Dka?= Date: Wed, 19 Feb 2025 15:10:37 +0100 Subject: [PATCH 2/4] integration: svm: use k8s ktesting package for test ctx init --- .../storageversionmigrator_test.go | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/test/integration/storageversionmigrator/storageversionmigrator_test.go b/test/integration/storageversionmigrator/storageversionmigrator_test.go index 581064e4af0..70ae6dac801 100644 --- a/test/integration/storageversionmigrator/storageversionmigrator_test.go +++ b/test/integration/storageversionmigrator/storageversionmigrator_test.go @@ -18,7 +18,6 @@ package storageversionmigrator import ( "bytes" - "context" "strconv" "sync" "testing" @@ -34,9 +33,9 @@ import ( clientgofeaturegate "k8s.io/client-go/features" "k8s.io/component-base/featuregate" featuregatetesting "k8s.io/component-base/featuregate/testing" - "k8s.io/klog/v2/ktesting" "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/test/integration/framework" + "k8s.io/kubernetes/test/utils/ktesting" ) // TestStorageVersionMigration is an integration test that verifies storage version migration works. @@ -56,9 +55,7 @@ func TestStorageVersionMigration(t *testing.T) { // this makes the test super responsive. It's set to a default of 1 minute. encryptionconfigcontroller.EncryptionConfigFileChangePollDuration = time.Second - _, ctx := ktesting.NewTestContext(t) - ctx, cancel := context.WithCancel(ctx) - defer cancel() + ctx := ktesting.Init(t) svmTest := svmSetup(ctx, t) @@ -163,9 +160,7 @@ func TestStorageVersionMigrationWithCRD(t *testing.T) { goleak.IgnoreTopFunction("github.com/moby/spdystream.(*Connection).shutdown"), ) - _, ctx := ktesting.NewTestContext(t) - ctx, cancel := context.WithCancel(ctx) - defer cancel() + ctx := ktesting.Init(t) crVersions := make(map[string]versions) @@ -291,9 +286,7 @@ func TestStorageVersionMigrationDuringChaos(t *testing.T) { featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StorageVersionMigrator, true) featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, featuregate.Feature(clientgofeaturegate.InformerResourceVersion), true) - _, ctx := ktesting.NewTestContext(t) - ctx, cancel := context.WithCancel(ctx) - t.Cleanup(cancel) + ctx := ktesting.Init(t) svmTest := svmSetup(ctx, t) From 80966ce5c44dcf79b7617c592044469db85b1d59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stanislav=20L=C3=A1zni=C4=8Dka?= Date: Wed, 19 Feb 2025 15:22:53 +0100 Subject: [PATCH 3/4] integration: svm: use consistent path args pattern in etcd fetch functions Use function argument order at which the strings would appear in the etcd path. --- .../storageversionmigrator_test.go | 2 +- test/integration/storageversionmigrator/util.go | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/test/integration/storageversionmigrator/storageversionmigrator_test.go b/test/integration/storageversionmigrator/storageversionmigrator_test.go index 70ae6dac801..1e20485368d 100644 --- a/test/integration/storageversionmigrator/storageversionmigrator_test.go +++ b/test/integration/storageversionmigrator/storageversionmigrator_test.go @@ -89,7 +89,7 @@ func TestStorageVersionMigration(t *testing.T) { } wantPrefix := "k8s:enc:aescbc:v1:key2" - etcdSecret, err := svmTest.getRawSecretFromETCD(t, secret.Name, secret.Namespace) + etcdSecret, err := svmTest.getRawSecretFromETCD(t, secret.Namespace, secret.Name) if err != nil { t.Fatalf("Failed to get secret from etcd: %v", err) } diff --git a/test/integration/storageversionmigrator/util.go b/test/integration/storageversionmigrator/util.go index 49e765a849a..8567214d59a 100644 --- a/test/integration/storageversionmigrator/util.go +++ b/test/integration/storageversionmigrator/util.go @@ -387,9 +387,9 @@ func (svm *svmTest) createSecret(ctx context.Context, t *testing.T, name, namesp return svm.client.CoreV1().Secrets(secret.Namespace).Create(ctx, secret, metav1.CreateOptions{}) } -func (svm *svmTest) getRawSecretFromETCD(t *testing.T, name, namespace string) ([]byte, error) { +func (svm *svmTest) getRawSecretFromETCD(t *testing.T, namespace, name string) ([]byte, error) { t.Helper() - secretETCDPath := getETCDPathForResource(t, svm.storageConfig.Prefix, "", "secrets", name, namespace) + secretETCDPath := getETCDPathForResource(t, svm.storageConfig.Prefix, "", "secrets", namespace, name) etcdResponse, err := svm.readRawRecordFromETCD(t, secretETCDPath) if err != nil { return nil, fmt.Errorf("failed to read %s from etcd: %w", secretETCDPath, err) @@ -397,7 +397,7 @@ func (svm *svmTest) getRawSecretFromETCD(t *testing.T, name, namespace string) ( return etcdResponse.Kvs[0].Value, nil } -func getETCDPathForResource(t *testing.T, storagePrefix, group, resource, name, namespaceName string) string { +func getETCDPathForResource(t *testing.T, storagePrefix, group, resource, namespaceName, name string) string { t.Helper() groupResource := resource if group != "" { @@ -431,9 +431,9 @@ func (svm *svmTest) readRawRecordFromETCD(t *testing.T, path string) (*clientv3. return response, nil } -func (svm *svmTest) getRawCRFromETCD(t *testing.T, name, namespace, crdGroup, crdName string) ([]byte, error) { +func (svm *svmTest) getRawCRFromETCD(t *testing.T, crdGroup, crdName, namespace, name string) ([]byte, error) { t.Helper() - crdETCDPath := getETCDPathForResource(t, svm.storageConfig.Prefix, crdGroup, crdName, name, namespace) + crdETCDPath := getETCDPathForResource(t, svm.storageConfig.Prefix, crdGroup, crdName, namespace, name) etcdResponse, err := svm.readRawRecordFromETCD(t, crdETCDPath) if err != nil { t.Fatalf("failed to read %s from etcd: %v", crdETCDPath, err) @@ -1056,7 +1056,7 @@ func (svm *svmTest) setupServerCert(t *testing.T) *certContext { func (svm *svmTest) isCRStoredAtVersion(t *testing.T, version, crName string) bool { t.Helper() - data, err := svm.getRawCRFromETCD(t, crName, defaultNamespace, crdGroup, crdName+"s") + data, err := svm.getRawCRFromETCD(t, crdGroup, crdName+"s", defaultNamespace, crName) if err != nil { t.Fatalf("Failed to get CR from etcd: %v", err) } @@ -1135,7 +1135,7 @@ func (svm *svmTest) validateRVAndGeneration(ctx context.Context, t *testing.T, c for crName, version := range crVersions { // get CR from etcd - data, err := svm.getRawCRFromETCD(t, crName, defaultNamespace, crdGroup, crdName+"s") + data, err := svm.getRawCRFromETCD(t, crdGroup, crdName+"s", defaultNamespace, crName) if err != nil { t.Fatalf("Failed to get CR from etcd: %v", err) } From a0a226d158474a356816eee9adb786b7c24f9828 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stanislav=20L=C3=A1zni=C4=8Dka?= Date: Wed, 19 Feb 2025 15:50:24 +0100 Subject: [PATCH 4/4] integration: svm: wait for CR to be stored as v2 after CRD v2 switch --- .../storageversionmigrator_test.go | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/test/integration/storageversionmigrator/storageversionmigrator_test.go b/test/integration/storageversionmigrator/storageversionmigrator_test.go index 1e20485368d..4f9fbb439b8 100644 --- a/test/integration/storageversionmigrator/storageversionmigrator_test.go +++ b/test/integration/storageversionmigrator/storageversionmigrator_test.go @@ -18,6 +18,7 @@ package storageversionmigrator import ( "bytes" + "context" "strconv" "sync" "testing" @@ -27,6 +28,8 @@ import ( svmv1alpha1 "k8s.io/api/storagemigration/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/wait" encryptionconfigcontroller "k8s.io/apiserver/pkg/server/options/encryptionconfig/controller" etcd3watcher "k8s.io/apiserver/pkg/storage/etcd3" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -205,10 +208,24 @@ func TestStorageVersionMigrationWithCRD(t *testing.T) { svmTest.updateCRD(ctx, t, crd.Name, v2StorageCRDVersion, []string{"v1", "v2"}, "v2") // create CR with v1 - cr3 := svmTest.createCR(ctx, t, "cr3", "v1") - if ok := svmTest.isCRStoredAtVersion(t, "v2", cr3.GetName()); !ok { - t.Fatalf("CR not stored at version v2") + var cr3 *unstructured.Unstructured + // updateCRD checks discovery returns storageVersionHash matching storage version v2 + // to make sure the API server uses v2 but CRD controllers may race and the resource + // might still get stored in v1. + // Attempt to recreate the CR until it gets stored as v2. + // https://github.com/kubernetes/kubernetes/issues/130235 + err := wait.PollUntilContextTimeout(ctx, 1*time.Second, 10*time.Second, true, func(waitCtx context.Context) (done bool, err error) { + cr3 = svmTest.createCR(waitCtx, t, "cr3", "v1") + if ok := svmTest.isCRStoredAtVersion(t, "v2", cr3.GetName()); !ok { + svmTest.deleteCR(waitCtx, t, cr3.GetName(), "v1") + return false, nil + } + return true, nil + }) + if err != nil { + t.Fatalf("timed out waiting for CR to be stored as v2: %v", err) } + crVersions[cr3.GetName()] = versions{ generation: cr3.GetGeneration(), rv: cr3.GetResourceVersion(),