when updating status, do a live GET after conflict

openshift · Jan 9, 2024 · a1945b6 · a1945b6
1 parent 5674ec6
commit a1945b6
Show file tree

Hide file tree

Showing 8 changed files with 71 additions and 12 deletions.
diff --git a/pkg/operator/configobserver/config_observer_controller_test.go b/pkg/operator/configobserver/config_observer_controller_test.go
@@ -43,6 +43,10 @@ func (c *fakeOperatorClient) GetOperatorState() (spec *operatorv1.OperatorSpec,
 	return c.startingSpec, &operatorv1.OperatorStatus{}, "", nil
 }
 
+func (c *fakeOperatorClient) GetOperatorStateWithQuorum(ctx context.Context) (spec *operatorv1.OperatorSpec, status *operatorv1.OperatorStatus, resourceVersion string, err error) {
+	return c.GetOperatorState()
+}
+
 func (c *fakeOperatorClient) UpdateOperatorSpec(ctx context.Context, rv string, in *operatorv1.OperatorSpec) (spec *operatorv1.OperatorSpec, resourceVersion string, err error) {
 	if c.specUpdateFailure != nil {
 		return &operatorv1.OperatorSpec{}, rv, c.specUpdateFailure

diff --git a/pkg/operator/genericoperatorclient/dynamic_operator_client.go b/pkg/operator/genericoperatorclient/dynamic_operator_client.go
@@ -89,6 +89,19 @@ func (c dynamicOperatorClient) GetOperatorState() (*operatorv1.OperatorSpec, *op
 	}
 	instance := uncastInstance.(*unstructured.Unstructured)
 
+	return getOperatorStateFromInstance(instance)
+}
+
+func (c dynamicOperatorClient) GetOperatorStateWithQuorum(ctx context.Context) (*operatorv1.OperatorSpec, *operatorv1.OperatorStatus, string, error) {
+	instance, err := c.client.Get(ctx, c.configName, metav1.GetOptions{})
+	if err != nil {
+		return nil, nil, "", err
+	}
+
+	return getOperatorStateFromInstance(instance)
+}
+
+func getOperatorStateFromInstance(instance *unstructured.Unstructured) (*operatorv1.OperatorSpec, *operatorv1.OperatorStatus, string, error) {
 	spec, err := getOperatorSpecFromUnstructured(instance.UnstructuredContent())
 	if err != nil {
 		return nil, nil, "", err

diff --git a/pkg/operator/genericoperatorclient/dynamic_staticpod_operator_client.go b/pkg/operator/genericoperatorclient/dynamic_staticpod_operator_client.go
@@ -48,6 +48,10 @@ func (c dynamicStaticPodOperatorClient) GetStaticPodOperatorState() (*operatorv1
 	}
 	instance := uncastInstance.(*unstructured.Unstructured)
 
+	return getStaticPodOperatorStateFromInstance(instance)
+}
+
+func getStaticPodOperatorStateFromInstance(instance *unstructured.Unstructured) (*operatorv1.StaticPodOperatorSpec, *operatorv1.StaticPodOperatorStatus, string, error) {
 	spec, err := getStaticPodOperatorSpecFromUnstructured(instance.UnstructuredContent())
 	if err != nil {
 		return nil, nil, "", err
@@ -66,16 +70,7 @@ func (c dynamicStaticPodOperatorClient) GetStaticPodOperatorStateWithQuorum(ctx
 		return nil, nil, "", err
 	}
 
-	spec, err := getStaticPodOperatorSpecFromUnstructured(instance.UnstructuredContent())
-	if err != nil {
-		return nil, nil, "", err
-	}
-	status, err := getStaticPodOperatorStatusFromUnstructured(instance.UnstructuredContent())
-	if err != nil {
-		return nil, nil, "", err
-	}
-
-	return spec, status, instance.GetResourceVersion(), nil
+	return getStaticPodOperatorStateFromInstance(instance)
 }
 
 func (c dynamicStaticPodOperatorClient) UpdateStaticPodOperatorSpec(ctx context.Context, resourceVersion string, spec *operatorv1.StaticPodOperatorSpec) (*operatorv1.StaticPodOperatorSpec, string, error) {

diff --git a/pkg/operator/managementstatecontroller/management_state_controller_test.go b/pkg/operator/managementstatecontroller/management_state_controller_test.go
@@ -135,6 +135,10 @@ func (c *statusClient) GetOperatorState() (*operatorv1.OperatorSpec, *operatorv1
 	return &c.spec, &c.status, "", nil
 }
 
+func (c *statusClient) GetOperatorStateWithQuorum(ctx context.Context) (*operatorv1.OperatorSpec, *operatorv1.OperatorStatus, string, error) {
+	return c.GetOperatorState()
+}
+
 func (c *statusClient) UpdateOperatorSpec(context.Context, string, *operatorv1.OperatorSpec) (spec *operatorv1.OperatorSpec, resourceVersion string, err error) {
 	panic("missing")
 }

diff --git a/pkg/operator/status/status_controller_test.go b/pkg/operator/status/status_controller_test.go
@@ -607,6 +607,10 @@ func (c *statusClient) GetOperatorState() (*operatorv1.OperatorSpec, *operatorv1
 	return &c.spec, &c.status, "", nil
 }
 
+func (c *statusClient) GetOperatorStateWithQuorum(ctx context.Context) (*operatorv1.OperatorSpec, *operatorv1.OperatorStatus, string, error) {
+	return c.GetOperatorState()
+}
+
 func (c *statusClient) UpdateOperatorSpec(context.Context, string, *operatorv1.OperatorSpec) (spec *operatorv1.OperatorSpec, resourceVersion string, err error) {
 	panic("missing")
 }

diff --git a/pkg/operator/v1helpers/helpers.go b/pkg/operator/v1helpers/helpers.go
@@ -159,8 +159,21 @@ type UpdateStatusFunc func(status *operatorv1.OperatorStatus) error
 func UpdateStatus(ctx context.Context, client OperatorClient, updateFuncs ...UpdateStatusFunc) (*operatorv1.OperatorStatus, bool, error) {
 	updated := false
 	var updatedOperatorStatus *operatorv1.OperatorStatus
+	numberOfFailedAttempts := 0
 	err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
-		_, oldStatus, resourceVersion, err := client.GetOperatorState()
+		defer func() {
+			numberOfFailedAttempts++
+		}()
+		var oldStatus *operatorv1.OperatorStatus
+		var resourceVersion string
+		var err error
+
+		if numberOfFailedAttempts < 1 { // prefer lister if we haven't already failed.
+			_, oldStatus, resourceVersion, err = client.GetOperatorState()
+
+		} else { // if we have failed enough times (chose 1 as a starting point, do a live GET
+			_, oldStatus, resourceVersion, err = client.GetOperatorStateWithQuorum(ctx)
+		}
 		if err != nil {
 			return err
 		}
@@ -201,8 +214,21 @@ type UpdateStaticPodStatusFunc func(status *operatorv1.StaticPodOperatorStatus)
 func UpdateStaticPodStatus(ctx context.Context, client StaticPodOperatorClient, updateFuncs ...UpdateStaticPodStatusFunc) (*operatorv1.StaticPodOperatorStatus, bool, error) {
 	updated := false
 	var updatedOperatorStatus *operatorv1.StaticPodOperatorStatus
+	numberOfFailedAttempts := 0
 	err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
-		_, oldStatus, resourceVersion, err := client.GetStaticPodOperatorState()
+		defer func() {
+			numberOfFailedAttempts++
+		}()
+		var oldStatus *operatorv1.StaticPodOperatorStatus
+		var resourceVersion string
+		var err error
+
+		if numberOfFailedAttempts < 1 { // prefer lister if we haven't already failed.
+			_, oldStatus, resourceVersion, err = client.GetStaticPodOperatorState()
+
+		} else { // if we have failed enough times (chose 1 as a starting point, do a live GET
+			_, oldStatus, resourceVersion, err = client.GetStaticPodOperatorStateWithQuorum(ctx)
+		}
 		if err != nil {
 			return err
 		}

diff --git a/pkg/operator/v1helpers/interfaces.go b/pkg/operator/v1helpers/interfaces.go
@@ -14,6 +14,8 @@ type OperatorClient interface {
 	GetObjectMeta() (meta *metav1.ObjectMeta, err error)
 	// GetOperatorState returns the operator spec, status and the resource version, potentially from a lister.
 	GetOperatorState() (spec *operatorv1.OperatorSpec, status *operatorv1.OperatorStatus, resourceVersion string, err error)
+	// GetOperatorStateWithQuorum return the operator spec, status and resource version directly from a server read.
+	GetOperatorStateWithQuorum(ctx context.Context) (spec *operatorv1.OperatorSpec, status *operatorv1.OperatorStatus, resourceVersion string, err error)
 	// UpdateOperatorSpec updates the spec of the operator, assuming the given resource version.
 	UpdateOperatorSpec(ctx context.Context, oldResourceVersion string, in *operatorv1.OperatorSpec) (out *operatorv1.OperatorSpec, newResourceVersion string, err error)
 	// UpdateOperatorStatus updates the status of the operator, assuming the given resource version.

diff --git a/pkg/operator/v1helpers/test_helpers.go b/pkg/operator/v1helpers/test_helpers.go
@@ -111,6 +111,10 @@ func (c *fakeStaticPodOperatorClient) GetStaticPodOperatorState() (*operatorv1.S
 	return c.fakeStaticPodOperatorSpec, c.fakeStaticPodOperatorStatus, c.resourceVersion, nil
 }
 
+func (c *fakeStaticPodOperatorClient) GetLiveStaticPodOperatorState() (*operatorv1.StaticPodOperatorSpec, *operatorv1.StaticPodOperatorStatus, string, error) {
+	return c.GetStaticPodOperatorState()
+}
+
 func (c *fakeStaticPodOperatorClient) GetStaticPodOperatorStateWithQuorum(ctx context.Context) (*operatorv1.StaticPodOperatorSpec, *operatorv1.StaticPodOperatorStatus, string, error) {
 	return c.fakeStaticPodOperatorSpec, c.fakeStaticPodOperatorStatus, c.resourceVersion, nil
 }
@@ -154,6 +158,9 @@ func (c *fakeStaticPodOperatorClient) UpdateStaticPodOperatorSpec(ctx context.Co
 func (c *fakeStaticPodOperatorClient) GetOperatorState() (*operatorv1.OperatorSpec, *operatorv1.OperatorStatus, string, error) {
 	return &c.fakeStaticPodOperatorSpec.OperatorSpec, &c.fakeStaticPodOperatorStatus.OperatorStatus, c.resourceVersion, nil
 }
+func (c *fakeStaticPodOperatorClient) GetOperatorStateWithQuorum(ctx context.Context) (*operatorv1.OperatorSpec, *operatorv1.OperatorStatus, string, error) {
+	return c.GetOperatorState()
+}
 func (c *fakeStaticPodOperatorClient) UpdateOperatorSpec(ctx context.Context, s string, p *operatorv1.OperatorSpec) (spec *operatorv1.OperatorSpec, resourceVersion string, err error) {
 	panic("not supported")
 }
@@ -241,6 +248,10 @@ func (c *fakeOperatorClient) GetOperatorState() (*operatorv1.OperatorSpec, *oper
 	return c.fakeOperatorSpec, c.fakeOperatorStatus, c.resourceVersion, nil
 }
 
+func (c *fakeOperatorClient) GetOperatorStateWithQuorum(ctx context.Context) (*operatorv1.OperatorSpec, *operatorv1.OperatorStatus, string, error) {
+	return c.GetOperatorState()
+}
+
 func (c *fakeOperatorClient) UpdateOperatorStatus(ctx context.Context, resourceVersion string, status *operatorv1.OperatorStatus) (*operatorv1.OperatorStatus, error) {
 	if c.resourceVersion != resourceVersion {
 		return nil, errors.NewConflict(schema.GroupResource{Group: operatorv1.GroupName, Resource: "TestOperatorConfig"}, "instance", fmt.Errorf("invalid resourceVersion"))