From c8d7de4a7ce26b5a84df0f77daf8ad9e7c93073f Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Wed, 20 Feb 2019 07:28:54 -0800 Subject: [PATCH] Clean up max surge and unavailable - add missing omitempty flags - change defaults to deployment defaults - fix flaky test --- controller/canary_test.go | 4 ---- examples/example-rollout.yaml | 8 +++++--- manifests/crds/rollout-crd.yaml | 1 + manifests/install.yaml | 1 + manifests/namespace-install.yaml | 1 + pkg/apis/rollouts/v1alpha1/openapi_generated.go | 9 ++++++++- pkg/apis/rollouts/v1alpha1/types.go | 4 ++-- pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go | 6 +++++- utils/defaults/defaults.go | 6 +++--- utils/defaults/defaults_test.go | 2 +- 10 files changed, 27 insertions(+), 15 deletions(-) diff --git a/controller/canary_test.go b/controller/canary_test.go index 0164869fa6..9b16614286 100644 --- a/controller/canary_test.go +++ b/controller/canary_test.go @@ -550,10 +550,6 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { f.expectPatchRolloutAction(r2) f.run(getKey(r2, t)) - key := fmt.Sprintf("%s/%s", r2.Namespace, r2.Name) - //When the controller starts, it will enqueue the rollout so we expect the rollout to enqueue at least once. - assert.Equal(t, 1, f.enqueuedObjects[key]) - patchBytes := filterInformerActions(f.client.Actions())[0].(core.PatchAction).GetPatch() assert.Equal(t, expectedPatch, string(patchBytes)) } diff --git a/examples/example-rollout.yaml b/examples/example-rollout.yaml index d5fab9e455..2a1e9600ca 100644 --- a/examples/example-rollout.yaml +++ b/examples/example-rollout.yaml @@ -3,7 +3,7 @@ kind: Rollout metadata: name: example-rollout spec: - replicas: 1 + replicas: 5 selector: matchLabels: app: guestbook @@ -24,5 +24,7 @@ spec: type: CanaryUpdate canary: steps: - - pause: true - - pause: true + - setWeight: 20 + - wait: 20 + - setWeight: 40 + - pause: {} diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index 2f24338066..390b57a484 100644 --- a/manifests/crds/rollout-crd.yaml +++ b/manifests/crds/rollout-crd.yaml @@ -121,6 +121,7 @@ spec: description: StableRS indicates the last replicaset that walked through all the canary steps or was the only replicaset type: string + waitStartTime: {} collisionCount: description: Count of hash collisions for the Rollout. The Rollout controller uses this field as a collision avoidance mechanism when it needs to diff --git a/manifests/install.yaml b/manifests/install.yaml index 21a5809e37..a085b2d050 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -122,6 +122,7 @@ spec: description: StableRS indicates the last replicaset that walked through all the canary steps or was the only replicaset type: string + waitStartTime: {} collisionCount: description: Count of hash collisions for the Rollout. The Rollout controller uses this field as a collision avoidance mechanism when it needs to diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 5ed693715c..abffb44bcf 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -122,6 +122,7 @@ spec: description: StableRS indicates the last replicaset that walked through all the canary steps or was the only replicaset type: string + waitStartTime: {} collisionCount: description: Count of hash collisions for the Rollout. The Rollout controller uses this field as a collision avoidance mechanism when it needs to diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go index 50580157c6..4bd9612061 100644 --- a/pkg/apis/rollouts/v1alpha1/openapi_generated.go +++ b/pkg/apis/rollouts/v1alpha1/openapi_generated.go @@ -84,10 +84,17 @@ func schema_pkg_apis_rollouts_v1alpha1_CanaryStatus(ref common.ReferenceCallback Format: "", }, }, + "waitStartTime": { + SchemaProps: spec.SchemaProps{ + Description: "WaitStartTime this field is set when the rollout is in a wait step and indicates the time the wait started at", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1.Time"), + }, + }, }, }, }, - Dependencies: []string{}, + Dependencies: []string{ + "k8s.io/apimachinery/pkg/apis/meta/v1.Time"}, } } diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index b0f3132ad0..e745d36ad8 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -98,7 +98,7 @@ type CanaryStrategy struct { // that at least 70% of original number of pods are available at all times // during the update. // +optional - MaxUnavailable *intstr.IntOrString `json:"maxUnavailable"` + MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` // MaxSurge The maximum number of pods that can be scheduled above the original number of // pods. @@ -111,7 +111,7 @@ type CanaryStrategy struct { // new RC can be scaled up further, ensuring that total number of pods running // at any time during the update is atmost 130% of original pods. // +optional - MaxSurge *intstr.IntOrString `json:"maxSurge"` + MaxSurge *intstr.IntOrString `json:"maxSurge,omitempty"` } // CanaryStep defines a step of a canary deployment. diff --git a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go index cff6a5db53..18581fece2 100644 --- a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go @@ -45,6 +45,10 @@ func (in *BlueGreenStrategy) DeepCopy() *BlueGreenStrategy { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CanaryStatus) DeepCopyInto(out *CanaryStatus) { *out = *in + if in.WaitStartTime != nil { + in, out := &in.WaitStartTime, &out.WaitStartTime + *out = (*in).DeepCopy() + } return } @@ -280,7 +284,7 @@ func (in *RolloutStatus) DeepCopyInto(out *RolloutStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } - out.CanaryStatus = in.CanaryStatus + in.CanaryStatus.DeepCopyInto(&out.CanaryStatus) return } diff --git a/utils/defaults/defaults.go b/utils/defaults/defaults.go index c6d16e851c..edc541135e 100644 --- a/utils/defaults/defaults.go +++ b/utils/defaults/defaults.go @@ -12,9 +12,9 @@ const ( // DefaultRevisionHistoryLimit default number of revisions to keep if .Spec.RevisionHistoryLimit is nil DefaultRevisionHistoryLimit = int32(10) // DefaultMaxSurge default number for the max number of additional pods that can be brought up during a rollout - DefaultMaxSurge = 1 + DefaultMaxSurge = "25" // DefaultMaxUnavailable default number for the max number of unavailable pods during a rollout - DefaultMaxUnavailable = 1 + DefaultMaxUnavailable = 0 ) // GetRolloutReplicasOrDefault returns the specified number of replicas in a rollout or the default number @@ -37,7 +37,7 @@ func GetMaxSurgeOrDefault(rollout *v1alpha1.Rollout) *intstr.IntOrString { if rollout.Spec.Strategy.CanaryStrategy != nil && rollout.Spec.Strategy.CanaryStrategy.MaxSurge != nil { return rollout.Spec.Strategy.CanaryStrategy.MaxSurge } - defaultValue := intstr.FromInt(DefaultMaxSurge) + defaultValue := intstr.FromString(DefaultMaxSurge) return &defaultValue } diff --git a/utils/defaults/defaults_test.go b/utils/defaults/defaults_test.go index d4ebda3af7..ff4b6c0d11 100644 --- a/utils/defaults/defaults_test.go +++ b/utils/defaults/defaults_test.go @@ -49,7 +49,7 @@ func TestGetMaxSurgeOrDefault(t *testing.T) { assert.Equal(t, maxSurge, *GetMaxSurgeOrDefault(rolloutNonDefaultValue)) rolloutDefaultValue := &v1alpha1.Rollout{} - assert.Equal(t, intstr.FromInt(DefaultMaxSurge), *GetMaxSurgeOrDefault(rolloutDefaultValue)) + assert.Equal(t, intstr.FromString(DefaultMaxSurge), *GetMaxSurgeOrDefault(rolloutDefaultValue)) } func TestGetMaxUnavailableOrDefault(t *testing.T) {