From bd5e34cc067445cdf4a109c85546b7b2ee63819a Mon Sep 17 00:00:00 2001 From: David Eads Date: Tue, 9 Jan 2024 14:56:28 -0500 Subject: [PATCH] when updating status, do a live GET after conflict --- .../config_observer_controller_test.go | 4 +++ .../dynamic_operator_client.go | 13 ++++++++ .../dynamic_staticpod_operator_client.go | 15 ++++------ .../management_state_controller_test.go | 4 +++ pkg/operator/status/status_controller_test.go | 4 +++ pkg/operator/v1helpers/helpers.go | 30 +++++++++++++++++-- pkg/operator/v1helpers/interfaces.go | 2 ++ pkg/operator/v1helpers/test_helpers.go | 11 +++++++ 8 files changed, 71 insertions(+), 12 deletions(-) diff --git a/pkg/operator/configobserver/config_observer_controller_test.go b/pkg/operator/configobserver/config_observer_controller_test.go index a03e169968..518346d2f1 100644 --- 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 index 7f8a0d80d1..c6ebf7b657 100644 --- 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 index e3c6316f12..a5d4216f6d 100644 --- 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 index 043bb85fb5..a7e49b8125 100644 --- 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 index 543246d0d6..300eccce23 100644 --- 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 index 22aedb18f5..b676ece19e 100644 --- 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 + numberOfAttempts := 0 err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { - _, oldStatus, resourceVersion, err := client.GetOperatorState() + defer func() { + numberOfAttempts++ + }() + var oldStatus *operatorv1.OperatorStatus + var resourceVersion string + var err error + + if numberOfAttempts < 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 + numberOfAttempts := 0 err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { - _, oldStatus, resourceVersion, err := client.GetStaticPodOperatorState() + defer func() { + numberOfAttempts++ + }() + var oldStatus *operatorv1.StaticPodOperatorStatus + var resourceVersion string + var err error + + if numberOfAttempts < 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 index d61d302946..1dd9c641a5 100644 --- 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 index 004adc2be7..7fa64719fe 100644 --- 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"))