Skip to content

Commit ebe8450

Browse files
joelanfordjoelanford
authored
installplans: retry crd updates on conflicts (#2293) (#2314)
Signed-off-by: Joe Lanford <[email protected]>
1 parent cfea59e commit ebe8450

File tree

2 files changed

+142
-125
lines changed

2 files changed

+142
-125
lines changed

‎pkg/controller/install/webhook.go

Lines changed: 64 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,17 @@ import (
55
"fmt"
66
"hash/fnv"
77

8-
"github.com/operator-framework/api/pkg/operators/v1alpha1"
9-
hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash"
10-
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
11-
128
log "github.com/sirupsen/logrus"
139
admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
1410
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
1511
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1612
"k8s.io/apimachinery/pkg/labels"
1713
"k8s.io/apimachinery/pkg/util/rand"
14+
"k8s.io/client-go/util/retry"
15+
16+
"github.com/operator-framework/api/pkg/operators/v1alpha1"
17+
hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash"
18+
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
1819
)
1920

2021
func ValidWebhookRules(rules []admissionregistrationv1.RuleWithOperations) error {
@@ -200,69 +201,74 @@ func (i *StrategyDeploymentInstaller) createOrUpdateConversionWebhook(caPEM []by
200201

201202
// iterate over all the ConversionCRDs
202203
for _, conversionCRD := range desc.ConversionCRDs {
203-
// Get existing CRD on cluster
204-
crd, err := i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{})
205-
if err != nil {
206-
return fmt.Errorf("Unable to get CRD %s specified in Conversion Webhook: %v", conversionCRD, err)
207-
}
204+
if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
205+
// Get existing CRD on cluster
206+
crd, err := i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Get(context.TODO(), conversionCRD, metav1.GetOptions{})
207+
if err != nil {
208+
return fmt.Errorf("Unable to get CRD %s specified in Conversion Webhook: %v", conversionCRD, err)
209+
}
208210

209-
// check if this CRD is an owned CRD
210-
foundCRD := false
211-
for _, ownedCRD := range csv.Spec.CustomResourceDefinitions.Owned {
212-
if ownedCRD.Name == conversionCRD {
213-
foundCRD = true
214-
break
211+
// check if this CRD is an owned CRD
212+
foundCRD := false
213+
for _, ownedCRD := range csv.Spec.CustomResourceDefinitions.Owned {
214+
if ownedCRD.Name == conversionCRD {
215+
foundCRD = true
216+
break
217+
}
218+
}
219+
if !foundCRD {
220+
return fmt.Errorf("CSV %s does not own CRD %s", csv.GetName(), conversionCRD)
215221
}
216-
}
217-
if !foundCRD {
218-
return fmt.Errorf("CSV %s does not own CRD %s", csv.GetName(), conversionCRD)
219-
}
220222

221-
// crd.Spec.Conversion.Strategy specifies how custom resources are converted between versions.
222-
// Allowed values are:
223-
// - None: The converter only change the apiVersion and would not touch any other field in the custom resource.
224-
// - Webhook: API Server will call to an external webhook to do the conversion. This requires crd.Spec.preserveUnknownFields to be false.
225-
// References:
226-
// - https://docs.openshift.com/container-platform/4.5/rest_api/extension_apis/customresourcedefinition-apiextensions-k8s-io-v1.html
227-
// - https://kubernetes.io/blog/2019/06/20/crd-structural-schema/#pruning-don-t-preserve-unknown-fields
228-
// By default the strategy is none
229-
// Reference:
230-
// - https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
231-
if crd.Spec.PreserveUnknownFields != false {
232-
return fmt.Errorf("crd.Spec.PreserveUnknownFields must be false to let API Server call webhook to do the conversion")
233-
}
223+
// crd.Spec.Conversion.Strategy specifies how custom resources are converted between versions.
224+
// Allowed values are:
225+
// - None: The converter only change the apiVersion and would not touch any other field in the custom resource.
226+
// - Webhook: API Server will call to an external webhook to do the conversion. This requires crd.Spec.preserveUnknownFields to be false.
227+
// References:
228+
// - https://docs.openshift.com/container-platform/4.5/rest_api/extension_apis/customresourcedefinition-apiextensions-k8s-io-v1.html
229+
// - https://kubernetes.io/blog/2019/06/20/crd-structural-schema/#pruning-don-t-preserve-unknown-fields
230+
// By default the strategy is none
231+
// Reference:
232+
// - https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
233+
if crd.Spec.PreserveUnknownFields != false {
234+
return fmt.Errorf("crd.Spec.PreserveUnknownFields must be false to let API Server call webhook to do the conversion")
235+
}
234236

235-
// Conversion WebhookClientConfig should not be set when Strategy is None
236-
// https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
237-
// Conversion WebhookClientConfig needs to be set when Strategy is None
238-
// https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#configure-customresourcedefinition-to-use-conversion-webhooks
237+
// Conversion WebhookClientConfig should not be set when Strategy is None
238+
// https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions
239+
// Conversion WebhookClientConfig needs to be set when Strategy is None
240+
// https://v1-15.docs.kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#configure-customresourcedefinition-to-use-conversion-webhooks
239241

240-
// use user defined path for CRD conversion webhook, else set default value
241-
conversionWebhookPath := "/"
242-
if desc.WebhookPath != nil {
243-
conversionWebhookPath = *desc.WebhookPath
244-
}
242+
// use user defined path for CRD conversion webhook, else set default value
243+
conversionWebhookPath := "/"
244+
if desc.WebhookPath != nil {
245+
conversionWebhookPath = *desc.WebhookPath
246+
}
245247

246-
// Override Name, Namespace, and CABundle
247-
crd.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{
248-
Strategy: "Webhook",
249-
Webhook: &apiextensionsv1.WebhookConversion{
250-
ClientConfig: &apiextensionsv1.WebhookClientConfig{
251-
Service: &apiextensionsv1.ServiceReference{
252-
Namespace: i.owner.GetNamespace(),
253-
Name: desc.DomainName() + "-service",
254-
Path: &conversionWebhookPath,
255-
Port: &desc.ContainerPort,
248+
// Override Name, Namespace, and CABundle
249+
crd.Spec.Conversion = &apiextensionsv1.CustomResourceConversion{
250+
Strategy: "Webhook",
251+
Webhook: &apiextensionsv1.WebhookConversion{
252+
ClientConfig: &apiextensionsv1.WebhookClientConfig{
253+
Service: &apiextensionsv1.ServiceReference{
254+
Namespace: i.owner.GetNamespace(),
255+
Name: desc.DomainName() + "-service",
256+
Path: &conversionWebhookPath,
257+
Port: &desc.ContainerPort,
258+
},
259+
CABundle: caPEM,
256260
},
257-
CABundle: caPEM,
261+
ConversionReviewVersions: desc.AdmissionReviewVersions,
258262
},
259-
ConversionReviewVersions: desc.AdmissionReviewVersions,
260-
},
261-
}
263+
}
262264

263-
// update CRD conversion Specs
264-
if _, err = i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); err != nil {
265-
return fmt.Errorf("Error updating CRD with Conversion info: %v", err)
265+
// update CRD conversion Specs
266+
if _, err = i.strategyClient.GetOpClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{}); err != nil {
267+
return fmt.Errorf("Error updating CRD with Conversion info: %w", err)
268+
}
269+
return nil
270+
}); err != nil {
271+
return err
266272
}
267273
}
268274

‎pkg/controller/operators/catalog/step.go

Lines changed: 78 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -4,22 +4,21 @@ import (
44
"context"
55
"fmt"
66

7-
"k8s.io/client-go/dynamic"
8-
"k8s.io/client-go/tools/cache"
9-
10-
"github.com/operator-framework/api/pkg/operators/v1alpha1"
11-
crdlib "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/crd"
12-
index "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/index"
13-
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
14-
errorwrap "github.com/pkg/errors"
157
logger "github.com/sirupsen/logrus"
168
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
179
apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
1810
apiextensionsv1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1"
1911
apiextensionsv1beta1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1beta1"
20-
2112
k8serrors "k8s.io/apimachinery/pkg/api/errors"
2213
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
14+
"k8s.io/client-go/dynamic"
15+
"k8s.io/client-go/tools/cache"
16+
"k8s.io/client-go/util/retry"
17+
18+
"github.com/operator-framework/api/pkg/operators/v1alpha1"
19+
crdlib "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/crd"
20+
index "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/index"
21+
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
2322
)
2423

2524
// Stepper manages cluster interactions based on the step.
@@ -99,7 +98,7 @@ func (b *builder) NewCRDV1Step(client apiextensionsv1client.ApiextensionsV1Inter
9998
if k8serrors.IsNotFound(err) {
10099
return v1alpha1.StepStatusNotPresent, nil
101100
} else {
102-
return v1alpha1.StepStatusNotPresent, errorwrap.Wrapf(err, "error finding the %s CRD", crd.Name)
101+
return v1alpha1.StepStatusNotPresent, fmt.Errorf("error finding the %q CRD: %w", crd.Name, err)
103102
}
104103
}
105104
established, namesAccepted := false, false
@@ -126,39 +125,45 @@ func (b *builder) NewCRDV1Step(client apiextensionsv1client.ApiextensionsV1Inter
126125

127126
_, createError := client.CustomResourceDefinitions().Create(context.TODO(), crd, metav1.CreateOptions{})
128127
if k8serrors.IsAlreadyExists(createError) {
129-
currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{})
130-
// Verify CRD ownership, only attempt to update if
131-
// CRD has only one owner
132-
// Example: provided=database.coreos.com/v1alpha1/EtcdCluster
133-
matchedCSV, err := index.V1CRDProviderNames(b.csvToProvidedAPIs, crd)
134-
if err != nil {
135-
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "error find matched CSV: %s", step.Resource.Name)
136-
}
137-
crd.SetResourceVersion(currentCRD.GetResourceVersion())
138-
if len(matchedCSV) == 1 {
139-
logger.Debugf("Found one owner for CRD %v", crd)
140-
} else if len(matchedCSV) > 1 {
141-
logger.Debugf("Found multiple owners for CRD %v", crd)
128+
err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
129+
currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{})
130+
// Verify CRD ownership, only attempt to update if
131+
// CRD has only one owner
132+
// Example: provided=database.coreos.com/v1alpha1/EtcdCluster
133+
matchedCSV, err := index.V1CRDProviderNames(b.csvToProvidedAPIs, crd)
134+
if err != nil {
135+
return fmt.Errorf("error finding matched CSV %q: %w", step.Resource.Name, err)
136+
}
137+
crd.SetResourceVersion(currentCRD.GetResourceVersion())
138+
if len(matchedCSV) == 1 {
139+
logger.Debugf("Found one owner for CRD %v", crd)
140+
} else if len(matchedCSV) > 1 {
141+
logger.Debugf("Found multiple owners for CRD %v", crd)
142142

143-
if err = validateV1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil {
144-
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "error validating existing CRs against new CRD's schema: %s", step.Resource.Name)
143+
if err = validateV1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil {
144+
return fmt.Errorf("error validating existing CRs against new CRD's schema for %q: %w", step.Resource.Name, err)
145+
}
145146
}
146-
}
147147

148-
// check to see if stored versions changed and whether the upgrade could cause potential data loss
149-
safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
150-
if !safe {
151-
b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err)
152-
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "risk of data loss updating %s", step.Resource.Name)
153-
}
154-
if err != nil {
155-
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "checking CRD for potential data loss updating %s", step.Resource.Name)
156-
}
148+
// check to see if stored versions changed and whether the upgrade could cause potential data loss
149+
safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
150+
if !safe {
151+
b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err)
152+
return fmt.Errorf("risk of data loss updating %q: %w", step.Resource.Name, err)
153+
}
154+
if err != nil {
155+
return fmt.Errorf("checking CRD for potential data loss updating %q: %w", step.Resource.Name, err)
156+
}
157157

158-
// Update CRD to new version
159-
_, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})
158+
// Update CRD to new version
159+
_, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})
160+
if err != nil {
161+
return fmt.Errorf("error updating CRD %q: %w", step.Resource.Name, err)
162+
}
163+
return nil
164+
})
160165
if err != nil {
161-
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "error updating CRD: %s", step.Resource.Name)
166+
return v1alpha1.StepStatusUnknown, err
162167
}
163168
// If it already existed, mark the step as Present.
164169
// they were equal - mark CRD as present
@@ -187,7 +192,7 @@ func (b *builder) NewCRDV1Beta1Step(client apiextensionsv1beta1client.Apiextensi
187192
if k8serrors.IsNotFound(err) {
188193
return v1alpha1.StepStatusNotPresent, nil
189194
} else {
190-
return v1alpha1.StepStatusNotPresent, errorwrap.Wrapf(err, "error finding the %s CRD", crd.Name)
195+
return v1alpha1.StepStatusNotPresent, fmt.Errorf("error finding the %q CRD: %w", crd.Name, err)
191196
}
192197
}
193198
established, namesAccepted := false, false
@@ -214,39 +219,45 @@ func (b *builder) NewCRDV1Beta1Step(client apiextensionsv1beta1client.Apiextensi
214219

215220
_, createError := client.CustomResourceDefinitions().Create(context.TODO(), crd, metav1.CreateOptions{})
216221
if k8serrors.IsAlreadyExists(createError) {
217-
currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{})
218-
// Verify CRD ownership, only attempt to update if
219-
// CRD has only one owner
220-
// Example: provided=database.coreos.com/v1alpha1/EtcdCluster
221-
matchedCSV, err := index.V1Beta1CRDProviderNames(b.csvToProvidedAPIs, crd)
222-
if err != nil {
223-
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "error find matched CSV: %s", step.Resource.Name)
224-
}
225-
crd.SetResourceVersion(currentCRD.GetResourceVersion())
226-
if len(matchedCSV) == 1 {
227-
logger.Debugf("Found one owner for CRD %v", crd)
228-
} else if len(matchedCSV) > 1 {
229-
logger.Debugf("Found multiple owners for CRD %v", crd)
222+
err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
223+
currentCRD, _ := client.CustomResourceDefinitions().Get(context.TODO(), crd.GetName(), metav1.GetOptions{})
224+
// Verify CRD ownership, only attempt to update if
225+
// CRD has only one owner
226+
// Example: provided=database.coreos.com/v1alpha1/EtcdCluster
227+
matchedCSV, err := index.V1Beta1CRDProviderNames(b.csvToProvidedAPIs, crd)
228+
if err != nil {
229+
return fmt.Errorf("error finding matched CSV %q: %w", step.Resource.Name, err)
230+
}
231+
crd.SetResourceVersion(currentCRD.GetResourceVersion())
232+
if len(matchedCSV) == 1 {
233+
logger.Debugf("Found one owner for CRD %v", crd)
234+
} else if len(matchedCSV) > 1 {
235+
logger.Debugf("Found multiple owners for CRD %v", crd)
230236

231-
if err = validateV1Beta1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil {
232-
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "error validating existing CRs against new CRD's schema: %s", step.Resource.Name)
237+
if err = validateV1Beta1CRDCompatibility(b.dynamicClient, currentCRD, crd); err != nil {
238+
return fmt.Errorf("error validating existing CRs against new CRD's schema for %q: %w", step.Resource.Name, err)
239+
}
233240
}
234-
}
235241

236-
// check to see if stored versions changed and whether the upgrade could cause potential data loss
237-
safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
238-
if !safe {
239-
b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err)
240-
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "risk of data loss updating %s", step.Resource.Name)
241-
}
242-
if err != nil {
243-
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "checking CRD for potential data loss updating %s", step.Resource.Name)
244-
}
242+
// check to see if stored versions changed and whether the upgrade could cause potential data loss
243+
safe, err := crdlib.SafeStorageVersionUpgrade(currentCRD, crd)
244+
if !safe {
245+
b.logger.Errorf("risk of data loss updating %s: %s", step.Resource.Name, err)
246+
return fmt.Errorf("risk of data loss updating %q: %w", step.Resource.Name, err)
247+
}
248+
if err != nil {
249+
return fmt.Errorf("checking CRD for potential data loss updating %q: %w", step.Resource.Name, err)
250+
}
245251

246-
// Update CRD to new version
247-
_, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})
252+
// Update CRD to new version
253+
_, err = client.CustomResourceDefinitions().Update(context.TODO(), crd, metav1.UpdateOptions{})
254+
if err != nil {
255+
return fmt.Errorf("error updating CRD %q: %w", step.Resource.Name, err)
256+
}
257+
return nil
258+
})
248259
if err != nil {
249-
return v1alpha1.StepStatusUnknown, errorwrap.Wrapf(err, "error updating CRD: %s", step.Resource.Name)
260+
return v1alpha1.StepStatusUnknown, err
250261
}
251262
// If it already existed, mark the step as Present.
252263
// they were equal - mark CRD as present

0 commit comments

Comments
 (0)