From f4358f8dfbc16bef506cc56b70627f45fbceaf14 Mon Sep 17 00:00:00 2001 From: Jesse Suen Date: Mon, 9 Nov 2020 15:02:29 -0800 Subject: [PATCH] feat(controller): use CRD status subresource (#789) Signed-off-by: Jesse Suen --- hack/update-manifests.sh | 4 +- manifests/cluster-install/kustomization.yaml | 5 +- manifests/crds/rollout-crd.yaml | 3 +- manifests/install.yaml | 144 +------------- manifests/namespace-install.yaml | 179 +++++++++--------- .../clusterrole-to-role.yaml | 3 + .../namespace-install/kustomization.yaml | 4 +- .../argo-rollouts-clusterrole.yaml | 0 manifests/role/kustomization.yaml | 5 + .../rollouts/v1alpha1/openapi_generated.go | 7 - pkg/apis/rollouts/v1alpha1/types.go | 5 +- .../rollouts/v1alpha1/fake/fake_rollout.go | 12 ++ .../typed/rollouts/v1alpha1/rollout.go | 17 ++ pkg/kubectl-argo-rollouts/cmd/abort/abort.go | 8 +- pkg/kubectl-argo-rollouts/cmd/get/get_test.go | 3 + .../cmd/promote/promote.go | 64 +++++-- pkg/kubectl-argo-rollouts/cmd/retry/retry.go | 8 +- pkg/kubectl-argo-rollouts/info/info_test.go | 67 +++++-- .../info/rollout_info.go | 30 ++- .../blue-green/bluegreen-rollout.yaml | 2 +- .../info/testdata/canary/canary-rollout.yaml | 2 +- .../rollout-experiment-analysis.yaml | 2 +- .../testdata/experiment-step/canary-demo.yaml | 2 +- .../testdata/rollout-aborted/rollout.yaml | 3 +- .../rollout-invalid/rollout-invalid.yaml | 2 +- rollout/analysis_test.go | 15 +- rollout/bluegreen_test.go | 5 +- rollout/canary.go | 8 - rollout/canary_test.go | 49 ++--- rollout/controller.go | 35 +--- rollout/controller_test.go | 69 ++----- rollout/experiment_test.go | 3 +- rollout/sync.go | 55 ++++-- test/e2e/analysis_test.go | 3 +- test/fixtures/then.go | 2 +- test/fixtures/when.go | 26 +-- utils/conditions/conditions.go | 12 +- utils/conditions/rollouts_test.go | 27 +-- utils/replicaset/canary_test.go | 4 +- 39 files changed, 392 insertions(+), 502 deletions(-) rename manifests/{cluster-install => role}/argo-rollouts-clusterrole.yaml (100%) create mode 100644 manifests/role/kustomization.yaml diff --git a/hack/update-manifests.sh b/hack/update-manifests.sh index 5b65ff1d27..fe781f6ee5 100755 --- a/hack/update-manifests.sh +++ b/hack/update-manifests.sh @@ -17,9 +17,9 @@ if [ ! -z "${IMAGE_TAG}" ]; then fi echo "${AUTOGENMSG}" > "${SRCROOT}/manifests/install.yaml" -kustomize build --load_restrictor none "${SRCROOT}/manifests/cluster-install" >> "${SRCROOT}/manifests/install.yaml" +kubectl kustomize "${SRCROOT}/manifests/cluster-install" >> "${SRCROOT}/manifests/install.yaml" update_image "${SRCROOT}/manifests/install.yaml" echo "${AUTOGENMSG}" > "${SRCROOT}/manifests/namespace-install.yaml" -kustomize build --load_restrictor none "${SRCROOT}/manifests/namespace-install" >> "${SRCROOT}/manifests/namespace-install.yaml" +kubectl kustomize "${SRCROOT}/manifests/namespace-install" >> "${SRCROOT}/manifests/namespace-install.yaml" update_image "${SRCROOT}/manifests/namespace-install.yaml" diff --git a/manifests/cluster-install/kustomization.yaml b/manifests/cluster-install/kustomization.yaml index afc056591d..906938decb 100644 --- a/manifests/cluster-install/kustomization.yaml +++ b/manifests/cluster-install/kustomization.yaml @@ -2,8 +2,9 @@ apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization bases: -- ../namespace-install +- ../crds +- ../base +- ../role resources: -- argo-rollouts-clusterrole.yaml - argo-rollouts-clusterrolebinding.yaml diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index a9a9dca660..f28901e5d7 100644 --- a/manifests/crds/rollout-crd.yaml +++ b/manifests/crds/rollout-crd.yaml @@ -36,6 +36,7 @@ spec: labelSelectorPath: .status.selector specReplicasPath: .spec.replicas statusReplicasPath: .status.HPAReplicas + status: {} validation: openAPIV3Schema: properties: @@ -3059,8 +3060,6 @@ spec: - name - status type: object - stableRS: - type: string type: object collisionCount: format: int32 diff --git a/manifests/install.yaml b/manifests/install.yaml index bdbb6c0577..aa6e22e48c 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -11057,11 +11057,13 @@ spec: name: Current type: integer - JSONPath: .status.updatedReplicas - description: Total number of non-terminated pods targeted by this rollout that have the desired template spec + description: Total number of non-terminated pods targeted by this rollout that + have the desired template spec name: Up-to-date type: integer - JSONPath: .status.availableReplicas - description: Total number of available pods (ready for at least minReadySeconds) targeted by this rollout + description: Total number of available pods (ready for at least minReadySeconds) + targeted by this rollout name: Available type: integer group: argoproj.io @@ -11078,6 +11080,7 @@ spec: labelSelectorPath: .status.selector specReplicasPath: .spec.replicas statusReplicasPath: .status.HPAReplicas + status: {} validation: openAPIV3Schema: properties: @@ -14101,8 +14104,6 @@ spec: - name - status type: object - stableRS: - type: string type: object collisionCount: format: int32 @@ -14193,137 +14194,6 @@ metadata: name: argo-rollouts --- apiVersion: rbac.authorization.k8s.io/v1 -kind: Role -metadata: - labels: - app.kubernetes.io/component: rollouts-controller - app.kubernetes.io/name: argo-rollouts-clusterrole - app.kubernetes.io/part-of: argo-rollouts - name: argo-rollouts-role -rules: -- apiGroups: - - argoproj.io - resources: - - rollouts - - rollouts/finalizers - verbs: - - get - - list - - watch - - update - - patch -- apiGroups: - - argoproj.io - resources: - - analysisruns - - analysisruns/finalizers - - experiments - - experiments/finalizers - verbs: - - create - - get - - list - - watch - - update - - patch - - delete -- apiGroups: - - argoproj.io - resources: - - analysistemplates - - clusteranalysistemplates - verbs: - - get - - list - - watch -- apiGroups: - - apps - resources: - - replicasets - verbs: - - create - - get - - list - - watch - - update - - patch - - delete -- apiGroups: - - "" - resources: - - services - verbs: - - get - - list - - watch - - patch -- apiGroups: - - "" - resources: - - secrets - verbs: - - get - - list - - watch -- apiGroups: - - "" - resources: - - pods - verbs: - - list - - delete - - update -- apiGroups: - - "" - resources: - - events - verbs: - - create - - update - - patch -- apiGroups: - - extensions - resources: - - ingresses - verbs: - - create - - get - - list - - watch - - patch -- apiGroups: - - batch - resources: - - jobs - verbs: - - create - - get - - list - - watch - - update - - patch - - delete -- apiGroups: - - networking.istio.io - resources: - - virtualservices - verbs: - - watch - - get - - update - - list -- apiGroups: - - split.smi-spec.io - resources: - - trafficsplits - verbs: - - create - - watch - - get - - update - - patch ---- -apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: labels: @@ -14592,9 +14462,7 @@ spec: app.kubernetes.io/name: argo-rollouts spec: containers: - - args: - - --namespaced - image: argoproj/argo-rollouts:latest + - image: argoproj/argo-rollouts:latest imagePullPolicy: Always name: argo-rollouts securityContext: diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 822c653c8d..ef585d74e6 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -11057,11 +11057,13 @@ spec: name: Current type: integer - JSONPath: .status.updatedReplicas - description: Total number of non-terminated pods targeted by this rollout that have the desired template spec + description: Total number of non-terminated pods targeted by this rollout that + have the desired template spec name: Up-to-date type: integer - JSONPath: .status.availableReplicas - description: Total number of available pods (ready for at least minReadySeconds) targeted by this rollout + description: Total number of available pods (ready for at least minReadySeconds) + targeted by this rollout name: Available type: integer group: argoproj.io @@ -11078,6 +11080,7 @@ spec: labelSelectorPath: .status.selector specReplicasPath: .spec.replicas statusReplicasPath: .status.HPAReplicas + status: {} validation: openAPIV3Schema: properties: @@ -14101,8 +14104,6 @@ spec: - name - status type: object - stableRS: - type: string type: object collisionCount: format: int32 @@ -14193,11 +14194,95 @@ metadata: name: argo-rollouts --- apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + labels: + app.kubernetes.io/component: aggregate-cluster-role + app.kubernetes.io/name: argo-rollouts-aggregate-to-admin + app.kubernetes.io/part-of: argo-rollouts + rbac.authorization.k8s.io/aggregate-to-admin: "true" + name: argo-rollouts-aggregate-to-admin +rules: +- apiGroups: + - argoproj.io + resources: + - rollouts + - rollouts/scale + - rollouts/status + - experiments + - analysistemplates + - clusteranalysistemplates + - analysisruns + verbs: + - create + - delete + - deletecollection + - get + - list + - patch + - update + - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + labels: + app.kubernetes.io/component: aggregate-cluster-role + app.kubernetes.io/name: argo-rollouts-aggregate-to-edit + app.kubernetes.io/part-of: argo-rollouts + rbac.authorization.k8s.io/aggregate-to-edit: "true" + name: argo-rollouts-aggregate-to-edit +rules: +- apiGroups: + - argoproj.io + resources: + - rollouts + - rollouts/scale + - rollouts/status + - experiments + - analysistemplates + - clusteranalysistemplates + - analysisruns + verbs: + - create + - delete + - deletecollection + - get + - list + - patch + - update + - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + labels: + app.kubernetes.io/component: aggregate-cluster-role + app.kubernetes.io/name: argo-rollouts-aggregate-to-view + app.kubernetes.io/part-of: argo-rollouts + rbac.authorization.k8s.io/aggregate-to-view: "true" + name: argo-rollouts-aggregate-to-view +rules: +- apiGroups: + - argoproj.io + resources: + - rollouts + - rollouts/scale + - experiments + - analysistemplates + - clusteranalysistemplates + - analysisruns + verbs: + - get + - list + - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 kind: Role metadata: labels: app.kubernetes.io/component: rollouts-controller - app.kubernetes.io/name: argo-rollouts-clusterrole + app.kubernetes.io/name: argo-rollouts-role app.kubernetes.io/part-of: argo-rollouts name: argo-rollouts-role rules: @@ -14323,90 +14408,6 @@ rules: - update - patch --- -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - labels: - app.kubernetes.io/component: aggregate-cluster-role - app.kubernetes.io/name: argo-rollouts-aggregate-to-admin - app.kubernetes.io/part-of: argo-rollouts - rbac.authorization.k8s.io/aggregate-to-admin: "true" - name: argo-rollouts-aggregate-to-admin -rules: -- apiGroups: - - argoproj.io - resources: - - rollouts - - rollouts/scale - - rollouts/status - - experiments - - analysistemplates - - clusteranalysistemplates - - analysisruns - verbs: - - create - - delete - - deletecollection - - get - - list - - patch - - update - - watch ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - labels: - app.kubernetes.io/component: aggregate-cluster-role - app.kubernetes.io/name: argo-rollouts-aggregate-to-edit - app.kubernetes.io/part-of: argo-rollouts - rbac.authorization.k8s.io/aggregate-to-edit: "true" - name: argo-rollouts-aggregate-to-edit -rules: -- apiGroups: - - argoproj.io - resources: - - rollouts - - rollouts/scale - - rollouts/status - - experiments - - analysistemplates - - clusteranalysistemplates - - analysisruns - verbs: - - create - - delete - - deletecollection - - get - - list - - patch - - update - - watch ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - labels: - app.kubernetes.io/component: aggregate-cluster-role - app.kubernetes.io/name: argo-rollouts-aggregate-to-view - app.kubernetes.io/part-of: argo-rollouts - rbac.authorization.k8s.io/aggregate-to-view: "true" - name: argo-rollouts-aggregate-to-view -rules: -- apiGroups: - - argoproj.io - resources: - - rollouts - - rollouts/scale - - experiments - - analysistemplates - - clusteranalysistemplates - - analysisruns - verbs: - - get - - list - - watch ---- apiVersion: v1 kind: Service metadata: diff --git a/manifests/namespace-install/clusterrole-to-role.yaml b/manifests/namespace-install/clusterrole-to-role.yaml index 1e5fb5c19d..d012454170 100644 --- a/manifests/namespace-install/clusterrole-to-role.yaml +++ b/manifests/namespace-install/clusterrole-to-role.yaml @@ -4,3 +4,6 @@ - op: replace path: /kind value: Role +- op: replace + path: /metadata/labels/app.kubernetes.io~1name + value: argo-rollouts-role diff --git a/manifests/namespace-install/kustomization.yaml b/manifests/namespace-install/kustomization.yaml index 98ecb2a0dd..fff1720ce1 100644 --- a/manifests/namespace-install/kustomization.yaml +++ b/manifests/namespace-install/kustomization.yaml @@ -1,10 +1,10 @@ apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization -resources: +bases: - ../crds - ../base -- ../cluster-install/argo-rollouts-clusterrole.yaml +- ../role patchesStrategicMerge: - add-namespaced-flag.yaml diff --git a/manifests/cluster-install/argo-rollouts-clusterrole.yaml b/manifests/role/argo-rollouts-clusterrole.yaml similarity index 100% rename from manifests/cluster-install/argo-rollouts-clusterrole.yaml rename to manifests/role/argo-rollouts-clusterrole.yaml diff --git a/manifests/role/kustomization.yaml b/manifests/role/kustomization.yaml new file mode 100644 index 0000000000..643995301d --- /dev/null +++ b/manifests/role/kustomization.yaml @@ -0,0 +1,5 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +resources: +- argo-rollouts-clusterrole.yaml diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go index 475134d382..45bd971578 100644 --- a/pkg/apis/rollouts/v1alpha1/openapi_generated.go +++ b/pkg/apis/rollouts/v1alpha1/openapi_generated.go @@ -777,13 +777,6 @@ func schema_pkg_apis_rollouts_v1alpha1_CanaryStatus(ref common.ReferenceCallback Description: "CanaryStatus status fields that only pertain to the canary rollout", Type: []string{"object"}, Properties: map[string]spec.Schema{ - "stableRS": { - SchemaProps: spec.SchemaProps{ - Description: "StableRS indicates the last replicaset that walked through all the canary steps or was the only replicaset", - Type: []string{"string"}, - Format: "", - }, - }, "currentStepAnalysisRun": { SchemaProps: spec.SchemaProps{ Description: "CurrentStepAnalysisRun indicates the analysisRun for the current step index", diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index fad738db64..e51f9a25fd 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -10,7 +10,6 @@ import ( ) // +genclient -// +genclient:noStatus // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // +kubebuilder:resource:path=rollouts,shortName=ro // +kubebuilder:subresource:scale:specpath=.spec.replicas,statuspath=.status.HPAReplicas,selectorpath=.status.selector @@ -18,6 +17,7 @@ import ( // +kubebuilder:printcolumn:name="Current",type="integer",JSONPath=".status.replicas",description="Total number of non-terminated pods targeted by this rollout" // +kubebuilder:printcolumn:name="Up-to-date",type="integer",JSONPath=".status.updatedReplicas",description="Total number of non-terminated pods targeted by this rollout that have the desired template spec" // +kubebuilder:printcolumn:name="Available",type="integer",JSONPath=".status.availableReplicas",description="Total number of available pods (ready for at least minReadySeconds) targeted by this rollout" +// +kubebuilder:subresource:status // Rollout is a specification for a Rollout resource type Rollout struct { @@ -599,9 +599,6 @@ type BlueGreenStatus struct { // CanaryStatus status fields that only pertain to the canary rollout type CanaryStatus struct { - // StableRS indicates the last replicaset that walked through all the canary steps or was the only replicaset - // +optional - StableRS string `json:"stableRS,omitempty"` // CurrentStepAnalysisRun indicates the analysisRun for the current step index // TODO(Deprecated): Remove in v0.10 CurrentStepAnalysisRun string `json:"currentStepAnalysisRun,omitempty"` diff --git a/pkg/client/clientset/versioned/typed/rollouts/v1alpha1/fake/fake_rollout.go b/pkg/client/clientset/versioned/typed/rollouts/v1alpha1/fake/fake_rollout.go index 9e0ca8cccf..fca9011ae1 100644 --- a/pkg/client/clientset/versioned/typed/rollouts/v1alpha1/fake/fake_rollout.go +++ b/pkg/client/clientset/versioned/typed/rollouts/v1alpha1/fake/fake_rollout.go @@ -102,6 +102,18 @@ func (c *FakeRollouts) Update(ctx context.Context, rollout *v1alpha1.Rollout, op return obj.(*v1alpha1.Rollout), err } +// UpdateStatus was generated because the type contains a Status member. +// Add a +genclient:noStatus comment above the type to avoid generating UpdateStatus(). +func (c *FakeRollouts) UpdateStatus(ctx context.Context, rollout *v1alpha1.Rollout, opts v1.UpdateOptions) (*v1alpha1.Rollout, error) { + obj, err := c.Fake. + Invokes(testing.NewUpdateSubresourceAction(rolloutsResource, "status", c.ns, rollout), &v1alpha1.Rollout{}) + + if obj == nil { + return nil, err + } + return obj.(*v1alpha1.Rollout), err +} + // Delete takes name of the rollout and deletes it. Returns an error if one occurs. func (c *FakeRollouts) Delete(ctx context.Context, name string, opts v1.DeleteOptions) error { _, err := c.Fake. diff --git a/pkg/client/clientset/versioned/typed/rollouts/v1alpha1/rollout.go b/pkg/client/clientset/versioned/typed/rollouts/v1alpha1/rollout.go index 6ca808bca0..bc38f6affa 100644 --- a/pkg/client/clientset/versioned/typed/rollouts/v1alpha1/rollout.go +++ b/pkg/client/clientset/versioned/typed/rollouts/v1alpha1/rollout.go @@ -40,6 +40,7 @@ type RolloutsGetter interface { type RolloutInterface interface { Create(ctx context.Context, rollout *v1alpha1.Rollout, opts v1.CreateOptions) (*v1alpha1.Rollout, error) Update(ctx context.Context, rollout *v1alpha1.Rollout, opts v1.UpdateOptions) (*v1alpha1.Rollout, error) + UpdateStatus(ctx context.Context, rollout *v1alpha1.Rollout, opts v1.UpdateOptions) (*v1alpha1.Rollout, error) Delete(ctx context.Context, name string, opts v1.DeleteOptions) error DeleteCollection(ctx context.Context, opts v1.DeleteOptions, listOpts v1.ListOptions) error Get(ctx context.Context, name string, opts v1.GetOptions) (*v1alpha1.Rollout, error) @@ -135,6 +136,22 @@ func (c *rollouts) Update(ctx context.Context, rollout *v1alpha1.Rollout, opts v return } +// UpdateStatus was generated because the type contains a Status member. +// Add a +genclient:noStatus comment above the type to avoid generating UpdateStatus(). +func (c *rollouts) UpdateStatus(ctx context.Context, rollout *v1alpha1.Rollout, opts v1.UpdateOptions) (result *v1alpha1.Rollout, err error) { + result = &v1alpha1.Rollout{} + err = c.client.Put(). + Namespace(c.ns). + Resource("rollouts"). + Name(rollout.Name). + SubResource("status"). + VersionedParams(&opts, scheme.ParameterCodec). + Body(rollout). + Do(ctx). + Into(result) + return +} + // Delete takes name of the rollout and deletes it. Returns an error if one occurs. func (c *rollouts) Delete(ctx context.Context, name string, opts v1.DeleteOptions) error { return c.client.Delete(). diff --git a/pkg/kubectl-argo-rollouts/cmd/abort/abort.go b/pkg/kubectl-argo-rollouts/cmd/abort/abort.go index c9c68bcc62..f90062abc8 100644 --- a/pkg/kubectl-argo-rollouts/cmd/abort/abort.go +++ b/pkg/kubectl-argo-rollouts/cmd/abort/abort.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/spf13/cobra" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" types "k8s.io/apimachinery/pkg/types" @@ -58,5 +59,10 @@ func NewCmdAbort(o *options.ArgoRolloutsOptions) *cobra.Command { // AbortRollout aborts a rollout func AbortRollout(rolloutIf clientset.RolloutInterface, name string) (*v1alpha1.Rollout, error) { ctx := context.TODO() - return rolloutIf.Patch(ctx, name, types.MergePatchType, []byte(abortPatch), metav1.PatchOptions{}) + // attempt using status subresource, first + ro, err := rolloutIf.Patch(ctx, name, types.MergePatchType, []byte(abortPatch), metav1.PatchOptions{}, "status") + if err != nil && k8serrors.IsNotFound(err) { + ro, err = rolloutIf.Patch(ctx, name, types.MergePatchType, []byte(abortPatch), metav1.PatchOptions{}) + } + return ro, err } diff --git a/pkg/kubectl-argo-rollouts/cmd/get/get_test.go b/pkg/kubectl-argo-rollouts/cmd/get/get_test.go index 8ce1ef7375..1e4abd429c 100644 --- a/pkg/kubectl-argo-rollouts/cmd/get/get_test.go +++ b/pkg/kubectl-argo-rollouts/cmd/get/get_test.go @@ -118,6 +118,7 @@ func TestGetBlueGreenRollout(t *testing.T) { Name: bluegreen-demo Namespace: jesse-test Status: ॥ Paused +Message: BlueGreenPause Strategy: BlueGreen Images: argoproj/rollouts-demo:blue (stable, active) argoproj/rollouts-demo:green (preview) @@ -165,6 +166,7 @@ func TestGetBlueGreenRolloutScaleDownDelay(t *testing.T) { Name: bluegreen-demo Namespace: jesse-test Status: ॥ Paused +Message: BlueGreenPause Strategy: BlueGreen Images: argoproj/rollouts-demo:blue (stable, active) argoproj/rollouts-demo:green @@ -212,6 +214,7 @@ func TestGetBlueGreenRolloutScaleDownDelayPassed(t *testing.T) { Name: bluegreen-demo Namespace: jesse-test Status: ॥ Paused +Message: BlueGreenPause Strategy: BlueGreen Images: argoproj/rollouts-demo:blue (stable, active) argoproj/rollouts-demo:green diff --git a/pkg/kubectl-argo-rollouts/cmd/promote/promote.go b/pkg/kubectl-argo-rollouts/cmd/promote/promote.go index e011ec4a7e..795a4b40f3 100644 --- a/pkg/kubectl-argo-rollouts/cmd/promote/promote.go +++ b/pkg/kubectl-argo-rollouts/cmd/promote/promote.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/spf13/cobra" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" types "k8s.io/apimachinery/pkg/types" @@ -29,20 +30,11 @@ If not on a pause step use '--skip-current-step' to progress to the next step in ) const ( - setCurrentStepIndex = `{ - "status": { - "currentStepIndex": %d - } -}` + setCurrentStepIndex = `{"status":{"currentStepIndex":%d}}` + unpausePatch = `{"spec":{"paused":false}}` + clearPauseConditionsPatch = `{"status":{"pauseConditions":null}}` + unpauseAndClearPauseConditionsPatch = `{"spec":{"paused":false},"status":{"pauseConditions":null}}` - unpausePatch = `{ - "spec": { - "paused": false - }, - "status": { - "pauseConditions": null - } -}` useBothSkipFlagsError = "Cannot use skip-current-step and skip-all-steps flags at the same time" skipFlagsWithBlueGreenError = "Cannot skip steps of a bluegreen rollout. Run without a flags" skipFlagWithNoStepCanaryError = "Cannot skip steps of a rollout without steps" @@ -97,15 +89,36 @@ func PromoteRollout(rolloutIf clientset.RolloutInterface, name string, skipCurre return nil, fmt.Errorf(skipFlagWithNoStepCanaryError) } } - patch := getPatch(ro, skipCurrentStep, skipAllSteps) - ro, err = rolloutIf.Patch(ctx, name, types.MergePatchType, patch, metav1.PatchOptions{}) - if err != nil { - return nil, err + + // This function is intended to be compatible with Rollouts v0.9 and Rollouts v0.10+, the latter + // of which uses CRD status subresources. When using status subresource, status must be updated + // separately from spec. Since we don't know which version is installed in the cluster, we + // attempt status patching first. If it errors with NotFound, it indicates that status + // subresource is not used (v0.9), at which point we need to use the unified patch that updates + // both spec and status. Otherwise, we proceed with a spec only patch. + specPatch, statusPatch, unifiedPatch := getPatches(ro, skipCurrentStep, skipAllSteps) + if statusPatch != nil { + ro, err = rolloutIf.Patch(ctx, name, types.MergePatchType, statusPatch, metav1.PatchOptions{}, "status") + if err != nil { + // NOTE: in the future, we can simply return error here, if we wish to drop support for v0.9 + if !k8serrors.IsNotFound(err) { + return nil, err + } + // we got a NotFound error. status subresource is not being used, so perform unifiedPatch + specPatch = unifiedPatch + } + } + if specPatch != nil { + ro, err = rolloutIf.Patch(ctx, name, types.MergePatchType, specPatch, metav1.PatchOptions{}) + if err != nil { + return nil, err + } } return ro, nil } -func getPatch(rollout *v1alpha1.Rollout, skipCurrentStep, skipAllStep bool) []byte { +func getPatches(rollout *v1alpha1.Rollout, skipCurrentStep, skipAllStep bool) ([]byte, []byte, []byte) { + var specPatch, statusPatch, unifiedPatch []byte switch { case skipCurrentStep: _, index := replicasetutil.GetCurrentCanaryStep(rollout) @@ -114,10 +127,19 @@ func getPatch(rollout *v1alpha1.Rollout, skipCurrentStep, skipAllStep bool) []by if *index < int32(len(rollout.Spec.Strategy.Canary.Steps)) { *index++ } - return []byte(fmt.Sprintf(setCurrentStepIndex, *index)) + statusPatch = []byte(fmt.Sprintf(setCurrentStepIndex, *index)) + unifiedPatch = statusPatch case skipAllStep: - return []byte(fmt.Sprintf(setCurrentStepIndex, len(rollout.Spec.Strategy.Canary.Steps))) + statusPatch = []byte(fmt.Sprintf(setCurrentStepIndex, len(rollout.Spec.Strategy.Canary.Steps))) + unifiedPatch = statusPatch default: - return []byte(unpausePatch) + if rollout.Spec.Paused { + specPatch = []byte(unpausePatch) + } + if len(rollout.Status.PauseConditions) > 0 { + statusPatch = []byte(clearPauseConditionsPatch) + } + unifiedPatch = []byte(unpauseAndClearPauseConditionsPatch) } + return specPatch, statusPatch, unifiedPatch } diff --git a/pkg/kubectl-argo-rollouts/cmd/retry/retry.go b/pkg/kubectl-argo-rollouts/cmd/retry/retry.go index dc52ca20db..e5d74a3733 100644 --- a/pkg/kubectl-argo-rollouts/cmd/retry/retry.go +++ b/pkg/kubectl-argo-rollouts/cmd/retry/retry.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/spf13/cobra" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" types "k8s.io/apimachinery/pkg/types" @@ -82,7 +83,12 @@ func NewCmdRetryRollout(o *options.ArgoRolloutsOptions) *cobra.Command { // RetryRollout retries a rollout after it's been aborted func RetryRollout(rolloutIf clientset.RolloutInterface, name string) (*v1alpha1.Rollout, error) { ctx := context.TODO() - return rolloutIf.Patch(ctx, name, types.MergePatchType, []byte(retryRolloutPatch), metav1.PatchOptions{}) + // attempt using status subresource, first + ro, err := rolloutIf.Patch(ctx, name, types.MergePatchType, []byte(retryRolloutPatch), metav1.PatchOptions{}, "status") + if err != nil && k8serrors.IsNotFound(err) { + ro, err = rolloutIf.Patch(ctx, name, types.MergePatchType, []byte(retryRolloutPatch), metav1.PatchOptions{}) + } + return ro, err } // NewCmdRetryExperiment returns a new instance of an `argo rollouts retry experiment` command diff --git a/pkg/kubectl-argo-rollouts/info/info_test.go b/pkg/kubectl-argo-rollouts/info/info_test.go index f804418862..adc0794923 100644 --- a/pkg/kubectl-argo-rollouts/info/info_test.go +++ b/pkg/kubectl-argo-rollouts/info/info_test.go @@ -1,6 +1,7 @@ package info import ( + "strconv" "testing" "time" @@ -15,8 +16,9 @@ import ( func newCanaryRollout() *v1alpha1.Rollout { return &v1alpha1.Rollout{ ObjectMeta: metav1.ObjectMeta{ - Name: "can-guestbook", - Namespace: "test", + Name: "can-guestbook", + Namespace: "test", + Generation: 1, }, Spec: v1alpha1.RolloutSpec{ Replicas: pointer.Int32Ptr(5), @@ -39,11 +41,12 @@ func newCanaryRollout() *v1alpha1.Rollout { }, }, Status: v1alpha1.RolloutStatus{ - CurrentStepIndex: pointer.Int32Ptr(1), - Replicas: 4, - ReadyReplicas: 1, - UpdatedReplicas: 3, - AvailableReplicas: 2, + ObservedGeneration: "1", + CurrentStepIndex: pointer.Int32Ptr(1), + Replicas: 4, + ReadyReplicas: 1, + UpdatedReplicas: 3, + AvailableReplicas: 2, }, } } @@ -51,8 +54,9 @@ func newCanaryRollout() *v1alpha1.Rollout { func newBlueGreenRollout() *v1alpha1.Rollout { return &v1alpha1.Rollout{ ObjectMeta: metav1.ObjectMeta{ - Name: "bg-guestbook", - Namespace: "test", + Name: "bg-guestbook", + Namespace: "test", + Generation: 1, }, Spec: v1alpha1.RolloutSpec{ Replicas: pointer.Int32Ptr(5), @@ -61,10 +65,11 @@ func newBlueGreenRollout() *v1alpha1.Rollout { }, }, Status: v1alpha1.RolloutStatus{ - Replicas: 4, - ReadyReplicas: 1, - UpdatedReplicas: 3, - AvailableReplicas: 2, + ObservedGeneration: "1", + Replicas: 4, + ReadyReplicas: 1, + UpdatedReplicas: 3, + AvailableReplicas: 2, }, } } @@ -201,7 +206,7 @@ func TestRolloutStatusPaused(t *testing.T) { ro.Spec.Paused = true status, message := RolloutStatusString(ro) assert.Equal(t, "Paused", status) - assert.Equal(t, "", message) + assert.Equal(t, "manually paused", message) } func TestRolloutStatusProgressing(t *testing.T) { @@ -267,8 +272,38 @@ func TestRolloutStatusProgressing(t *testing.T) { ro := newCanaryRollout() ro.Spec.Replicas = nil ro.Status = v1alpha1.RolloutStatus{ - StableRS: "abc1234", - CurrentPodHash: "abc1234", + ObservedGeneration: strconv.Itoa(int(ro.Generation)), + StableRS: "abc1234", + CurrentPodHash: "abc1234", + } + status, message := RolloutStatusString(ro) + assert.Equal(t, "Progressing", status) + assert.Equal(t, "more replicas need to be updated", message) + } + { + // Rollout observed generation is not updated + ro := newCanaryRollout() + ro.Generation = 2 + ro.Spec.Replicas = nil + ro.Status = v1alpha1.RolloutStatus{ + StableRS: "abc1234", + CurrentPodHash: "abc1234", + ObservedGeneration: "1", + } + status, message := RolloutStatusString(ro) + assert.Equal(t, "Progressing", status) + assert.Equal(t, "waiting for rollout spec update to be observed", message) + } + { + // Make sure we skip isGenerationObserved check when rollout is a v0.9 legacy rollout using + // a hash and not a numeric observed generation + ro := newCanaryRollout() + ro.Generation = 2 + ro.Spec.Replicas = nil + ro.Status = v1alpha1.RolloutStatus{ + StableRS: "abc1234", + CurrentPodHash: "abc1234", + ObservedGeneration: "7d66d4485f", } status, message := RolloutStatusString(ro) assert.Equal(t, "Progressing", status) diff --git a/pkg/kubectl-argo-rollouts/info/rollout_info.go b/pkg/kubectl-argo-rollouts/info/rollout_info.go index 53d5cb1841..18a3885bed 100644 --- a/pkg/kubectl-argo-rollouts/info/rollout_info.go +++ b/pkg/kubectl-argo-rollouts/info/rollout_info.go @@ -122,10 +122,24 @@ func RolloutErrorConditions(ro *v1alpha1.Rollout) []string { return errorConditions } +// isGenerationObserved determines if the rollout spec has been observed by the controller. This +// only applies to v0.10 rollout which uses a numeric status.observedGeneration. For v0.9 rollouts +// and below this function always returns true. +func isGenerationObserved(ro *v1alpha1.Rollout) bool { + observedGen, err := strconv.Atoi(ro.Status.ObservedGeneration) + if err != nil { + return true + } + return int64(observedGen) == ro.Generation +} + // RolloutStatusString returns a status and message for a rollout // This logic is more or less the same as the Argo CD rollouts health.lua check // Any changes to this function should also be changed there func RolloutStatusString(ro *v1alpha1.Rollout) (string, string) { + if !isGenerationObserved(ro) { + return "Progressing", "waiting for rollout spec update to be observed" + } for _, cond := range ro.Status.Conditions { if cond.Type == v1alpha1.InvalidSpec { return "Degraded", fmt.Sprintf("%s: %s", v1alpha1.InvalidSpec, cond.Message) @@ -135,8 +149,11 @@ func RolloutStatusString(ro *v1alpha1.Rollout) (string, string) { return "Degraded", fmt.Sprintf("%s: %s", cond.Reason, cond.Message) } } - if ro.Spec.Paused || len(ro.Status.PauseConditions) > 0 { - return "Paused", "" + if ro.Spec.Paused { + return "Paused", "manually paused" + } + for _, pauseCond := range ro.Status.PauseConditions { + return "Paused", string(pauseCond.Reason) } if ro.Status.UpdatedReplicas < defaults.GetReplicasOrDefault(ro.Spec.Replicas) { return "Progressing", "more replicas need to be updated" @@ -144,16 +161,11 @@ func RolloutStatusString(ro *v1alpha1.Rollout) (string, string) { if ro.Status.AvailableReplicas < ro.Status.UpdatedReplicas { return "Progressing", "updated replicas are still becoming available" } - stableRS := ro.Status.StableRS - //TODO(dthomson) Remove canary.stableRS for v0.9 - if ro.Status.Canary.StableRS != "" { - stableRS = ro.Status.Canary.StableRS - } if ro.Spec.Strategy.BlueGreen != nil { if ro.Status.BlueGreen.ActiveSelector == "" || ro.Status.BlueGreen.ActiveSelector != ro.Status.CurrentPodHash { return "Progressing", "active service cutover pending" } - if stableRS == "" || stableRS != ro.Status.CurrentPodHash { + if ro.Status.StableRS == "" || ro.Status.StableRS != ro.Status.CurrentPodHash { return "Progressing", "waiting for analysis to complete" } } else if ro.Spec.Strategy.Canary != nil { @@ -162,7 +174,7 @@ func RolloutStatusString(ro *v1alpha1.Rollout) (string, string) { // scaleDownDelay feature which leaves the old stack of replicas running for a long time return "Progressing", "old replicas are pending termination" } - if stableRS == "" || stableRS != ro.Status.CurrentPodHash { + if ro.Status.StableRS == "" || ro.Status.StableRS != ro.Status.CurrentPodHash { return "Progressing", "waiting for all steps to complete" } } diff --git a/pkg/kubectl-argo-rollouts/info/testdata/blue-green/bluegreen-rollout.yaml b/pkg/kubectl-argo-rollouts/info/testdata/blue-green/bluegreen-rollout.yaml index 6127a30e7a..6c753e7e50 100644 --- a/pkg/kubectl-argo-rollouts/info/testdata/blue-green/bluegreen-rollout.yaml +++ b/pkg/kubectl-argo-rollouts/info/testdata/blue-green/bluegreen-rollout.yaml @@ -64,7 +64,7 @@ status: type: Progressing controllerPause: true currentPodHash: 74b948fccb - observedGeneration: 7d66d4485f + observedGeneration: "410" pauseConditions: - reason: BlueGreenPause startTime: "2019-10-28T04:50:20Z" diff --git a/pkg/kubectl-argo-rollouts/info/testdata/canary/canary-rollout.yaml b/pkg/kubectl-argo-rollouts/info/testdata/canary/canary-rollout.yaml index ae3545121e..2c89df2b94 100644 --- a/pkg/kubectl-argo-rollouts/info/testdata/canary/canary-rollout.yaml +++ b/pkg/kubectl-argo-rollouts/info/testdata/canary/canary-rollout.yaml @@ -75,7 +75,7 @@ status: currentPodHash: 65fb5ffc84 currentStepHash: f64cdc9d currentStepIndex: 0 - observedGeneration: 5c76fd9f86 + observedGeneration: "429" readyReplicas: 5 replicas: 6 selector: app=canary-demo diff --git a/pkg/kubectl-argo-rollouts/info/testdata/experiment-analysis/rollout-experiment-analysis.yaml b/pkg/kubectl-argo-rollouts/info/testdata/experiment-analysis/rollout-experiment-analysis.yaml index e7566ac164..a9333ca50e 100644 --- a/pkg/kubectl-argo-rollouts/info/testdata/experiment-analysis/rollout-experiment-analysis.yaml +++ b/pkg/kubectl-argo-rollouts/info/testdata/experiment-analysis/rollout-experiment-analysis.yaml @@ -65,7 +65,7 @@ status: currentPodHash: 6f646bf7b7 currentStepHash: 5f7ff4d7d8 currentStepIndex: 1 - observedGeneration: 569d957d8 + observedGeneration: "25" pauseConditions: - reason: InconclusiveAnalysisRun startTime: "2019-10-30T16:13:40Z" diff --git a/pkg/kubectl-argo-rollouts/info/testdata/experiment-step/canary-demo.yaml b/pkg/kubectl-argo-rollouts/info/testdata/experiment-step/canary-demo.yaml index 41a7c201d9..a9cdb00c30 100644 --- a/pkg/kubectl-argo-rollouts/info/testdata/experiment-step/canary-demo.yaml +++ b/pkg/kubectl-argo-rollouts/info/testdata/experiment-step/canary-demo.yaml @@ -75,7 +75,7 @@ status: currentPodHash: 645d5dbc4c currentStepHash: 7d595679dd currentStepIndex: 0 - observedGeneration: 7664c98448 + observedGeneration: "13" readyReplicas: 5 replicas: 5 selector: app=canary-demo diff --git a/pkg/kubectl-argo-rollouts/info/testdata/rollout-aborted/rollout.yaml b/pkg/kubectl-argo-rollouts/info/testdata/rollout-aborted/rollout.yaml index eaf4d62253..33bf7eb307 100644 --- a/pkg/kubectl-argo-rollouts/info/testdata/rollout-aborted/rollout.yaml +++ b/pkg/kubectl-argo-rollouts/info/testdata/rollout-aborted/rollout.yaml @@ -55,7 +55,6 @@ status: message: metric "web" assessed Failed due to failed (1) > failureLimit (0) name: rollout-background-analysis-db976bc44-2 status: Failed - stableRS: 7d84d44bb8 conditions: - lastTransitionTime: "2020-09-22T09:09:47Z" lastUpdateTime: "2020-09-22T09:09:47Z" @@ -72,7 +71,7 @@ status: currentPodHash: db976bc44 currentStepHash: 849b756fd4 currentStepIndex: 0 - observedGeneration: 7d77fd9646 + observedGeneration: "15" readyReplicas: 1 replicas: 1 selector: app=rollout-background-analysis diff --git a/pkg/kubectl-argo-rollouts/info/testdata/rollout-invalid/rollout-invalid.yaml b/pkg/kubectl-argo-rollouts/info/testdata/rollout-invalid/rollout-invalid.yaml index 439433358e..8ea4be03c6 100644 --- a/pkg/kubectl-argo-rollouts/info/testdata/rollout-invalid/rollout-invalid.yaml +++ b/pkg/kubectl-argo-rollouts/info/testdata/rollout-invalid/rollout-invalid.yaml @@ -43,4 +43,4 @@ status: reason: InvalidSpec status: "True" type: InvalidSpec - observedGeneration: 67655ff475 + observedGeneration: "3" diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go index fc8e62a622..68d4566dd9 100644 --- a/rollout/analysis_test.go +++ b/rollout/analysis_test.go @@ -3,6 +3,7 @@ package rollout import ( "encoding/json" "fmt" + "strconv" "testing" "time" @@ -1317,7 +1318,7 @@ func TestCancelBackgroundAnalysisRunWhenRolloutIsCompleted(t *testing.T) { rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r2 = updateCanaryRolloutStatus(r2, rs2PodHash, 1, 1, 1, false) - r2.Status.ObservedGeneration = conditions.ComputeGenerationHash(r2.Spec) + r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) r2.Status.Canary.CurrentBackgroundAnalysisRun = ar.Name f.rolloutLister = append(f.rolloutLister, r2) @@ -1397,8 +1398,7 @@ func TestDoNotCreateBackgroundAnalysisRunOnNewCanaryRollout(t *testing.T) { f.objects = append(f.objects, r1, at) f.expectCreateReplicaSetAction(rs1) - // Update the revision - f.expectUpdateRolloutAction(r1) + f.expectUpdateRolloutStatusAction(r1) // update conditions f.expectPatchRolloutAction(r1) f.run(getKey(r1, t)) } @@ -1428,8 +1428,7 @@ func TestDoNotCreateBackgroundAnalysisRunOnNewCanaryRolloutStableRSEmpty(t *test f.objects = append(f.objects, r1, at) f.expectCreateReplicaSetAction(rs1) - // Update the revision - f.expectUpdateRolloutAction(r1) + f.expectUpdateRolloutStatusAction(r1) // update conditions f.expectPatchRolloutAction(r1) f.run(getKey(r1, t)) } @@ -1516,7 +1515,7 @@ func TestDoNotCreatePrePromotionAnalysisAfterPromotionRollout(t *testing.T) { f.objects = append(f.objects, at) r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 1, 1, false, true) - r2.Status.ObservedGeneration = conditions.ComputeGenerationHash(r2.Spec) + r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -1562,7 +1561,7 @@ func TestDoNotCreatePrePromotionAnalysisRunOnNewRollout(t *testing.T) { rs := newReplicaSet(r, 1) f.expectCreateReplicaSetAction(rs) - f.expectUpdateRolloutAction(r) + f.expectUpdateRolloutStatusAction(r) f.expectPatchRolloutAction(r) f.run(getKey(r, t)) } @@ -1826,7 +1825,7 @@ func TestRolloutPrePromotionAnalysisDoNothingOnInconclusiveAnalysis(t *testing.T StartTime: metav1.Now(), } r2.Status.PauseConditions = append(r2.Status.PauseConditions, inconclusivePauseCondition) - r2.Status.ObservedGeneration = conditions.ComputeGenerationHash(r2.Spec) + r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) pausedCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, r2, "") conditions.SetRolloutCondition(&r2.Status, pausedCondition) diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go index 69f8b08602..7d76ce2847 100644 --- a/rollout/bluegreen_test.go +++ b/rollout/bluegreen_test.go @@ -2,6 +2,7 @@ package rollout import ( "fmt" + "strconv" "testing" "time" @@ -51,7 +52,7 @@ func TestBlueGreenCreatesReplicaSet(t *testing.T) { f.expectCreateReplicaSetAction(rs) servicePatchIndex := f.expectPatchServiceAction(previewSvc, rsPodHash) - updatedRolloutIndex := f.expectUpdateRolloutAction(r) + updatedRolloutIndex := f.expectUpdateRolloutStatusAction(r) expectedPatchWithoutSubs := `{ "status":{ "blueGreen" : { @@ -989,7 +990,7 @@ func TestBlueGreenRolloutCompleted(t *testing.T) { f.kubeobjects = append(f.kubeobjects, s) r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 1, 1, false, true) - r2.Status.ObservedGeneration = conditions.ComputeGenerationHash(r2.Spec) + r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) diff --git a/rollout/canary.go b/rollout/canary.go index f09f74bf55..357e1cf9ac 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -236,8 +236,6 @@ func (c *rolloutContext) syncRolloutStatusCanary() error { _, currentStepIndex := replicasetutil.GetCurrentCanaryStep(c.rollout) newStatus.StableRS = c.rollout.Status.StableRS - //TODO(dthomson) Remove in v0.9.0 - newStatus.Canary.StableRS = c.rollout.Status.Canary.StableRS newStatus.CurrentStepHash = conditions.ComputeStepHash(c.rollout) stepCount := int32(len(c.rollout.Spec.Strategy.Canary.Steps)) @@ -264,8 +262,6 @@ func (c *rolloutContext) syncRolloutStatusCanary() error { msg := fmt.Sprintf("Setting StableRS to CurrentPodHash: StableRS hash: %s", newStatus.CurrentPodHash) c.log.Info(msg) newStatus.StableRS = newStatus.CurrentPodHash - //TODO(dthomson) Remove in v0.9.0 - newStatus.Canary.StableRS = newStatus.CurrentPodHash if stepCount > 0 { if stepCount != *currentStepIndex { c.recorder.Event(c.rollout, corev1.EventTypeNormal, "SettingStableRS", msg) @@ -296,8 +292,6 @@ func (c *rolloutContext) syncRolloutStatusCanary() error { if c.newRS != nil && c.newRS.Status.AvailableReplicas == defaults.GetReplicasOrDefault(c.rollout.Spec.Replicas) { c.log.Info("New RS has successfully progressed") newStatus.StableRS = newStatus.CurrentPodHash - //TODO(dthomson) Remove in v0.9.0 - newStatus.Canary.StableRS = newStatus.CurrentPodHash } c.pauseContext.ClearPauseConditions() newStatus = c.calculateRolloutConditions(newStatus) @@ -309,8 +303,6 @@ func (c *rolloutContext) syncRolloutStatusCanary() error { if c.newRS != nil && c.newRS.Status.AvailableReplicas == defaults.GetReplicasOrDefault(c.rollout.Spec.Replicas) { c.log.Info("New RS has successfully progressed") newStatus.StableRS = newStatus.CurrentPodHash - //TODO(dthomson) Remove in v0.9.0 - newStatus.Canary.StableRS = newStatus.CurrentPodHash } newStatus = c.calculateRolloutConditions(newStatus) return c.persistRolloutStatus(&newStatus) diff --git a/rollout/canary_test.go b/rollout/canary_test.go index 7ec7605f6c..1064ec519f 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -42,6 +42,7 @@ func newCanaryRollout(name string, replicas int, revisionHistoryLimit *int32, st func bumpVersion(rollout *v1alpha1.Rollout) *v1alpha1.Rollout { newRollout := rollout.DeepCopy() + newRollout.Generation = newRollout.Generation + 1 revision := rollout.Annotations[annotations.RevisionAnnotation] newRevision, _ := strconv.Atoi(revision) newRevision++ @@ -61,9 +62,9 @@ func TestCanaryRolloutBumpVersion(t *testing.T) { r1 := newCanaryRollout("foo", 10, nil, nil, int32Ptr(0), intstr.FromInt(1), intstr.FromInt(0)) rs1 := newReplicaSetWithStatus(r1, 10, 10) r1.Status.StableRS = rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - r1.Status.Canary.StableRS = rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r2 := bumpVersion(r1) + r2.Annotations[annotations.RevisionAnnotation] = "1" f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -72,7 +73,8 @@ func TestCanaryRolloutBumpVersion(t *testing.T) { f.replicaSetLister = append(f.replicaSetLister, rs1) createdRSIndex := f.expectCreateReplicaSetAction(rs2) - updatedRolloutIndex := f.expectUpdateRolloutAction(r2) + updatedRolloutRevisionIndex := f.expectUpdateRolloutAction(r2) // update rollout revision + updatedRolloutConditionsIndex := f.expectUpdateRolloutStatusAction(r2) // update rollout conditions f.expectPatchRolloutAction(r2) f.run(getKey(r2, t)) @@ -80,9 +82,11 @@ func TestCanaryRolloutBumpVersion(t *testing.T) { assert.Equal(t, int32(1), *createdRS.Spec.Replicas) assert.Equal(t, "2", createdRS.Annotations[annotations.RevisionAnnotation]) - updatedRollout := f.getUpdatedRollout(updatedRolloutIndex) - progressingCondition := conditions.GetRolloutCondition(updatedRollout.Status, v1alpha1.RolloutProgressing) + updatedRollout := f.getUpdatedRollout(updatedRolloutRevisionIndex) assert.Equal(t, "2", updatedRollout.Annotations[annotations.RevisionAnnotation]) + + updatedRollout = f.getUpdatedRollout(updatedRolloutConditionsIndex) + progressingCondition := conditions.GetRolloutCondition(updatedRollout.Status, v1alpha1.RolloutProgressing) assert.NotNil(t, progressingCondition) assert.Equal(t, conditions.NewReplicaSetReason, progressingCondition.Reason) assert.Equal(t, corev1.ConditionTrue, progressingCondition.Status) @@ -353,14 +357,11 @@ func TestCanaryRolloutUpdateStatusWhenAtEndOfSteps(t *testing.T) { expectedPatchWithoutStableRS := `{ "status": { "stableRS": "%s", - "canary": { - "stableRS": "%s" - }, "conditions": %s } }` - expectedPatch := fmt.Sprintf(expectedPatchWithoutStableRS, expectedStableRS, expectedStableRS, generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "")) + expectedPatch := fmt.Sprintf(expectedPatchWithoutStableRS, expectedStableRS, generateConditionsPatch(true, conditions.ReplicaSetUpdatedReason, rs2, false, "")) assert.Equal(t, calculatePatch(r2, expectedPatch), patch) } @@ -462,7 +463,7 @@ func TestCanaryRolloutCreateFirstReplicasetNoSteps(t *testing.T) { rs := newReplicaSet(r, 1) f.expectCreateReplicaSetAction(rs) - updatedRolloutIndex := f.expectUpdateRolloutAction(r) + updatedRolloutIndex := f.expectUpdateRolloutStatusAction(r) patchIndex := f.expectPatchRolloutAction(r) f.run(getKey(r, t)) @@ -477,9 +478,6 @@ func TestCanaryRolloutCreateFirstReplicasetNoSteps(t *testing.T) { expectedPatch := `{ "status":{ "stableRS":"` + rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + `", - "canary": { - "stableRS":"` + rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + `" - }, "currentPodHash":"` + rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + `", "conditions": %s } @@ -504,7 +502,7 @@ func TestCanaryRolloutCreateFirstReplicasetWithSteps(t *testing.T) { rs := newReplicaSet(r, 1) f.expectCreateReplicaSetAction(rs) - updatedRolloutIndex := f.expectUpdateRolloutAction(r) + updatedRolloutIndex := f.expectUpdateRolloutStatusAction(r) patchIndex := f.expectPatchRolloutAction(r) f.run(getKey(r, t)) @@ -519,9 +517,6 @@ func TestCanaryRolloutCreateFirstReplicasetWithSteps(t *testing.T) { expectedPatchWithSub := `{ "status":{ "stableRS":"` + rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + `", - "canary": { - "stableRS":"` + rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + `" - }, "currentStepIndex":1, "currentPodHash":"` + rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] + `", "conditions": %s @@ -541,7 +536,6 @@ func TestCanaryRolloutCreateNewReplicaWithCorrectWeight(t *testing.T) { }} r1 := newCanaryRollout("foo", 10, nil, steps, int32Ptr(0), intstr.FromInt(1), intstr.FromInt(0)) r1.Status.StableRS = "895c6c4f9" - r1.Status.Canary.StableRS = "895c6c4f9" r2 := bumpVersion(r1) f.rolloutLister = append(f.rolloutLister, r2) @@ -553,7 +547,7 @@ func TestCanaryRolloutCreateNewReplicaWithCorrectWeight(t *testing.T) { f.replicaSetLister = append(f.replicaSetLister, rs1) createdRSIndex := f.expectCreateReplicaSetAction(rs2) - updatedRolloutIndex := f.expectUpdateRolloutAction(r2) + updatedRolloutIndex := f.expectUpdateRolloutStatusAction(r2) f.expectPatchRolloutAction(r2) f.run(getKey(r2, t)) @@ -577,7 +571,6 @@ func TestCanaryRolloutScaleUpNewReplicaWithCorrectWeight(t *testing.T) { }} r1 := newCanaryRollout("foo", 5, nil, steps, int32Ptr(0), intstr.FromInt(0), intstr.FromInt(1)) r1.Status.StableRS = "895c6c4f9" - r1.Status.Canary.StableRS = "895c6c4f9" r2 := bumpVersion(r1) f.rolloutLister = append(f.rolloutLister, r2) @@ -607,7 +600,6 @@ func TestCanaryRolloutScaleDownStableToMatchWeight(t *testing.T) { }} r1 := newCanaryRollout("foo", 10, nil, steps, int32Ptr(0), intstr.FromInt(0), intstr.FromInt(1)) r1.Status.StableRS = r1.Status.CurrentPodHash - r1.Status.Canary.StableRS = r1.Status.CurrentPodHash r2 := bumpVersion(r1) f.rolloutLister = append(f.rolloutLister, r2) @@ -639,7 +631,6 @@ func TestCanaryRolloutScaleDownOldRs(t *testing.T) { }} r1 := newCanaryRollout("foo", 10, nil, steps, int32Ptr(0), intstr.FromInt(1), intstr.FromInt(0)) r1.Status.StableRS = r1.Status.CurrentPodHash - r1.Status.Canary.StableRS = r1.Status.CurrentPodHash r2 := bumpVersion(r1) @@ -870,7 +861,7 @@ func TestSyncRolloutWaitAddToQueue(t *testing.T) { progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) - r2.Status.ObservedGeneration = conditions.ComputeGenerationHash(r2.Spec) + r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) f.rolloutLister = append(f.rolloutLister, r2) f.objects = append(f.objects, r2) @@ -908,7 +899,7 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) { f.replicaSetLister = append(f.replicaSetLister, rs1, rs2) r2 = updateCanaryRolloutStatus(r2, rs1PodHash, 10, 1, 10, true) - r2.Status.ObservedGeneration = conditions.ComputeGenerationHash(r2.Spec) + r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation)) progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "") conditions.SetRolloutCondition(&r2.Status, progressingCondition) @@ -1077,7 +1068,6 @@ func TestCanaryRolloutWithStableService(t *testing.T) { rs := newReplicaSetWithStatus(rollout, 0, 0) rollout.Spec.Strategy.Canary.StableService = stableSvc.Name rollout.Status.StableRS = rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - rollout.Status.Canary.StableRS = rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] f.rolloutLister = append(f.rolloutLister, rollout) f.objects = append(f.objects, rollout) @@ -1097,7 +1087,6 @@ func TestCanaryRolloutWithInvalidStableServiceName(t *testing.T) { rs := newReplicaSetWithStatus(rollout, 0, 0) rollout.Spec.Strategy.Canary.StableService = "invalid-stable" rollout.Status.StableRS = rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] - rollout.Status.Canary.StableRS = rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] f.rolloutLister = append(f.rolloutLister, rollout) f.objects = append(f.objects, rollout) @@ -1182,7 +1171,7 @@ func TestResumeRolloutAfterPauseDuration(t *testing.T) { rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r1 = updateCanaryRolloutStatus(r1, rs1PodHash, 1, 1, 1, true) overAMinuteAgo := metav1.Time{Time: time.Now().Add(-61 * time.Second)} - r1.Status.ObservedGeneration = conditions.ComputeGenerationHash(r1.Spec) + r1.Status.ObservedGeneration = strconv.Itoa(int(r1.Generation)) r1.Status.PauseConditions = []v1alpha1.PauseCondition{{ Reason: v1alpha1.PauseReasonCanaryPauseStep, StartTime: overAMinuteAgo, @@ -1229,8 +1218,7 @@ func TestNoResumeAfterPauseDurationIfUserPaused(t *testing.T) { rs1 := newReplicaSetWithStatus(r1, 1, 1) rs1PodHash := rs1.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r1 = updateCanaryRolloutStatus(r1, rs1PodHash, 1, 1, 1, true) - overAMinuteAgo := metav1.Time{Time: time.Now().Add(-61 * time.Second)} - r1.Status.ObservedGeneration = conditions.ComputeGenerationHash(r1.Spec) + overAMinuteAgo := metav1.Time{Time: time.Now().Add(-63 * time.Second)} r1.Status.PauseConditions = []v1alpha1.PauseCondition{{ Reason: v1alpha1.PauseReasonCanaryPauseStep, StartTime: overAMinuteAgo, @@ -1388,7 +1376,7 @@ func TestSyncEphemeralMetadataInitialRevision(t *testing.T) { f.rolloutLister = append(f.rolloutLister, r1) f.objects = append(f.objects, r1) - f.expectUpdateRolloutAction(r1) + f.expectUpdateRolloutStatusAction(r1) idx := f.expectCreateReplicaSetAction(rs1) _ = f.expectPatchRolloutAction(r1) f.run(getKey(r1, t)) @@ -1423,7 +1411,6 @@ func TestSyncEphemeralMetadataSecondRevision(t *testing.T) { rs1 := newReplicaSetWithStatus(r1, 3, 3) r2 := bumpVersion(r1) r2.Status.StableRS = r1.Status.CurrentPodHash - r2.Status.Canary.StableRS = r1.Status.CurrentPodHash rs2 := newReplicaSetWithStatus(r2, 3, 3) rsGVK := schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "ReplicaSet"} pod := corev1.Pod{ @@ -1443,7 +1430,7 @@ func TestSyncEphemeralMetadataSecondRevision(t *testing.T) { f.kubeobjects = append(f.kubeobjects, rs1, &pod) f.replicaSetLister = append(f.replicaSetLister, rs1) - f.expectUpdateRolloutAction(r2) // Update Rollout revision to 2 + f.expectUpdateRolloutStatusAction(r2) // Update Rollout conditions rs2idx := f.expectCreateReplicaSetAction(rs2) // Create revision 2 ReplicaSet f.expectListPodAction(r1.Namespace) // list pods to patch ephemeral data on revision 1 ReplicaSets pods podIdx := f.expectUpdatePodAction(&pod) // Update pod with ephemeral data diff --git a/rollout/controller.go b/rollout/controller.go index 31a0efa36a..4486789ea1 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "reflect" + "strconv" "time" smiclientset "github.com/servicemeshinterface/smi-sdk-go/pkg/gen/client/split/clientset/versioned" @@ -381,13 +382,6 @@ func (c *Controller) syncHandler(key string) error { r := remarshalRollout(rollout) logCtx := logutil.WithRollout(r) - // TODO(dthomson) remove in v0.9.0 - migrated := c.migrateCanaryStableRS(r) - if migrated { - logCtx.Info("Migrated stableRS field") - return nil - } - if r.ObjectMeta.DeletionTimestamp != nil { logCtx.Info("No reconciliation as rollout marked for deletion") return nil @@ -510,8 +504,7 @@ func (c *rolloutContext) createInvalidRolloutCondition(validationError error, r invalidSpecCond = conditions.NewRolloutCondition(v1alpha1.InvalidSpec, corev1.ConditionTrue, conditions.InvalidSpecReason, errorMessage) } c.log.Error(errorMessage) - generation := conditions.ComputeGenerationHash(r.Spec) - if r.Status.ObservedGeneration != generation || !reflect.DeepEqual(invalidSpecCond, prevCond) { + if r.Status.ObservedGeneration != strconv.Itoa(int(r.Generation)) || !reflect.DeepEqual(invalidSpecCond, prevCond) { newStatus := r.Status.DeepCopy() // SetRolloutCondition only updates the condition when the status and/or reason changes, but // the controller should update the invalidSpec if there is a change in why the spec is invalid @@ -763,30 +756,6 @@ func (c *rolloutContext) getReferencedVirtualServices() (*[]unstructured.Unstruc return &virtualServices, nil } -func (c *Controller) migrateCanaryStableRS(rollout *v1alpha1.Rollout) bool { - ctx := context.TODO() - if rollout.Spec.Strategy.Canary == nil { - return false - } - if rollout.Status.StableRS == "" && rollout.Status.Canary.StableRS == "" { - return false - } - if rollout.Status.StableRS != "" && rollout.Status.Canary.StableRS != "" { - return false - } - stableRS := rollout.Status.StableRS - if rollout.Status.Canary.StableRS != "" { - stableRS = rollout.Status.Canary.StableRS - } - rollout.Status.Canary.StableRS = stableRS - rollout.Status.StableRS = stableRS - _, err := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(rollout.Namespace).Update(ctx, rollout, metav1.UpdateOptions{}) - if err != nil { - logutil.WithRollout(rollout).Errorf("Unable to migrate Rollout's status.canary.stableRS to status.stableRS: %s", err.Error()) - } - return true -} - func remarshalRollout(r *v1alpha1.Rollout) *v1alpha1.Rollout { rolloutBytes, err := json.Marshal(r) if err != nil { diff --git a/rollout/controller_test.go b/rollout/controller_test.go index e1b6f4733d..6aefca37bb 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -114,6 +114,7 @@ func newRollout(name string, replicas int, revisionHistoryLimit *int32, selector Annotations: map[string]string{ annotations.RevisionAnnotation: "1", }, + Generation: 123, }, Spec: v1alpha1.RolloutSpec{ Template: corev1.PodTemplateSpec{ @@ -287,8 +288,6 @@ func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active, stable s func updateCanaryRolloutStatus(r *v1alpha1.Rollout, stableRS string, availableReplicas, updatedReplicas, hpaReplicas int32, pause bool) *v1alpha1.Rollout { newRollout := updateBaseRolloutStatus(r, availableReplicas, updatedReplicas, availableReplicas, hpaReplicas) newRollout.Status.StableRS = stableRS - //TODO(dthomson) Remove in v0.9 - newRollout.Status.Canary.StableRS = stableRS if pause { now := metav1.Now() cond := v1alpha1.PauseCondition{ @@ -353,7 +352,7 @@ func calculatePatch(ro *v1alpha1.Rollout, patch string) string { } newRO := &v1alpha1.Rollout{} json.Unmarshal(newBytes, newRO) - newObservedGen := conditions.ComputeGenerationHash(newRO.Spec) + newObservedGen := strconv.Itoa(int(newRO.Generation)) newPatch := make(map[string]interface{}) err = json.Unmarshal([]byte(patch), &newPatch) @@ -699,6 +698,13 @@ func (f *fixture) expectUpdateRolloutAction(rollout *v1alpha1.Rollout) int { return len } +func (f *fixture) expectUpdateRolloutStatusAction(rollout *v1alpha1.Rollout) int { + action := core.NewUpdateSubresourceAction(schema.GroupVersionResource{Resource: "rollouts"}, "status", rollout.Namespace, rollout) + len := len(f.actions) + f.actions = append(f.actions, action) + return len +} + func (f *fixture) expectPatchExperimentAction(ex *v1alpha1.Experiment) int { experimentSchema := schema.GroupVersionResource{ Resource: "experiments", @@ -715,7 +721,7 @@ func (f *fixture) expectPatchRolloutAction(rollout *v1alpha1.Rollout) int { Version: "v1alpha1", } len := len(f.actions) - f.actions = append(f.actions, core.NewPatchAction(serviceSchema, rollout.Namespace, rollout.Name, types.MergePatchType, nil)) + f.actions = append(f.actions, core.NewPatchSubresourceAction(serviceSchema, rollout.Namespace, rollout.Name, types.MergePatchType, nil, "status")) return len } @@ -726,7 +732,7 @@ func (f *fixture) expectPatchRolloutActionWithPatch(rollout *v1alpha1.Rollout, p Version: "v1alpha1", } len := len(f.actions) - f.actions = append(f.actions, core.NewPatchAction(serviceSchema, rollout.Namespace, rollout.Name, types.MergePatchType, []byte(expectedPatch))) + f.actions = append(f.actions, core.NewPatchSubresourceAction(serviceSchema, rollout.Namespace, rollout.Name, types.MergePatchType, []byte(expectedPatch), "status")) return len } @@ -961,7 +967,7 @@ func TestAdoptReplicaSet(t *testing.T) { f.replicaSetLister = append(f.replicaSetLister, rs) f.serviceLister = append(f.serviceLister, previewSvc, activeSvc) - updatedRolloutIndex := f.expectUpdateRolloutAction(r) + updatedRolloutIndex := f.expectUpdateRolloutStatusAction(r) // update rollout progressing condition f.expectPatchServiceAction(previewSvc, "") f.expectPatchRolloutAction(r) f.run(getKey(r, t)) @@ -981,13 +987,14 @@ func TestRequeueStuckRollout(t *testing.T) { ProgressDeadlineSeconds: progressDeadlineSeconds, }, } + r.Generation = 123 if rolloutPaused { r.Status.PauseConditions = []v1alpha1.PauseCondition{{ Reason: v1alpha1.PauseReasonBlueGreenPause, }} } if rolloutCompleted { - r.Status.ObservedGeneration = conditions.ComputeGenerationHash(r.Spec) + r.Status.ObservedGeneration = strconv.Itoa(int(r.Generation)) } if progressingConditionReason != "" { @@ -1145,7 +1152,7 @@ requests: f.serviceLister = append(f.serviceLister, activeSvc) f.objects = append(f.objects, r) - _ = f.expectUpdateRolloutAction(r) + f.expectUpdateRolloutStatusAction(r) f.expectPatchRolloutAction(r) rs := newReplicaSet(r, 1) rsIdx := f.expectCreateReplicaSetAction(rs) @@ -1181,7 +1188,7 @@ func TestComputeHashChangeTolerationBlueGreen(t *testing.T) { r.Status.AvailableReplicas = 1 r.Status.ReadyReplicas = 1 r.Status.BlueGreen.ActiveSelector = "fakepodhash" - r.Status.ObservedGeneration = "fakeobservedgeneration" + r.Status.ObservedGeneration = "122" rs := newReplicaSet(r, 1) rs.Name = "foo-fakepodhash" rs.Status.AvailableReplicas = 1 @@ -1217,10 +1224,7 @@ func TestComputeHashChangeTolerationBlueGreen(t *testing.T) { patchIndex := f.expectPatchRolloutAction(r) f.run(getKey(r, t)) - // this should only update observedGeneration and nothing else - // NOTE: This test will fail on every k8s library upgrade. - // To fix it, update expectedPatch to match the new hash. - expectedPatch := `{"status":{"observedGeneration":"6979d9866d"}}` + expectedPatch := `{"status":{"observedGeneration":"123"}}` patch := f.getPatchedRollout(patchIndex) assert.Equal(t, expectedPatch, patch) } @@ -1234,10 +1238,9 @@ func TestComputeHashChangeTolerationCanary(t *testing.T) { r.Status.CurrentPodHash = "fakepodhash" r.Status.StableRS = "fakepodhash" - r.Status.Canary.StableRS = "fakepodhash" r.Status.AvailableReplicas = 1 r.Status.ReadyReplicas = 1 - r.Status.ObservedGeneration = "fakeobservedgeneration" + r.Status.ObservedGeneration = "122" rs := newReplicaSet(r, 1) rs.Name = "foo-fakepodhash" rs.Status.AvailableReplicas = 1 @@ -1262,43 +1265,11 @@ func TestComputeHashChangeTolerationCanary(t *testing.T) { patchIndex := f.expectPatchRolloutAction(r) f.run(getKey(r, t)) - // this should only update observedGeneration and nothing else - // NOTE: This test will fail on every k8s library upgrade. - // To fix it, update expectedPatch to match the new hash. - expectedPatch := `{"status":{"observedGeneration":"6f94ff96c9"}}` + expectedPatch := `{"status":{"observedGeneration":"123"}}` patch := f.getPatchedRollout(patchIndex) assert.Equal(t, expectedPatch, patch) } -func TestMigrateCanaryStableRS(t *testing.T) { - t.Run("Copy canary.stableRS to stableRS", func(t *testing.T) { - f := newFixture(t) - defer f.Close() - r := newCanaryRollout("foo", 1, nil, nil, nil, intstr.FromInt(0), intstr.FromInt(1)) - r.Status.Canary.StableRS = "fakepodhash" - index := f.expectUpdateRolloutAction(r) - f.rolloutLister = append(f.rolloutLister, r) - f.objects = append(f.objects, r) - - f.run(getKey(r, t)) - updatedRollout := f.getUpdatedRollout(index) - assert.Equal(t, "fakepodhash", updatedRollout.Status.StableRS) - }) - t.Run("Copy StableRS to canary.stableRS", func(t *testing.T) { - f := newFixture(t) - defer f.Close() - r := newCanaryRollout("foo", 1, nil, nil, nil, intstr.FromInt(0), intstr.FromInt(1)) - r.Status.StableRS = "fakepodhash" - index := f.expectUpdateRolloutAction(r) - f.rolloutLister = append(f.rolloutLister, r) - f.objects = append(f.objects, r) - - f.run(getKey(r, t)) - updatedRollout := f.getUpdatedRollout(index) - assert.Equal(t, "fakepodhash", updatedRollout.Status.Canary.StableRS) - }) -} - func TestSwitchBlueGreenToCanary(t *testing.T) { f := newFixture(t) defer f.Close() @@ -1308,7 +1279,7 @@ func TestSwitchBlueGreenToCanary(t *testing.T) { rsPodHash := rs.Labels[v1alpha1.DefaultRolloutUniqueLabelKey] r = updateBlueGreenRolloutStatus(r, "", rsPodHash, rsPodHash, 1, 1, 1, 1, false, true) // StableRS is set to avoid running the migration code. When .status.canary.stableRS is removed, the line below can be deleted - r.Status.Canary.StableRS = rsPodHash + //r.Status.Canary.StableRS = rsPodHash r.Spec.Strategy.BlueGreen = nil r.Spec.Strategy.Canary = &v1alpha1.CanaryStrategy{ Steps: []v1alpha1.CanaryStep{{ diff --git a/rollout/experiment_test.go b/rollout/experiment_test.go index 68041fbf7f..c7079320b1 100644 --- a/rollout/experiment_test.go +++ b/rollout/experiment_test.go @@ -513,7 +513,8 @@ func TestRolloutDoNotCreateExperimentWithoutStableRS(t *testing.T) { f.objects = append(f.objects, r2) f.expectCreateReplicaSetAction(rs2) - f.expectUpdateRolloutAction(r2) + f.expectUpdateRolloutAction(r2) // update revision + f.expectUpdateRolloutStatusAction(r2) // update progressing condition f.expectPatchRolloutAction(r1) f.run(getKey(r2, t)) } diff --git a/rollout/sync.go b/rollout/sync.go index 9066b0d61e..3fd748c8f4 100644 --- a/rollout/sync.go +++ b/rollout/sync.go @@ -84,10 +84,17 @@ func (c *rolloutContext) syncReplicaSetRevision() (*appsv1.ReplicaSet, error) { } // Should use the revision in existingNewRS's annotation, since it set by before - revisionNeedsUpdate := annotations.SetRolloutRevision(c.rollout, rsCopy.Annotations[annotations.RevisionAnnotation]) - if revisionNeedsUpdate { - c.log.Infof("Updating rollout revision annotation to %s", rsCopy.Annotations[annotations.RevisionAnnotation]) + if annotations.SetRolloutRevision(c.rollout, rsCopy.Annotations[annotations.RevisionAnnotation]) { + updatedRollout, err := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(c.rollout.Namespace).Update(ctx, c.rollout, metav1.UpdateOptions{}) + if err != nil { + c.log.WithError(err).Error("Error: updating rollout revision") + return nil, err + } + c.rollout = updatedRollout + c.newRollout = updatedRollout + c.log.Infof("Updated rollout revision annotation to %s", rsCopy.Annotations[annotations.RevisionAnnotation]) } + // If no other Progressing condition has been recorded and we need to estimate the progress // of this rollout then it is likely that old users started caring about progress. In that // case we need to take into account the first time we noticed their new replica set. @@ -96,15 +103,14 @@ func (c *rolloutContext) syncReplicaSetRevision() (*appsv1.ReplicaSet, error) { msg := fmt.Sprintf(conditions.FoundNewRSMessage, rsCopy.Name) condition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionTrue, conditions.FoundNewRSReason, msg) conditions.SetRolloutCondition(&c.rollout.Status, *condition) - c.log.Infof("Initializing Progressing condition: %v", condition) - } - - if revisionNeedsUpdate || cond == nil { - _, err := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(c.rollout.Namespace).Update(ctx, c.rollout, metav1.UpdateOptions{}) + updatedRollout, err := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(c.rollout.Namespace).UpdateStatus(ctx, c.rollout, metav1.UpdateOptions{}) if err != nil { - c.log.WithError(err).Error("Error: updating rollout") + c.log.WithError(err).Error("Error: updating rollout revision") return nil, err } + c.rollout = updatedRollout + c.newRollout = updatedRollout + c.log.Infof("Initialized Progressing condition: %v", condition) } return rsCopy, nil } @@ -198,7 +204,7 @@ func (c *rolloutContext) createDesiredReplicaSet() (*appsv1.ReplicaSet, error) { *c.rollout.Status.CollisionCount++ // Update the collisionCount for the Rollout and let it requeue by returning the original // error. - _, roErr := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(c.rollout.Namespace).Update(ctx, c.rollout, metav1.UpdateOptions{}) + _, roErr := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(c.rollout.Namespace).UpdateStatus(ctx, c.rollout, metav1.UpdateOptions{}) if roErr == nil { c.log.Warnf("Found a hash collision - bumped collisionCount (%d->%d) to resolve it", preCollisionCount, *c.rollout.Status.CollisionCount) } @@ -219,16 +225,25 @@ func (c *rolloutContext) createDesiredReplicaSet() (*appsv1.ReplicaSet, error) { c.recorder.Eventf(c.rollout, corev1.EventTypeNormal, "ScalingReplicaSet", "Scaled up replica set %s to %d", createdRS.Name, newReplicasCount) } - needsUpdate := annotations.SetRolloutRevision(c.rollout, newRevision) + if annotations.SetRolloutRevision(c.rollout, newRevision) { + updatedRollout, err := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(c.rollout.Namespace).Update(ctx, c.rollout, metav1.UpdateOptions{}) + if err != nil { + return nil, err + } + c.rollout = updatedRollout + c.log.Infof("Updated rollout revision to %s", c.rollout.Annotations[annotations.RevisionAnnotation]) + } if !alreadyExists { msg := fmt.Sprintf(conditions.NewReplicaSetMessage, createdRS.Name) condition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionTrue, conditions.NewReplicaSetReason, msg) conditions.SetRolloutCondition(&c.rollout.Status, *condition) - needsUpdate = true - } - - if needsUpdate { - _, err = c.argoprojclientset.ArgoprojV1alpha1().Rollouts(c.rollout.Namespace).Update(ctx, c.rollout, metav1.UpdateOptions{}) + updatedRollout, err := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(c.rollout.Namespace).UpdateStatus(ctx, c.rollout, metav1.UpdateOptions{}) + if err != nil { + return nil, err + } + c.rollout = updatedRollout + c.newRollout = updatedRollout + c.log.Infof("Set rollout condition: %v", condition) } return createdRS, err } @@ -485,7 +500,7 @@ func (c *rolloutContext) checkPausedConditions() error { func (c *rolloutContext) patchCondition(r *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatus, condition *v1alpha1.RolloutCondition) error { ctx := context.TODO() conditions.SetRolloutCondition(newStatus, *condition) - newStatus.ObservedGeneration = conditions.ComputeGenerationHash(r.Spec) + newStatus.ObservedGeneration = strconv.Itoa(int(c.rollout.Generation)) patch, modified, err := diff.CreateTwoWayMergePatch( &v1alpha1.Rollout{ @@ -503,7 +518,7 @@ func (c *rolloutContext) patchCondition(r *v1alpha1.Rollout, newStatus *v1alpha1 return nil } c.log.Debugf("Rollout Condition Patch: %s", patch) - _, err = c.argoprojclientset.ArgoprojV1alpha1().Rollouts(r.Namespace).Patch(ctx, r.Name, patchtypes.MergePatchType, patch, metav1.PatchOptions{}) + _, err = c.argoprojclientset.ArgoprojV1alpha1().Rollouts(r.Namespace).Patch(ctx, r.Name, patchtypes.MergePatchType, patch, metav1.PatchOptions{}, "status") if err != nil { c.log.Warnf("Error patching rollout: %v", err) return err @@ -610,7 +625,7 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt func (c *rolloutContext) persistRolloutStatus(newStatus *v1alpha1.RolloutStatus) error { ctx := context.TODO() c.pauseContext.CalculatePauseStatus(newStatus) - newStatus.ObservedGeneration = conditions.ComputeGenerationHash(c.rollout.Spec) + newStatus.ObservedGeneration = strconv.Itoa(int(c.rollout.Generation)) patch, modified, err := diff.CreateTwoWayMergePatch( &v1alpha1.Rollout{ Status: c.rollout.Status, @@ -627,7 +642,7 @@ func (c *rolloutContext) persistRolloutStatus(newStatus *v1alpha1.RolloutStatus) c.requeueStuckRollout(*newStatus) return nil } - newRollout, err := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(c.rollout.Namespace).Patch(ctx, c.rollout.Name, patchtypes.MergePatchType, patch, metav1.PatchOptions{}) + newRollout, err := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(c.rollout.Namespace).Patch(ctx, c.rollout.Name, patchtypes.MergePatchType, patch, metav1.PatchOptions{}, "status") if err != nil { c.log.Warningf("Error updating application: %v", err) return err diff --git a/test/e2e/analysis_test.go b/test/e2e/analysis_test.go index 78868d3fe5..f7a8855926 100644 --- a/test/e2e/analysis_test.go +++ b/test/e2e/analysis_test.go @@ -168,7 +168,8 @@ spec: ExpectPreviewRevision("2"). When(). ApplyManifests(original). // perform a rollback and make sure we skip pause/analysis - Sleep(2 * time.Second). // checking too early may not catch the bug + Sleep(2 * time.Second). // checking too early may not catch the bug where we create analysis unnecessarily + //WaitForRolloutStatus("Healthy", 2*time.Second). // rollout is Progressing->Healthy immediately Then(). ExpectRolloutStatus("Healthy"). // rollout is healthy immediately ExpectAnalysisRunCount(2). // no new analysis runs created diff --git a/test/fixtures/then.go b/test/fixtures/then.go index 0c92f6c518..1f98fe7008 100644 --- a/test/fixtures/then.go +++ b/test/fixtures/then.go @@ -98,7 +98,7 @@ func (t *Then) ExpectCanaryStablePodCount(canaryCount, stableCount int) *Then { ro, err := t.rolloutClient.ArgoprojV1alpha1().Rollouts(t.namespace).Get(t.Context, t.rollout.GetName(), metav1.GetOptions{}) t.CheckError(err) return t.expectPodCountByHash("canary", ro.Status.CurrentPodHash, canaryCount). - expectPodCountByHash("stable", ro.Status.Canary.StableRS, stableCount) + expectPodCountByHash("stable", ro.Status.StableRS, stableCount) } func (t *Then) ExpectRevisionPodCount(revision string, expectedCount int) *Then { diff --git a/test/fixtures/when.go b/test/fixtures/when.go index a1d8bec526..b7c048c0d9 100644 --- a/test/fixtures/when.go +++ b/test/fixtures/when.go @@ -177,15 +177,15 @@ func (w *When) PatchSpec(patch string) *When { return w } -func (w *When) WaitForRolloutStatus(status string) *When { +func (w *When) WaitForRolloutStatus(status string, timeout ...time.Duration) *When { checkStatus := func(ro *rov1.Rollout) bool { s, _ := info.RolloutStatusString(ro) return s == status } - return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("status=%s", status), E2EWaitTimeout) + return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("status=%s", status), timeout...) } -func (w *When) WaitForRolloutCanaryStepIndex(index int32) *When { +func (w *When) WaitForRolloutCanaryStepIndex(index int32, timeout ...time.Duration) *When { checkStatus := func(ro *rov1.Rollout) bool { if ro.Status.CurrentStepIndex == nil || *ro.Status.CurrentStepIndex != index { return false @@ -204,32 +204,32 @@ func (w *When) WaitForRolloutCanaryStepIndex(index int32) *When { } return true } - return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("status.currentStepIndex=%d", index), E2EWaitTimeout) + return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("status.currentStepIndex=%d", index), timeout...) } -func (w *When) WaitForRolloutAvailableReplicas(count int32) *When { +func (w *When) WaitForRolloutAvailableReplicas(count int32, timeout ...time.Duration) *When { checkStatus := func(ro *rov1.Rollout) bool { return ro.Status.AvailableReplicas == count } - return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("status.availableReplicas=%d", count), E2EWaitTimeout) + return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("status.availableReplicas=%d", count), timeout...) } -func (w *When) WaitForRolloutReplicas(count int32) *When { +func (w *When) WaitForRolloutReplicas(count int32, timeout ...time.Duration) *When { checkStatus := func(ro *rov1.Rollout) bool { return ro.Status.Replicas == count } - return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("status.replicas=%d", count), E2EWaitTimeout) + return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("status.replicas=%d", count), timeout...) } -func (w *When) WaitForActiveRevision(revision string) *When { +func (w *When) WaitForActiveRevision(revision string, timeout ...time.Duration) *When { rs := w.GetReplicaSetByRevision(revision) checkStatus := func(ro *rov1.Rollout) bool { return ro.Status.BlueGreen.ActiveSelector == rs.Labels[rov1.DefaultRolloutUniqueLabelKey] } - return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("active revision=%s", revision), E2EWaitTimeout) + return w.WaitForRolloutCondition(checkStatus, fmt.Sprintf("active revision=%s", revision), timeout...) } -func (w *When) WaitForRolloutCondition(test func(ro *rov1.Rollout) bool, condition string, timeout time.Duration) *When { +func (w *When) WaitForRolloutCondition(test func(ro *rov1.Rollout) bool, condition string, timeouts ...time.Duration) *When { start := time.Now() w.log.Infof("Waiting for condition: %s", condition) rolloutIf := w.dynamicClient.Resource(rov1.RolloutGVR).Namespace(w.namespace) @@ -243,6 +243,10 @@ func (w *When) WaitForRolloutCondition(test func(ro *rov1.Rollout) bool, conditi }) w.CheckError(err) defer retryWatcher.Stop() + timeout := E2EWaitTimeout + if len(timeouts) > 0 { + timeout = timeouts[0] + } timeoutCh := make(chan bool, 1) go func() { time.Sleep(timeout) diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index 84dad14c4f..a9338d20f7 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -4,13 +4,13 @@ import ( "encoding/json" "fmt" "hash/fnv" + "strconv" "time" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/rand" - hashutil "k8s.io/kubernetes/pkg/util/hash" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/argoproj/argo-rollouts/utils/defaults" @@ -231,7 +231,7 @@ func RolloutComplete(rollout *v1alpha1.Rollout, newStatus *v1alpha1.RolloutStatu return newStatus.UpdatedReplicas == replicas && newStatus.AvailableReplicas == replicas && - rollout.Status.ObservedGeneration == ComputeGenerationHash(rollout.Spec) && + rollout.Status.ObservedGeneration == strconv.Itoa(int(rollout.Generation)) && completedStrategy } @@ -253,14 +253,6 @@ func ComputeStepHash(rollout *v1alpha1.Rollout) string { return rand.SafeEncodeString(fmt.Sprint(rolloutStepHasher.Sum32())) } -// ComputeGenerationHash returns a hash value calculated from the Rollout Spec. The hash will -// be safe encoded to avoid bad words. -func ComputeGenerationHash(spec v1alpha1.RolloutSpec) string { - rolloutSpecHasher := fnv.New32a() - hashutil.DeepHashObject(rolloutSpecHasher, spec) - return rand.SafeEncodeString(fmt.Sprint(rolloutSpecHasher.Sum32())) -} - // RolloutTimedOut considers a rollout to have timed out once its condition that reports progress // is older than progressDeadlineSeconds or a Progressing condition with a TimedOutReason reason already // exists. diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index 6fb734c6ca..8a64e2b0eb 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -1,6 +1,7 @@ package conditions import ( + "strconv" "testing" "time" @@ -361,6 +362,7 @@ func TestRolloutComplete(t *testing.T) { AvailableReplicas: available, }, } + r.Generation = 123 podHash := controller.ComputeHash(&r.Spec.Template, r.Status.CollisionCount) r.Status.CurrentPodHash = podHash return r @@ -377,7 +379,7 @@ func TestRolloutComplete(t *testing.T) { r.Status.BlueGreen.ActiveSelector = activeSelector r.Status.BlueGreen.PreviewSelector = previewSelector if correctObservedGeneration { - r.Status.ObservedGeneration = ComputeGenerationHash(r.Spec) + r.Status.ObservedGeneration = strconv.Itoa(int(r.Generation)) } return r } @@ -396,7 +398,7 @@ func TestRolloutComplete(t *testing.T) { r.Status.StableRS = stableRS r.Status.CurrentStepIndex = stepIndex if correctObservedGeneration { - r.Status.ObservedGeneration = ComputeGenerationHash(r.Spec) + r.Status.ObservedGeneration = strconv.Itoa(int(r.Generation)) } return r } @@ -543,27 +545,6 @@ func TestRolloutTimedOut(t *testing.T) { } } -func TestComputeGenerationHash(t *testing.T) { - ro := &v1alpha1.Rollout{ - Spec: v1alpha1.RolloutSpec{ - Replicas: pointer.Int32Ptr(10), - Template: v1.PodTemplateSpec{ - Spec: v1.PodSpec{ - Containers: []v1.Container{{ - Image: "name", - }}, - }, - }, - }, - } - baseline := ComputeGenerationHash(ro.Spec) - roPaused := ro.DeepCopy() - roPaused.Spec.Paused = true - roPausedHash := ComputeGenerationHash(roPaused.Spec) - - assert.NotEqual(t, baseline, roPausedHash) -} - // TestComputeStableStepHash verifies we generate different hashes for various step definitions. // Also verifies we do not unintentionally break our ComputeStepHash function somehow (e.g. by // modifying types or change libraries) diff --git a/utils/replicaset/canary_test.go b/utils/replicaset/canary_test.go index 80aaf1ab0e..2c9d68e498 100644 --- a/utils/replicaset/canary_test.go +++ b/utils/replicaset/canary_test.go @@ -39,9 +39,7 @@ func newRollout( }, }, Status: v1alpha1.RolloutStatus{ - Canary: v1alpha1.CanaryStatus{ - StableRS: stablePodHash, - }, + StableRS: stablePodHash, CurrentPodHash: currentPodHash, }, }