From c494f7e6b0132482494644ed7f3ce49cba7b8792 Mon Sep 17 00:00:00 2001 From: Joseph Kwasniewski Date: Wed, 27 Sep 2023 20:11:53 -0700 Subject: [PATCH] Move from `webhook.status` to `canary.status` Signed-off-by: Joseph Kwasniewski --- artifacts/flagger/crd.yaml | 15 ++++++----- charts/flagger/crds/crd.yaml | 15 ++++++----- kustomize/base/flagger/crd.yaml | 15 ++++++----- pkg/apis/flagger/v1beta1/canary.go | 3 --- pkg/apis/flagger/v1beta1/status.go | 4 ++- .../flagger/v1beta1/zz_generated.deepcopy.go | 8 +++++- pkg/canary/controller.go | 2 +- pkg/canary/daemonset_status.go | 6 ++--- pkg/canary/deployment_status.go | 6 ++--- pkg/canary/service_controller.go | 6 ++--- pkg/canary/status.go | 9 +++++-- pkg/controller/scheduler.go | 27 ++++++++++++------- pkg/controller/scheduler_deployment_test.go | 12 ++++----- pkg/controller/scheduler_test.go | 6 ++--- 14 files changed, 77 insertions(+), 57 deletions(-) diff --git a/artifacts/flagger/crd.yaml b/artifacts/flagger/crd.yaml index 8f50c6c1f..71b92db45 100644 --- a/artifacts/flagger/crd.yaml +++ b/artifacts/flagger/crd.yaml @@ -1086,13 +1086,6 @@ spec: type: object additionalProperties: type: string - status: - description: CanaryStatus defines the observed state of a canary. - type: object - properties: - retries: - description: Current number of retries the webhook has performed - type: number retries: description: Number of retries before incrementing canary failure count type: number @@ -1142,6 +1135,14 @@ spec: additionalProperties: type: string type: object + webhooks: + description: Webhook status of this canary + type: object + additionalProperties: + type: object + properties: + retries: + type: number lastAppliedSpec: description: LastAppliedSpec of this canary type: string diff --git a/charts/flagger/crds/crd.yaml b/charts/flagger/crds/crd.yaml index 8f50c6c1f..71b92db45 100644 --- a/charts/flagger/crds/crd.yaml +++ b/charts/flagger/crds/crd.yaml @@ -1086,13 +1086,6 @@ spec: type: object additionalProperties: type: string - status: - description: CanaryStatus defines the observed state of a canary. - type: object - properties: - retries: - description: Current number of retries the webhook has performed - type: number retries: description: Number of retries before incrementing canary failure count type: number @@ -1142,6 +1135,14 @@ spec: additionalProperties: type: string type: object + webhooks: + description: Webhook status of this canary + type: object + additionalProperties: + type: object + properties: + retries: + type: number lastAppliedSpec: description: LastAppliedSpec of this canary type: string diff --git a/kustomize/base/flagger/crd.yaml b/kustomize/base/flagger/crd.yaml index 8f50c6c1f..71b92db45 100644 --- a/kustomize/base/flagger/crd.yaml +++ b/kustomize/base/flagger/crd.yaml @@ -1086,13 +1086,6 @@ spec: type: object additionalProperties: type: string - status: - description: CanaryStatus defines the observed state of a canary. - type: object - properties: - retries: - description: Current number of retries the webhook has performed - type: number retries: description: Number of retries before incrementing canary failure count type: number @@ -1142,6 +1135,14 @@ spec: additionalProperties: type: string type: object + webhooks: + description: Webhook status of this canary + type: object + additionalProperties: + type: object + properties: + retries: + type: number lastAppliedSpec: description: LastAppliedSpec of this canary type: string diff --git a/pkg/apis/flagger/v1beta1/canary.go b/pkg/apis/flagger/v1beta1/canary.go index a37e52396..58a247567 100644 --- a/pkg/apis/flagger/v1beta1/canary.go +++ b/pkg/apis/flagger/v1beta1/canary.go @@ -391,9 +391,6 @@ type CanaryWebhook struct { // +optional Metadata *map[string]string `json:"metadata,omitempty"` - // Canary Webhook Status - Status CanaryWebhookStatus `json:"status"` - // Retries for failing webhook before incrementing rollout failure threshold // +optional Retries int `json:"retries,omitempty"` diff --git a/pkg/apis/flagger/v1beta1/status.go b/pkg/apis/flagger/v1beta1/status.go index 91814c517..03e169cd1 100644 --- a/pkg/apis/flagger/v1beta1/status.go +++ b/pkg/apis/flagger/v1beta1/status.go @@ -87,9 +87,11 @@ type CanaryStatus struct { LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty"` // +optional Conditions []CanaryCondition `json:"conditions,omitempty"` + // +optional + Webhooks map[string]CanaryWebhookStatus `json:"webhooks,omitempty"` } -// CanaryStatus is used for state persistence (read-only) +// CanaryWebhookStatus is a status condition for a Canary Webhook type CanaryWebhookStatus struct { Retries int `json:"retries"` } diff --git a/pkg/apis/flagger/v1beta1/zz_generated.deepcopy.go b/pkg/apis/flagger/v1beta1/zz_generated.deepcopy.go index f139ef06d..857c300d6 100644 --- a/pkg/apis/flagger/v1beta1/zz_generated.deepcopy.go +++ b/pkg/apis/flagger/v1beta1/zz_generated.deepcopy.go @@ -531,6 +531,13 @@ func (in *CanaryStatus) DeepCopyInto(out *CanaryStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.Webhooks != nil { + in, out := &in.Webhooks, &out.Webhooks + *out = make(map[string]CanaryWebhookStatus, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } return } @@ -584,7 +591,6 @@ func (in *CanaryWebhook) DeepCopyInto(out *CanaryWebhook) { } } } - out.Status = in.Status return } diff --git a/pkg/canary/controller.go b/pkg/canary/controller.go index 72edc5930..a5339a0db 100644 --- a/pkg/canary/controller.go +++ b/pkg/canary/controller.go @@ -29,7 +29,7 @@ type Controller interface { SetStatusWeight(canary *flaggerv1.Canary, val int) error SetStatusIterations(canary *flaggerv1.Canary, val int) error SetStatusPhase(canary *flaggerv1.Canary, phase flaggerv1.CanaryPhase) error - SetWebhookStatusRetries(canary *flaggerv1.Canary, webhook int, val int) error + SetStatusWebhookRetries(canary *flaggerv1.Canary, webhook string, val int) error Initialize(canary *flaggerv1.Canary) error Promote(canary *flaggerv1.Canary) error HasTargetChanged(canary *flaggerv1.Canary) (bool, error) diff --git a/pkg/canary/daemonset_status.go b/pkg/canary/daemonset_status.go index 55512290b..75facd4e1 100644 --- a/pkg/canary/daemonset_status.go +++ b/pkg/canary/daemonset_status.go @@ -72,7 +72,7 @@ func (c *DaemonSetController) SetStatusPhase(cd *flaggerv1.Canary, phase flagger return setStatusPhase(c.flaggerClient, cd, phase) } -// SetWebhookStatusRetries updates the webhook retries counter -func (c *DaemonSetController) SetWebhookStatusRetries(cd *flaggerv1.Canary, webhook int, val int) error { - return SetWebhookStatusRetries(c.flaggerClient, cd, webhook, val) +// SetStatusWebhookRetries updates the webhook retries counter +func (c *DaemonSetController) SetStatusWebhookRetries(cd *flaggerv1.Canary, webhook string, val int) error { + return SetStatusWebhookRetries(c.flaggerClient, cd, webhook, val) } diff --git a/pkg/canary/deployment_status.go b/pkg/canary/deployment_status.go index 2ec0a36ba..91f8df51b 100644 --- a/pkg/canary/deployment_status.go +++ b/pkg/canary/deployment_status.go @@ -62,7 +62,7 @@ func (c *DeploymentController) SetStatusPhase(cd *flaggerv1.Canary, phase flagge return setStatusPhase(c.flaggerClient, cd, phase) } -// SetWebhookStatusRetries updates the webhook retries counter -func (c *DeploymentController) SetWebhookStatusRetries(cd *flaggerv1.Canary, webhook int, val int) error { - return SetWebhookStatusRetries(c.flaggerClient, cd, webhook, val) +// SetStatusWebhookRetries updates the webhook retries counter +func (c *DeploymentController) SetStatusWebhookRetries(cd *flaggerv1.Canary, webhook string, val int) error { + return SetStatusWebhookRetries(c.flaggerClient, cd, webhook, val) } diff --git a/pkg/canary/service_controller.go b/pkg/canary/service_controller.go index fdd51dfa0..ea0e91601 100644 --- a/pkg/canary/service_controller.go +++ b/pkg/canary/service_controller.go @@ -60,9 +60,9 @@ func (c *ServiceController) SetStatusPhase(cd *flaggerv1.Canary, phase flaggerv1 return setStatusPhase(c.flaggerClient, cd, phase) } -// SetWebhookStatusRetries updates the webhook retries counter -func (c *ServiceController) SetWebhookStatusRetries(cd *flaggerv1.Canary, webhook int, val int) error { - return SetWebhookStatusRetries(c.flaggerClient, cd, webhook, val) +// SetStatusWebhookRetries updates the webhook retries counter +func (c *ServiceController) SetStatusWebhookRetries(cd *flaggerv1.Canary, webhook string, val int) error { + return SetStatusWebhookRetries(c.flaggerClient, cd, webhook, val) } // GetMetadata returns the pod label selector, label value and svc ports diff --git a/pkg/canary/status.go b/pkg/canary/status.go index d93d93eb7..b4b1fa499 100644 --- a/pkg/canary/status.go +++ b/pkg/canary/status.go @@ -47,6 +47,7 @@ func syncCanaryStatus(flaggerClient clientset.Interface, cd *flaggerv1.Canary, s cdCopy.Status.CanaryWeight = status.CanaryWeight cdCopy.Status.FailedChecks = status.FailedChecks cdCopy.Status.Iterations = status.Iterations + cdCopy.Status.Webhooks = status.Webhooks cdCopy.Status.LastAppliedSpec = hash if status.Phase == flaggerv1.CanaryPhaseInitialized { cdCopy.Status.LastPromotedSpec = hash @@ -185,7 +186,7 @@ func setStatusPhase(flaggerClient clientset.Interface, cd *flaggerv1.Canary, pha return nil } -func SetWebhookStatusRetries(flaggerClient clientset.Interface, cd *flaggerv1.Canary, webhook int, val int) error { +func SetStatusWebhookRetries(flaggerClient clientset.Interface, cd *flaggerv1.Canary, webhook string, val int) error { firstTry := true name, ns := cd.GetName(), cd.GetNamespace() err := retry.RetryOnConflict(retry.DefaultBackoff, func() (err error) { @@ -197,7 +198,11 @@ func SetWebhookStatusRetries(flaggerClient clientset.Interface, cd *flaggerv1.Ca } cdCopy := cd.DeepCopy() - cdCopy.GetAnalysis().Webhooks[webhook].Status.Retries = val + // initialize the map if it's nil + if cdCopy.Status.Webhooks == nil { + cdCopy.Status.Webhooks = make(map[string]flaggerv1.CanaryWebhookStatus) + } + cdCopy.Status.Webhooks[webhook] = flaggerv1.CanaryWebhookStatus{Retries: val} cdCopy.Status.LastTransitionTime = metav1.Now() err = updateStatusWithUpgrade(flaggerClient, cdCopy) diff --git a/pkg/controller/scheduler.go b/pkg/controller/scheduler.go index 2dac9c1e8..794aa47c4 100644 --- a/pkg/controller/scheduler.go +++ b/pkg/controller/scheduler.go @@ -735,25 +735,32 @@ func (c *Controller) runBlueGreen(canary *flaggerv1.Canary, canaryController can func (c *Controller) runAnalysisWebhooks(canary *flaggerv1.Canary, canaryController canary.Controller) string { // run external checks - for id, webhook := range canary.GetAnalysis().Webhooks { + var retry bool + var failure bool + for _, webhook := range canary.GetAnalysis().Webhooks { if webhook.Type == "" || webhook.Type == flaggerv1.RolloutHook { err := CallWebhook(canary.Name, canary.Namespace, flaggerv1.CanaryPhaseProgressing, webhook) if err != nil { - if webhook.Retries > 0 && webhook.Status.Retries < webhook.Retries { - if err := canaryController.SetWebhookStatusRetries(canary, id, webhook.Status.Retries+1); err != nil { + if webhook.Retries > 0 && canary.Status.Webhooks[webhook.Name].Retries < webhook.Retries { + if err := canaryController.SetStatusWebhookRetries(canary, webhook.Name, canary.Status.Webhooks[webhook.Name].Retries+1); err != nil { c.recordEventWarningf(canary, "%v", err) } - c.recordEventWarningf(canary, "Halt %s.%s advancement external check %s failed %v retrying %d more times", - canary.Name, canary.Namespace, webhook.Name, err, webhook.Retries-webhook.Status.Retries-1) - return "retry" + c.recordEventWarningf(canary, "Halt %s.%s advancement external check %s failed %v retries remaining %d", + canary.Name, canary.Namespace, webhook.Name, err, webhook.Retries-canary.Status.Webhooks[webhook.Name].Retries-1) + retry = true + } else { + c.recordEventWarningf(canary, "Halt %s.%s advancement external check %s failed %v", + canary.Name, canary.Namespace, webhook.Name, err) + failure = true } - c.recordEventWarningf(canary, "Halt %s.%s advancement external check %s failed %v", - canary.Name, canary.Namespace, webhook.Name, err) - return "failure" } } } - + if failure { + return "failure" + } else if retry { + return "retry" + } return "success" } diff --git a/pkg/controller/scheduler_deployment_test.go b/pkg/controller/scheduler_deployment_test.go index a6402d877..4e6cd9290 100644 --- a/pkg/controller/scheduler_deployment_test.go +++ b/pkg/controller/scheduler_deployment_test.go @@ -702,16 +702,16 @@ func TestScheduler_DeploymentWebhookRetry(t *testing.T) { // verify webhook retry counter increases mocks.ctrl.advanceCanary("podinfo", "default") // Failing URL 1, Success URL 0 - require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", 0, 1)) - require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", 1, 0)) + require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", "failing-url", 1)) + require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", "success-url", 0)) // Failing URL 2, Success URL 0 mocks.ctrl.advanceCanary("podinfo", "default") - require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", 0, 2)) - require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", 1, 0)) + require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", "failing-url", 2)) + require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", "success-url", 0)) // Failing URL 3, Success URL 0 mocks.ctrl.advanceCanary("podinfo", "default") - require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", 0, 3)) - require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", 1, 0)) + require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", "failing-url", 3)) + require.NoError(t, assertWebhookRetries(mocks.flaggerClient, "podinfo", "success-url", 0)) // update failed checks to max mocks.deployer.SyncStatus(mocks.canary, flaggerv1.CanaryStatus{Phase: flaggerv1.CanaryPhaseProgressing, FailedChecks: 10}) diff --git a/pkg/controller/scheduler_test.go b/pkg/controller/scheduler_test.go index 818bf4f1e..08426fb3c 100644 --- a/pkg/controller/scheduler_test.go +++ b/pkg/controller/scheduler_test.go @@ -60,14 +60,14 @@ func assertPhase(flaggerClient clientset.Interface, canary string, phase flagger return nil } -func assertWebhookRetries(flaggerClient clientset.Interface, canary string, webhook int, retries int) error { +func assertWebhookRetries(flaggerClient clientset.Interface, canary string, webhook string, retries int) error { c, err := flaggerClient.FlaggerV1beta1().Canaries("default").Get(context.TODO(), canary, metav1.GetOptions{}) if err != nil { return err } - if c.GetAnalysis().Webhooks[webhook].Status.Retries != retries { - return fmt.Errorf("got webhook status retries %d wanted %d", c.GetAnalysis().Webhooks[webhook].Status.Retries, retries) + if c.Status.Webhooks[webhook].Retries != retries { + return fmt.Errorf("got webhook status retries %d wanted %d %v", c.Status.Webhooks[webhook].Retries, retries, c.Status.Webhooks) } return nil