From 4c2a541c4834b205418df313333bf0b6336f3980 Mon Sep 17 00:00:00 2001 From: qmhu Date: Thu, 29 Apr 2021 15:29:15 +0800 Subject: [PATCH 1/3] Use patch to replace update in generic client for multi cases --- pkg/controller/sync/dispatch/unmanaged.go | 9 +++++---- pkg/kubefedctl/disable.go | 4 +++- pkg/kubefedctl/enable/enable.go | 4 +++- pkg/kubefedctl/join.go | 4 +++- test/common/typeconfig.go | 10 ++++++++-- test/e2e/crud.go | 6 ++---- test/e2e/validation.go | 8 ++++++++ 7 files changed, 32 insertions(+), 13 deletions(-) diff --git a/pkg/controller/sync/dispatch/unmanaged.go b/pkg/controller/sync/dispatch/unmanaged.go index de08bf0a63..089047c696 100644 --- a/pkg/controller/sync/dispatch/unmanaged.go +++ b/pkg/controller/sync/dispatch/unmanaged.go @@ -28,7 +28,7 @@ import ( "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/klog/v2" - "sigs.k8s.io/controller-runtime/pkg/client" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/kubefed/pkg/client/generic" "sigs.k8s.io/kubefed/pkg/controller/sync/status" @@ -43,7 +43,7 @@ const eventTemplate = "%s %s %q in cluster %q" type UnmanagedDispatcher interface { OperationDispatcher - Delete(clusterName string, opts ...client.DeleteOption) + Delete(clusterName string, opts ...runtimeclient.DeleteOption) RemoveManagedLabel(clusterName string, clusterObj *unstructured.Unstructured) } @@ -74,7 +74,7 @@ func (d *unmanagedDispatcherImpl) Wait() (bool, error) { return d.dispatcher.Wait() } -func (d *unmanagedDispatcherImpl) Delete(clusterName string, opts ...client.DeleteOption) { +func (d *unmanagedDispatcherImpl) Delete(clusterName string, opts ...runtimeclient.DeleteOption) { start := time.Now() d.dispatcher.incrementOperationsInitiated() const op = "delete" @@ -120,10 +120,11 @@ func (d *unmanagedDispatcherImpl) RemoveManagedLabel(clusterName string, cluster // Avoid mutating the resource in the informer cache updateObj := clusterObj.DeepCopy() + patch := runtimeclient.MergeFrom(updateObj) util.RemoveManagedLabel(updateObj) - err := client.Update(context.Background(), updateObj) + err := client.Patch(context.Background(), updateObj, patch) if err != nil { if d.recorder == nil { wrappedErr := d.wrapOperationError(err, clusterName, op) diff --git a/pkg/kubefedctl/disable.go b/pkg/kubefedctl/disable.go index d14cde5f3b..735a6a9575 100644 --- a/pkg/kubefedctl/disable.go +++ b/pkg/kubefedctl/disable.go @@ -33,6 +33,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/rest" "k8s.io/klog/v2" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/kubefed/pkg/apis/core/typeconfig" fedv1b1 "sigs.k8s.io/kubefed/pkg/apis/core/v1beta1" @@ -262,8 +263,9 @@ func checkFederatedTypeConfigExists(client genericclient.Client, typeConfig *fed func disablePropagation(client genericclient.Client, typeConfig *fedv1b1.FederatedTypeConfig, typeConfigName ctlutil.QualifiedName, write func(string)) error { if typeConfig.GetPropagationEnabled() { + patch := runtimeclient.MergeFrom(typeConfig.DeepCopy()) typeConfig.Spec.Propagation = fedv1b1.PropagationDisabled - err := client.Update(context.TODO(), typeConfig) + err := client.Patch(context.TODO(), typeConfig, patch) if err != nil { return errors.Wrapf(err, "Error disabling propagation for FederatedTypeConfig %q", typeConfigName) } diff --git a/pkg/kubefedctl/enable/enable.go b/pkg/kubefedctl/enable/enable.go index a2ea602a86..0b9f7ab591 100644 --- a/pkg/kubefedctl/enable/enable.go +++ b/pkg/kubefedctl/enable/enable.go @@ -34,6 +34,7 @@ import ( pkgruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/rest" "k8s.io/klog/v2" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/kubefed/pkg/apis/core/typeconfig" fedv1b1 "sigs.k8s.io/kubefed/pkg/apis/core/v1beta1" @@ -338,9 +339,10 @@ func CreateResources(cmdOut io.Writer, config *rest.Config, resources *typeResou } } } else { + patch := runtimeclient.MergeFrom(existingTypeConfig.DeepCopy()) existingTypeConfig.Spec = concreteTypeConfig.Spec if !dryRun { - err = client.Update(context.TODO(), existingTypeConfig) + err := client.Patch(context.TODO(), existingTypeConfig, patch) if err != nil { return errors.Wrapf(err, "Error updating FederatedTypeConfig %q", concreteTypeConfig.Name) } diff --git a/pkg/kubefedctl/join.go b/pkg/kubefedctl/join.go index 277871482b..a662504dbf 100644 --- a/pkg/kubefedctl/join.go +++ b/pkg/kubefedctl/join.go @@ -37,6 +37,7 @@ import ( kubeclient "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" "k8s.io/klog/v2" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" fedv1b1 "sigs.k8s.io/kubefed/pkg/apis/core/v1beta1" genericclient "sigs.k8s.io/kubefed/pkg/client/generic" @@ -361,8 +362,9 @@ func createKubeFedCluster(client genericclient.Client, joiningClusterName, apiEn case err == nil && errorOnExisting: return nil, errors.Errorf("federated cluster %s already exists in host cluster", joiningClusterName) case err == nil: + patch := runtimeclient.MergeFrom(existingFedCluster.DeepCopy()) existingFedCluster.Spec = fedCluster.Spec - err = client.Update(context.TODO(), existingFedCluster) + err := client.Patch(context.TODO(), existingFedCluster, patch) if err != nil { klog.V(2).Infof("Could not update federated cluster %s due to %v", fedCluster.Name, err) return nil, err diff --git a/test/common/typeconfig.go b/test/common/typeconfig.go index c6334b82a0..f1eee6dec6 100644 --- a/test/common/typeconfig.go +++ b/test/common/typeconfig.go @@ -19,6 +19,8 @@ package common import ( "context" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/kubefed/pkg/apis/core/typeconfig" fedv1b1 "sigs.k8s.io/kubefed/pkg/apis/core/v1beta1" client "sigs.k8s.io/kubefed/pkg/client/generic" @@ -34,8 +36,12 @@ func GetTypeConfig(genericClient client.Client, name, namespace string) (typecon return typeConfig, nil } -func UpdateTypeConfig(genericClient client.Client, typeConfig *fedv1b1.FederatedTypeConfig, namespace string) error { - err := genericClient.Update(context.Background(), typeConfig) +func EnableStatusCollection(genericClient client.Client, typeConfig *fedv1b1.FederatedTypeConfig) error { + patch := runtimeclient.MergeFrom(typeConfig.DeepCopy()) + statusCollection := fedv1b1.StatusCollectionEnabled + typeConfig.Spec.StatusCollection = &statusCollection + + err := genericClient.Patch(context.Background(), typeConfig, patch) if err != nil { return err } diff --git a/test/e2e/crud.go b/test/e2e/crud.go index 1c6d671014..b76129888e 100644 --- a/test/e2e/crud.go +++ b/test/e2e/crud.go @@ -284,11 +284,9 @@ func getCrudTestInput(f framework.KubeFedFramework, tl common.TestLogger, tl.Logf("TypeConfig name: %s", tc.GetName()) if tc.GetName() == typeName { tl.Logf("Enabling remote status collection for %v", typeConfig.GetFederatedType().Kind) - statusCollection := v1beta1.StatusCollectionEnabled - tc.Spec.StatusCollection = &statusCollection - err = common.UpdateTypeConfig(client, tc, f.KubeFedSystemNamespace()) + err = common.EnableStatusCollection(client, tc) if err != nil { - tl.Fatalf("Error updating the federatedtypeconfig %q: %v", typeConfigName, err) + tl.Fatalf("Error enabling the federatedtypeconfig %q: %v", typeConfigName, err) } } } diff --git a/test/e2e/validation.go b/test/e2e/validation.go index 19fa1c52df..5a5c48b99d 100644 --- a/test/e2e/validation.go +++ b/test/e2e/validation.go @@ -19,6 +19,7 @@ package e2e import ( "context" "fmt" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -84,6 +85,13 @@ func runValidationResourceTests(f framework.KubeFedFramework, vrt validationReso f.Logger().Fatalf("Expected error updating invalid %s = %+v", resourceName, vrt) } + By(fmt.Sprintf("Patching with an invalid %s", resourceName)) + patch := runtimeclient.MergeFrom(validObj.DeepCopyObject()) + err = client.Patch(context.TODO(), invalidObj, patch) + if err == nil { + f.Logger().Fatalf("Expected error patching invalid %s = %+v", resourceName, vrt) + } + // Immediately delete the created test resource to avoid errors in // other e2e tests that rely on the original e2e testing setup. For // example for KubeFedCluster, delete the test cluster we just From c9a62ff1408491237e458e4db436a8b92d2da7f1 Mon Sep 17 00:00:00 2001 From: qmhu Date: Thu, 29 Apr 2021 16:45:36 +0800 Subject: [PATCH 2/3] gohint for e2e validation --- test/e2e/validation.go | 1 + 1 file changed, 1 insertion(+) diff --git a/test/e2e/validation.go b/test/e2e/validation.go index 5a5c48b99d..99892ade31 100644 --- a/test/e2e/validation.go +++ b/test/e2e/validation.go @@ -19,6 +19,7 @@ package e2e import ( "context" "fmt" + runtimeclient "sigs.k8s.io/controller-runtime/pkg/client" apierrors "k8s.io/apimachinery/pkg/api/errors" From 9d988f5c8195d0905fe0d2b3dd0cf6d3779ba631 Mon Sep 17 00:00:00 2001 From: qmhu Date: Mon, 10 May 2021 17:42:35 +0800 Subject: [PATCH 3/3] bugfix: deepcopy obj before using MergeFrom --- pkg/controller/sync/dispatch/unmanaged.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/sync/dispatch/unmanaged.go b/pkg/controller/sync/dispatch/unmanaged.go index 089047c696..bf968e2079 100644 --- a/pkg/controller/sync/dispatch/unmanaged.go +++ b/pkg/controller/sync/dispatch/unmanaged.go @@ -120,7 +120,7 @@ func (d *unmanagedDispatcherImpl) RemoveManagedLabel(clusterName string, cluster // Avoid mutating the resource in the informer cache updateObj := clusterObj.DeepCopy() - patch := runtimeclient.MergeFrom(updateObj) + patch := runtimeclient.MergeFrom(updateObj.DeepCopy()) util.RemoveManagedLabel(updateObj)