Skip to content

Commit

Permalink
Fix: using ownerRef to control the removal of storageversionmigration…
Browse files Browse the repository at this point in the history 
…s. (#342)

Signed-off-by: xuezhaojun <[email protected]>
  • Loading branch information
@xuezhaojun
xuezhaojun authored Jan 10, 2024
1 parent 40135fd commit 4b6e12a
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 84 deletions.
37 changes: 14 additions & 23 deletions ...operator/operators/clustermanager/controllers/migrationcontroller/migration_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,6 @@ func (c *crdMigrationController) sync(ctx context.Context, controllerContext fac
return err
}

// if the ClusterManager is deleting, we remove its related resources on hub
if !clusterManager.DeletionTimestamp.IsZero() {
return removeStorageVersionMigrations(ctx, migrations, migrationClient)
}

// do not apply storage version migrations until other resources are applied
if applied := meta.IsStatusConditionTrue(clusterManager.Status.Conditions, clusterManagerApplied); !applied {
controllerContext.Queue().AddRateLimited(clusterManagerName)
Expand Down Expand Up @@ -189,7 +184,7 @@ func (c *crdMigrationController) sync(ctx context.Context, controllerContext fac
return nil
}

err = createStorageVersionMigrations(ctx, migrations, migrationClient, c.recorder)
err = createStorageVersionMigrations(ctx, migrations, newClusterManagerOwner(clusterManager), migrationClient, c.recorder)
if err != nil {
klog.Errorf("Failed to apply StorageVersionMigrations. %v", err)

Expand Down Expand Up @@ -223,22 +218,6 @@ func supportStorageVersionMigration(ctx context.Context, apiExtensionClient apie
return true, nil
}

func removeStorageVersionMigrations(
ctx context.Context,
toRemoveMigrations []*migrationv1alpha1.StorageVersionMigration,
migrationClient migrationv1alpha1client.StorageVersionMigrationsGetter) error {
for _, migration := range toRemoveMigrations {
err := migrationClient.StorageVersionMigrations().Delete(ctx, migration.Name, metav1.DeleteOptions{})
if errors.IsNotFound(err) {
continue
}
if err != nil {
return nil
}
}
return nil
}

// 1.The CRD must exists before the migration CR is created.
// 2.The CRD must have at least 2 version.
// 3.The version set in the migration CR must exist in the CRD.
Expand Down Expand Up @@ -299,10 +278,11 @@ func checkCRDStorageVersion(ctx context.Context, toCreateMigrations []*migration
// https://github.com/kubernetes-sigs/kube-storage-version-migrator/blob/5c8923c5ff96ceb4435f66b986b5aec2dd0cbc22/pkg/controller/kubemigrator.go#L105-L108
func createStorageVersionMigrations(ctx context.Context,
toCreateMigrations []*migrationv1alpha1.StorageVersionMigration,
ownerRef metav1.OwnerReference,
migrationClient migrationv1alpha1client.StorageVersionMigrationsGetter, recorder events.Recorder) error {
errs := []error{}
for _, migration := range toCreateMigrations {
err := createStorageVersionMigration(ctx, migrationClient, migration, recorder)
err := createStorageVersionMigration(ctx, migrationClient, migration, ownerRef, recorder)
if err != nil {
errs = append(errs, err)
continue
Expand Down Expand Up @@ -411,13 +391,15 @@ func createStorageVersionMigration(
ctx context.Context,
client migrationv1alpha1client.StorageVersionMigrationsGetter,
migration *migrationv1alpha1.StorageVersionMigration,
ownerRefs metav1.OwnerReference,
recorder events.Recorder,
) error {
if migration == nil {
return fmt.Errorf("required StorageVersionMigration is nil")
}
existing, err := client.StorageVersionMigrations().Get(ctx, migration.Name, metav1.GetOptions{})
if errors.IsNotFound(err) {
migration.ObjectMeta.OwnerReferences = append(migration.ObjectMeta.OwnerReferences, ownerRefs)
actual, err := client.StorageVersionMigrations().Create(context.TODO(), migration, metav1.CreateOptions{})
if err != nil {
recorder.Warningf("StorageVersionMigrationCreateFailed", "Failed to create %s: %v", resourcehelper.FormatResourceForCLIWithNamespace(migration), err)
Expand Down Expand Up @@ -479,3 +461,12 @@ func generateHubClients(hubKubeConfig *rest.Config) (apiextensionsclient.Interfa
}
return hubApiExtensionClient, hubMigrationClient, nil
}

func newClusterManagerOwner(clusterManager *operatorapiv1.ClusterManager) metav1.OwnerReference {
return metav1.OwnerReference{
APIVersion: operatorapiv1.GroupVersion.WithKind("ClusterManager").GroupVersion().String(),
Kind: operatorapiv1.GroupVersion.WithKind("ClusterManager").Kind,
Name: clusterManager.Name,
UID: clusterManager.UID,
}
}
67 changes: 6 additions & 61 deletions ...tor/operators/clustermanager/controllers/migrationcontroller/migration_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
fakeapiextensions "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -225,7 +224,12 @@ func TestCreateStorageVersionMigrations(t *testing.T) {
fakeMigrationClient := fakemigrationclient.NewSimpleClientset(c.existingMigrations...)

err := createStorageVersionMigrations(context.TODO(),
c.toCreateMigrations, fakeMigrationClient.MigrationV1alpha1(),
c.toCreateMigrations, newClusterManagerOwner(&operatorapiv1.ClusterManager{
ObjectMeta: metav1.ObjectMeta{
Name: "testhub",
UID: "testhub-uid",
},
}), fakeMigrationClient.MigrationV1alpha1(),
eventstesting.NewTestingEventRecorder(t))
if c.expectErr && err != nil {
return
Expand All @@ -239,65 +243,6 @@ func TestCreateStorageVersionMigrations(t *testing.T) {
}
}

func TestRemoveStorageVersionMigrations(t *testing.T) {
cases := []struct {
name string
existingMigrations []runtime.Object
toRemoveMigrations []*migrationv1alpha1.StorageVersionMigration
}{
{
name: "not exists",
},
{
name: "removed",
existingMigrations: []runtime.Object{
&migrationv1alpha1.StorageVersionMigration{
ObjectMeta: metav1.ObjectMeta{
Name: "foo.cluster.open-cluster-management.io",
},
},
&migrationv1alpha1.StorageVersionMigration{
ObjectMeta: metav1.ObjectMeta{
Name: "bar.cluster.open-cluster-management.io",
},
},
},
toRemoveMigrations: []*migrationv1alpha1.StorageVersionMigration{
{
ObjectMeta: metav1.ObjectMeta{
Name: "foo.cluster.open-cluster-management.io",
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "bar.cluster.open-cluster-management.io",
},
},
},
},
}

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
fakeMigrationClient := fakemigrationclient.NewSimpleClientset(c.existingMigrations...)
err := removeStorageVersionMigrations(context.TODO(), nil, fakeMigrationClient.MigrationV1alpha1())
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

for _, m := range c.toRemoveMigrations {
_, err := fakeMigrationClient.MigrationV1alpha1().StorageVersionMigrations().Get(context.TODO(), m.Name, metav1.GetOptions{})
if errors.IsNotFound(err) {
continue
}
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
}
})
}
}

func assertActions(t *testing.T, actualActions []clienttesting.Action, expectedVerbs ...string) {
if len(actualActions) != len(expectedVerbs) {
t.Fatalf("expected %d call but got: %#v", len(expectedVerbs), actualActions)
Expand Down

0 comments on commit 4b6e12a

Please sign in to comment.