installplans: retry crd updates on conflicts (#2293)

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
operator-framework · Jul 29, 2021 · 8b80f26 · 8b80f26
1 parent 939bf94
commit 8b80f26
Show file tree

Hide file tree

Showing 2 changed files with 114 additions and 96 deletions.
diff --git a/pkg/controller/install/webhook.go b/pkg/controller/install/webhook.go
@@ -5,16 +5,17 @@ import (
  "fmt"
  "hash/fnv"
 
- "github.com/operator-framework/api/pkg/operators/v1alpha1"
- hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash"
- "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
-
  log "github.com/sirupsen/logrus"
  admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
  apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
  "k8s.io/apimachinery/pkg/labels"
  "k8s.io/apimachinery/pkg/util/rand"
+ "k8s.io/client-go/util/retry"
+
+ "github.com/operator-framework/api/pkg/operators/v1alpha1"
+ hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash"
+ "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 )
 
 func ValidWebhookRules(rules []admissionregistrationv1.RuleWithOperations) error {
@@ -200,69 +201,74 @@ func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []by
 
  // iterate over all the ConversionCRDs
  for _, conversionCRD := range desc.ConversionCRDs {
- // Get existing CRD on cluster
- crd, err := i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{})
- if err != nil {
- return fmt.Errorf("Unable to get CRD %s specified in Conversion Webhook: %v", conversionCRD, err)
- }
+ if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+ // Get existing CRD on cluster
+ crd, err := i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{})
+ if err != nil {
+ return fmt.Errorf("Unable to get CRD %s specified in Conversion Webhook: %v", conversionCRD, err)
+ }
 
- // check if this CRD is an owned CRD
- foundCRD := false
- for _, ownedCRD := range csv.Spec.CustomResourceDefinitions.Owned {
- if ownedCRD.Name == conversionCRD {
- foundCRD = true
- break
+ // check if this CRD is an owned CRD
+ foundCRD := false
+ for _, ownedCRD := range csv.Spec.CustomResourceDefinitions.Owned {
+ if ownedCRD.Name == conversionCRD {
+ foundCRD = true
+ break
+ }
+ }
+ if !foundCRD {
+ return fmt.Errorf("CSV %s does not own CRD %s", csv.GetName(), conversionCRD)
  }
- }
- if !foundCRD {
- return fmt.Errorf("CSV %s does not own CRD %s", csv.GetName(), conversionCRD)
- }
 
- // crd.Spec.Conversion.Strategy specifies how custom resources are converted between versions.
- // Allowed values are:
- // - None: The converter only change the apiVersion and would not touch any other field in the custom resource.
- // - Webhook: API Server will call to an external webhook to do the conversion. This requires crd.Spec.preserveUnknownFields to be false.
- // References:
- // - https://docs.openshift.com/container-platform/4.5/rest_api/extension_apis/customresourcedefinition-apiextensions-k8s-io-v1.html
- // - https://kubernetes.io/blog/2019/06/20/crd-structural-schema/#pruning-don-t-preserve-unknown-fields
- // By default the strategy is none
- // Reference:
- // - https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
- if crd.Spec.PreserveUnknownFields != false {
- return fmt.Errorf("crd.Spec.PreserveUnknownFields must be false to let API Server call webhook to do the conversion")
- }
+  // crd.Spec.Conversion.Strategy specifies how custom resources are converted between versions.
+  // Allowed values are:
+  // - None: The converter only change the apiVersion and would not touch any other field in the custom resource.
+  // - Webhook: API Server will call to an external webhook to do the conversion. This requires crd.Spec.preserveUnknownFields to be false.
+  // References:
+  // - https://docs.openshift.com/container-platform/4.5/rest_api/extension_apis/customresourcedefinition-apiextensions-k8s-io-v1.html
+  // - https://kubernetes.io/blog/2019/06/20/crd-structural-schema/#pruning-don-t-preserve-unknown-fields
+  // By default the strategy is none
+  // Reference:
+  // - https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
+  if crd.Spec.PreserveUnknownFields != false {
+  return fmt.Errorf("crd.Spec.PreserveUnknownFields must be false to let API Server call webhook to do the conversion")
+  }
 
- // Conversion WebhookClientConfig should not be set when Strategy is None
- // https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
- // Conversion WebhookClientConfig needs to be set when Strategy is None
- // https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#configure-customresourcedefinition-to-use-conversion-webhooks
+  // Conversion WebhookClientConfig should not be set when Strategy is None
+  // https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
+  // Conversion WebhookClientConfig needs to be set when Strategy is None
+  // https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#configure-customresourcedefinition-to-use-conversion-webhooks
 
- // use user defined path for CRD conversion webhook, else set default value
- conversionWebhookPath := "/"
- if desc.WebhookPath != nil {
- conversionWebhookPath = *desc.WebhookPath
- }
+  // use user defined path for CRD conversion webhook, else set default value
+  conversionWebhookPath := "/"
+  if desc.WebhookPath != nil {
+  conversionWebhookPath = *desc.WebhookPath
+  }
 
- // Override Name, Namespace, and CABundle
- crd.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{
- Strategy: "Webhook",
- Webhook: &apiextensionsv1.WebhookConversion{
- ClientConfig: &apiextensionsv1.WebhookClientConfig{
- Service: &apiextensionsv1.ServiceReference{
- Namespace: i.owner.GetNamespace(),
- Name: desc.DomainName() + "-service",
- Path: &conversionWebhookPath,
- Port: &desc.ContainerPort,
+ // Override Name, Namespace, and CABundle
+ crd.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{
+ Strategy: "Webhook",
+ Webhook: &apiextensionsv1.WebhookConversion{
+ ClientConfig: &apiextensionsv1.WebhookClientConfig{
+ Service: &apiextensionsv1.ServiceReference{
+ Namespace: i.owner.GetNamespace(),
+ Name: desc.DomainName() + "-service",
+ Path: &conversionWebhookPath,
+ Port: &desc.ContainerPort,
+ },
+ CABundle: caPEM,
  },
- CABundle: caPEM,
+ ConversionReviewVersions: desc.AdmissionReviewVersions,
  },
- ConversionReviewVersions: desc.AdmissionReviewVersions,
- },
- }
+ }
 
- // update CRD conversion Specs
- if _, err = i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); err != nil {
- return fmt.Errorf("Error updating CRD with Conversion info: %v", err)
+ // update CRD conversion Specs
+ if _, err = i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); err != nil {
+ return fmt.Errorf("Error updating CRD with Conversion info: %w", err)
+ }
+ return nil
+ }); err != nil {
+ return err
  }
  }
 

diff --git a/pkg/controller/operators/catalog/step.go b/pkg/controller/operators/catalog/step.go
@@ -6,14 +6,14 @@ import (
 
  "github.com/pkg/errors"
  "github.com/sirupsen/logrus"
-
  apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
  apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
  apiextensionsv1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
  apiextensionsv1beta1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1beta1"
  k8serrors "k8s.io/apimachinery/pkg/api/errors"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
  "k8s.io/client-go/dynamic"
+ "k8s.io/client-go/util/retry"
 
  "github.com/operator-framework/api/pkg/operators/v1alpha1"
  listersv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1"
@@ -132,27 +132,33 @@ func (b *builder) NewCRDV1Step(client apiextensionsv1client.ApiextensionsV1Inter
 
  _, createError := client.CustomResourceDefinitions().Create(context.TODO(), crd, metav1.CreateOptions{})
  if k8serrors.IsAlreadyExists(createError) {
- currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{})
- crd.SetResourceVersion(currentCRD.GetResourceVersion())
- if err = validateV1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil {
- return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "error validating existing CRs against new CRD's schema: %s", step.Resource.Name)
- }
+ err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+ currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{})
+ crd.SetResourceVersion(currentCRD.GetResourceVersion())
+ if err = validateV1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil {
+ return fmt.Errorf("error validating existing CRs against new CRD's schema for %q: %w", step.Resource.Name, err)
+ }
 
- // check to see if stored versions changed and whether the upgrade could cause potential data loss
- safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
- if !safe {
- b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err)
- return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "risk of data loss updating %s", step.Resource.Name)
- }
- if err != nil {
- return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "checking CRD for potential data loss updating %s", step.Resource.Name)
- }
+  // check to see if stored versions changed and whether the upgrade could cause potential data loss
+  safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
+  if !safe {
+  b.logger.Errorf("risk of data loss updating %q: %s", step.Resource.Name, err)
+  return fmt.Errorf("risk of data loss updating %q: %w", step.Resource.Name, err)
+  }
+  if err != nil {
+  return fmt.Errorf("checking CRD for potential data loss updating %q: %w", step.Resource.Name, err)
+  }
 
- // Update CRD to new version
- setInstalledAlongsideAnnotation(b.annotator, crd, b.plan.GetNamespace(), step.Resolving, b.csvLister, crd, currentCRD)
- _, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})
+ // Update CRD to new version
+ setInstalledAlongsideAnnotation(b.annotator, crd, b.plan.GetNamespace(), step.Resolving, b.csvLister, crd, currentCRD)
+ _, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})
+ if err != nil {
+ return fmt.Errorf("error updating CRD %q: %w", step.Resource.Name, err)
+ }
+ return nil
+ })
  if err != nil {
- return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "error updating CRD: %s", step.Resource.Name)
+ return v1alpha1.StepStatusUnknown, err
  }
  // If it already existed, mark the step as Present.
  // they were equal - mark CRD as present
@@ -181,7 +187,7 @@ func (b *builder) NewCRDV1Beta1Step(client apiextensionsv1beta1client.Apiextensi
  if k8serrors.IsNotFound(err) {
  return v1alpha1.StepStatusNotPresent, nil
  } else {
- return v1alpha1.StepStatusNotPresent, errors.Wrapf(err, "error finding the %s CRD", crd.Name)
+ return v1alpha1.StepStatusNotPresent, fmt.Errorf("error finding the %q CRD: %w", crd.Name, err)
  }
  }
  established, namesAccepted := false, false
@@ -210,28 +216,34 @@ func (b *builder) NewCRDV1Beta1Step(client apiextensionsv1beta1client.Apiextensi
 
  _, createError := client.CustomResourceDefinitions().Create(context.TODO(), crd, metav1.CreateOptions{})
  if k8serrors.IsAlreadyExists(createError) {
- currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{})
- crd.SetResourceVersion(currentCRD.GetResourceVersion())
+ err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+ currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{})
+ crd.SetResourceVersion(currentCRD.GetResourceVersion())
 
- if err = validateV1Beta1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil {
- return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "error validating existing CRs against new CRD's schema: %s", step.Resource.Name)
- }
+  if err = validateV1Beta1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil {
+  return fmt.Errorf("error validating existing CRs against new CRD's schema for %q: %w", step.Resource.Name, err)
+  }
 
- // check to see if stored versions changed and whether the upgrade could cause potential data loss
- safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
- if !safe {
- b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err)
- return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "risk of data loss updating %s", step.Resource.Name)
- }
- if err != nil {
- return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "checking CRD for potential data loss updating %s", step.Resource.Name)
- }
+  // check to see if stored versions changed and whether the upgrade could cause potential data loss
+  safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
+  if !safe {
+  b.logger.Errorf("risk of data loss updating %q: %s", step.Resource.Name, err)
+  return fmt.Errorf("risk of data loss updating %q: %w", step.Resource.Name, err)
+  }
+  if err != nil {
+  return fmt.Errorf("checking CRD for potential data loss updating %q: %w", step.Resource.Name, err)
+  }
 
- // Update CRD to new version
- setInstalledAlongsideAnnotation(b.annotator, crd, b.plan.GetNamespace(), step.Resolving, b.csvLister, crd, currentCRD)
- _, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})
+ // Update CRD to new version
+ setInstalledAlongsideAnnotation(b.annotator, crd, b.plan.GetNamespace(), step.Resolving, b.csvLister, crd, currentCRD)
+ _, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})
+ if err != nil {
+ return fmt.Errorf("error updating CRD %q: %w", step.Resource.Name, err)
+ }
+ return nil
+ })
  if err != nil {
- return v1alpha1.StepStatusUnknown, errors.Wrapf(err, "error updating CRD: %s", step.Resource.Name)
+ return v1alpha1.StepStatusUnknown, err
  }
  // If it already existed, mark the step as Present.
  // they were equal - mark CRD as present