From a2541bfc003ce0a0c4ec3e8289be8120899c77de Mon Sep 17 00:00:00 2001 From: Dario Tranchitella Date: Sat, 9 Jul 2022 10:49:43 +0200 Subject: [PATCH] refactor: retrying mutate function in case of conflict error --- internal/resources/api_server_certificate.go | 4 +-- .../api_server_kubelet_client_certificate.go | 4 +-- internal/resources/ca_certificate.go | 2 +- internal/resources/etcd_ca_certificates.go | 2 +- internal/resources/etcd_certificates.go | 2 +- .../front-proxy-client-certificate.go | 2 +- .../resources/front_proxy_ca_certificate.go | 2 +- internal/resources/k8s_deployment_resource.go | 14 ++++++--- internal/resources/k8s_ingress_resource.go | 10 +++++-- internal/resources/k8s_service_resource.go | 8 +++-- internal/resources/kubeadm_config.go | 2 +- internal/resources/kubeconfig.go | 2 +- internal/resources/sa_certificate.go | 2 +- internal/resources/sql_certificate.go | 2 +- internal/resources/sql_storage_config.go | 2 +- .../utilities/create_or_update_conflict.go | 30 +++++++++++++++++++ 16 files changed, 67 insertions(+), 23 deletions(-) create mode 100644 internal/utilities/create_or_update_conflict.go diff --git a/internal/resources/api_server_certificate.go b/internal/resources/api_server_certificate.go index 8e126dd..305a2e5 100644 --- a/internal/resources/api_server_certificate.go +++ b/internal/resources/api_server_certificate.go @@ -64,8 +64,8 @@ func (r *APIServerCertificate) GetTmpDirectory() string { return r.TmpDirectory } -func (r *APIServerCertificate) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) +func (r *APIServerCertificate) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (res controllerutil.OperationResult, err error) { + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *APIServerCertificate) GetName() string { diff --git a/internal/resources/api_server_kubelet_client_certificate.go b/internal/resources/api_server_kubelet_client_certificate.go index cbc0d76..13c9e7b 100644 --- a/internal/resources/api_server_kubelet_client_certificate.go +++ b/internal/resources/api_server_kubelet_client_certificate.go @@ -64,8 +64,8 @@ func (r *APIServerKubeletClientCertificate) GetTmpDirectory() string { return r.TmpDirectory } -func (r *APIServerKubeletClientCertificate) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) +func (r *APIServerKubeletClientCertificate) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (res controllerutil.OperationResult, err error) { + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *APIServerKubeletClientCertificate) GetName() string { diff --git a/internal/resources/ca_certificate.go b/internal/resources/ca_certificate.go index b347b0b..4c33644 100644 --- a/internal/resources/ca_certificate.go +++ b/internal/resources/ca_certificate.go @@ -65,7 +65,7 @@ func (r *CACertificate) GetTmpDirectory() string { } func (r *CACertificate) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *CACertificate) GetName() string { diff --git a/internal/resources/etcd_ca_certificates.go b/internal/resources/etcd_ca_certificates.go index 8fc4dae..0d10dc7 100644 --- a/internal/resources/etcd_ca_certificates.go +++ b/internal/resources/etcd_ca_certificates.go @@ -59,7 +59,7 @@ func (r *ETCDCACertificatesResource) Define(ctx context.Context, tenantControlPl } func (r *ETCDCACertificatesResource) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *ETCDCACertificatesResource) GetName() string { diff --git a/internal/resources/etcd_certificates.go b/internal/resources/etcd_certificates.go index 32efe44..45d0fa3 100644 --- a/internal/resources/etcd_certificates.go +++ b/internal/resources/etcd_certificates.go @@ -56,7 +56,7 @@ func (r *ETCDCertificatesResource) Define(ctx context.Context, tenantControlPlan } func (r *ETCDCertificatesResource) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *ETCDCertificatesResource) GetName() string { diff --git a/internal/resources/front-proxy-client-certificate.go b/internal/resources/front-proxy-client-certificate.go index a3b4102..941c755 100644 --- a/internal/resources/front-proxy-client-certificate.go +++ b/internal/resources/front-proxy-client-certificate.go @@ -65,7 +65,7 @@ func (r *FrontProxyClientCertificate) GetTmpDirectory() string { } func (r *FrontProxyClientCertificate) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *FrontProxyClientCertificate) GetName() string { diff --git a/internal/resources/front_proxy_ca_certificate.go b/internal/resources/front_proxy_ca_certificate.go index a981527..2aa941f 100644 --- a/internal/resources/front_proxy_ca_certificate.go +++ b/internal/resources/front_proxy_ca_certificate.go @@ -66,7 +66,7 @@ func (r *FrontProxyCACertificate) GetTmpDirectory() string { } func (r *FrontProxyCACertificate) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *FrontProxyCACertificate) GetName() string { diff --git a/internal/resources/k8s_deployment_resource.go b/internal/resources/k8s_deployment_resource.go index 706d4ba..728df0c 100644 --- a/internal/resources/k8s_deployment_resource.go +++ b/internal/resources/k8s_deployment_resource.go @@ -73,17 +73,19 @@ func (r *KubernetesDeploymentResource) Define(ctx context.Context, tenantControl return nil } -func (r *KubernetesDeploymentResource) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { +func (r *KubernetesDeploymentResource) mutate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) controllerutil.MutateFn { maxSurge := intstr.FromString("100%") maxUnavailable := intstr.FromInt(0) address, err := tenantControlPlane.GetControlPlaneAddress(ctx, r.Client) if err != nil { - return controllerutil.OperationResultNone, errors.Wrap(err, "cannot create TenantControlPlane Deployment") + return func() error { + return errors.Wrap(err, "cannot create TenantControlPlane Deployment") + } } - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, func() error { + return func() error { labels := utilities.MergeMaps(r.resource.GetLabels(), tenantControlPlane.Spec.ControlPlane.Deployment.AdditionalMetadata.Labels) r.resource.SetLabels(labels) @@ -500,7 +502,11 @@ func (r *KubernetesDeploymentResource) CreateOrUpdate(ctx context.Context, tenan } return controllerutil.SetControllerReference(tenantControlPlane, r.resource, r.Client.Scheme()) - }) + } +} + +func (r *KubernetesDeploymentResource) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *KubernetesDeploymentResource) GetName() string { diff --git a/internal/resources/k8s_ingress_resource.go b/internal/resources/k8s_ingress_resource.go index de879c8..d22062d 100644 --- a/internal/resources/k8s_ingress_resource.go +++ b/internal/resources/k8s_ingress_resource.go @@ -73,8 +73,8 @@ func (r *KubernetesIngressResource) Define(ctx context.Context, tenantControlPla return nil } -func (r *KubernetesIngressResource) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, func() error { +func (r *KubernetesIngressResource) mutate(tenantControlPlane *kamajiv1alpha1.TenantControlPlane) controllerutil.MutateFn { + return func() error { labels := utilities.MergeMaps(r.resource.GetLabels(), tenantControlPlane.Spec.ControlPlane.Ingress.AdditionalMetadata.Labels) r.resource.SetLabels(labels) @@ -131,7 +131,11 @@ func (r *KubernetesIngressResource) CreateOrUpdate(ctx context.Context, tenantCo } return controllerutil.SetControllerReference(tenantControlPlane, r.resource, r.Client.Scheme()) - }) + } +} + +func (r *KubernetesIngressResource) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(tenantControlPlane)) } func (r *KubernetesIngressResource) GetName() string { diff --git a/internal/resources/k8s_service_resource.go b/internal/resources/k8s_service_resource.go index a15c58c..f50ebc5 100644 --- a/internal/resources/k8s_service_resource.go +++ b/internal/resources/k8s_service_resource.go @@ -62,11 +62,15 @@ func (r *KubernetesServiceResource) Define(ctx context.Context, tenantControlPla } func (r *KubernetesServiceResource) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) +} + +func (r *KubernetesServiceResource) mutate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) controllerutil.MutateFn { // We don't need to check error here: in case of dynamic external IP, the Service must be created in advance. // After that, the specific cloud controller-manager will provide an IP that will be then used. address, _ := tenantControlPlane.GetControlPlaneAddress(ctx, r.Client) - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, func() error { + return func() error { var servicePort corev1.ServicePort if len(r.resource.Spec.Ports) > 0 { servicePort = r.resource.Spec.Ports[0] @@ -109,7 +113,7 @@ func (r *KubernetesServiceResource) CreateOrUpdate(ctx context.Context, tenantCo } return controllerutil.SetControllerReference(tenantControlPlane, r.resource, r.Client.Scheme()) - }) + } } func (r *KubernetesServiceResource) GetName() string { diff --git a/internal/resources/kubeadm_config.go b/internal/resources/kubeadm_config.go index d81fc99..3f94994 100644 --- a/internal/resources/kubeadm_config.go +++ b/internal/resources/kubeadm_config.go @@ -71,7 +71,7 @@ func (r *KubeadmConfigResource) CreateOrUpdate(ctx context.Context, tenantContro return controllerutil.OperationResultNone, err } - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(tenantControlPlane, address)) + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(tenantControlPlane, address)) } func (r *KubeadmConfigResource) GetName() string { diff --git a/internal/resources/kubeconfig.go b/internal/resources/kubeconfig.go index 79ff059..d4bab35 100644 --- a/internal/resources/kubeconfig.go +++ b/internal/resources/kubeconfig.go @@ -102,7 +102,7 @@ func (r *KubeconfigResource) getKubeconfigStatus(tenantControlPlane *kamajiv1alp } func (r *KubeconfigResource) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *KubeconfigResource) mutate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) controllerutil.MutateFn { diff --git a/internal/resources/sa_certificate.go b/internal/resources/sa_certificate.go index 3ee6d8f..ce16fab 100644 --- a/internal/resources/sa_certificate.go +++ b/internal/resources/sa_certificate.go @@ -64,7 +64,7 @@ func (r *SACertificate) GetTmpDirectory() string { } func (r *SACertificate) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *SACertificate) GetName() string { diff --git a/internal/resources/sql_certificate.go b/internal/resources/sql_certificate.go index 2d9a98e..dd22cae 100644 --- a/internal/resources/sql_certificate.go +++ b/internal/resources/sql_certificate.go @@ -62,7 +62,7 @@ func (r *SQLCertificate) GetClient() client.Client { } func (r *SQLCertificate) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *SQLCertificate) GetName() string { diff --git a/internal/resources/sql_storage_config.go b/internal/resources/sql_storage_config.go index f604f23..bf32905 100644 --- a/internal/resources/sql_storage_config.go +++ b/internal/resources/sql_storage_config.go @@ -62,7 +62,7 @@ func (r *SQLStorageConfig) GetClient() client.Client { } func (r *SQLStorageConfig) CreateOrUpdate(ctx context.Context, tenantControlPlane *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error) { - return controllerutil.CreateOrUpdate(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) + return utilities.CreateOrUpdateWithConflict(ctx, r.Client, r.resource, r.mutate(ctx, tenantControlPlane)) } func (r *SQLStorageConfig) GetName() string { diff --git a/internal/utilities/create_or_update_conflict.go b/internal/utilities/create_or_update_conflict.go new file mode 100644 index 0000000..3550b65 --- /dev/null +++ b/internal/utilities/create_or_update_conflict.go @@ -0,0 +1,30 @@ +// Copyright 2022 Clastix Labs +// SPDX-License-Identifier: Apache-2.0 + +package utilities + +import ( + "context" + + k8stypes "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +// CreateOrUpdateWithConflict is a helper function that wraps the RetryOnConflict around the CreateOrUpdate function: +// this allows to fetch from the cache the latest modified object an try to apply the changes defined in the MutateFn +// without enqueuing back the request in order to get the latest changes of the resource. +func CreateOrUpdateWithConflict(ctx context.Context, client client.Client, resource client.Object, f controllerutil.MutateFn) (res controllerutil.OperationResult, err error) { + err = retry.RetryOnConflict(retry.DefaultRetry, func() (scopeErr error) { + if scopeErr = client.Get(ctx, k8stypes.NamespacedName{Namespace: resource.GetNamespace(), Name: resource.GetName()}, resource); err != nil { + return err + } + + res, scopeErr = controllerutil.CreateOrUpdate(ctx, client, resource, f) + + return scopeErr + }) + + return +}