From bcd57dbd73d7a6b5021777a50021e4bc8c8acf0e Mon Sep 17 00:00:00 2001 From: Kyle Cronin Date: Sun, 1 Mar 2020 13:16:08 -0500 Subject: [PATCH 1/3] feat: pause duration as string with time unit Support declaring pause duration as string with optional time unit (i.e. "s", "m", "h"). If no time unit or value is an integer, seconds are inferred. closes #292 --- docs/features/canary.md | 30 ++++++++++++-- docs/features/index.md | 2 +- hack/update-codegen.sh | 5 +-- hack/update-openapigen.sh | 3 +- manifests/crds/rollout-crd.yaml | 5 ++- manifests/install.yaml | 5 ++- manifests/namespace-install.yaml | 5 ++- .../rollouts/v1alpha1/openapi_generated.go | 5 ++- pkg/apis/rollouts/v1alpha1/types.go | 40 ++++++++++++++++++- pkg/apis/rollouts/v1alpha1/types_test.go | 24 +++++++++++ .../v1alpha1/zz_generated.deepcopy.go | 2 +- .../cmd/list/list_test.go | 2 +- pkg/kubectl-argo-rollouts/info/info_test.go | 2 +- rollout/canary.go | 2 +- rollout/canary_test.go | 12 +++--- rollout/pause.go | 2 +- utils/conditions/conditions.go | 2 +- utils/conditions/rollouts_test.go | 14 ++++++- 18 files changed, 132 insertions(+), 30 deletions(-) create mode 100644 pkg/apis/rollouts/v1alpha1/types_test.go diff --git a/docs/features/canary.md b/docs/features/canary.md index 723194c433..545bab1cc6 100644 --- a/docs/features/canary.md +++ b/docs/features/canary.md @@ -36,9 +36,32 @@ spec: steps: - setWeight: 10 - pause: - duration: 3600 # 1 hour + duration: "1h" # 1 hour - setWeight: 20 - - pause: {} + - pause: {} # pause indefinitely +``` + +### Pause Duration +Pause duration can be specied with an optional time unit suffix. Valid time units are "s", "m", "h". Defaults to "s" if not specified. Values less than zero are not allowed. + +```yaml +spec: + strategy: + canary: + steps: + - pause: { duration: 10 } # 10 seconds + - pause: { duration: 10s } # 10 seconds + - pause: { duration: 10m } # 10 minutes + - pause: { duration: 10h } # 10 hours + - pause: { duration: -10 } # invalid spec! + - pause: {} # pause indefinitely +``` + +If no `duration` specified for a pause step the rollout will be paused indefinitely. To unpause use the [argo kubectl plugin](kubectl-plugin.md) `promote` command. + +```shell +# promote to the next step +kubectl argo rollouts promote ``` ## Mimicking Rolling Update @@ -59,11 +82,12 @@ spec: `maxSurge` defines the maximum number of replicas the rollout can create to move to the correct ratio set by the last setWeight. Max Surge can either be an integer or percentage as a string (i.e. "20%") Defaults to "25%". + ### maxUnavailable The maximum number of pods that can be unavailable during the update. Value can be an absolute number (ex: 5) or a percentage of desired pods (ex: 10%). This can not be 0 if MaxSurge is 0. // Defaults to 0 -### CanaryService +### canaryService `canaryService` references a Service that will be modified to send traffic to only the canary ReplicaSet. This allows users to only hit the canary ReplicaSet. Defaults to an empty string diff --git a/docs/features/index.md b/docs/features/index.md index 873200b74d..c63e479771 100644 --- a/docs/features/index.md +++ b/docs/features/index.md @@ -63,7 +63,7 @@ spec: - setWeight: 20 # Pauses the rollout for an hour - pause: - duration: 3600 # One hour + duration: "1h" # One hour - setWeight: 40 # Sets .spec.paused to true and waits until the field is changed back - pause: {} diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh index b91eb95dbc..0c13cc3324 100755 --- a/hack/update-codegen.sh +++ b/hack/update-codegen.sh @@ -6,11 +6,10 @@ set -o pipefail # code-generator does work with go.mod but makes assumptions about the project living in `$GOPATH/src`. # To work around this and support any location: # create a temporary directory, use this as an output base, and copy everything back once generated. - +export GOPATH=$(go env GOPATH) # export gopath so it's available to generate scripts SCRIPT_ROOT="$( cd "$( dirname "${BASH_SOURCE[0]}" )/.." >/dev/null 2>&1 && pwd )" CODEGEN_VERSION=$(go list -m k8s.io/code-generator | awk '{print $2}' | head -1) -CODEGEN_PKG=$(echo `go env GOPATH`"/pkg/mod/k8s.io/code-generator@${CODEGEN_VERSION}") - +CODEGEN_PKG="${GOPATH}/pkg/mod/k8s.io/code-generator@${CODEGEN_VERSION}" TEMP_DIR=$(mktemp -d) cleanup() { rm -rf ${TEMP_DIR} diff --git a/hack/update-openapigen.sh b/hack/update-openapigen.sh index 4e8d5e4679..28c94cb02b 100755 --- a/hack/update-openapigen.sh +++ b/hack/update-openapigen.sh @@ -4,9 +4,10 @@ set -o errexit set -o nounset set -o pipefail +export GOPATH=$(go env GOPATH) # export gopath do generator output goes to the correct location PROJECT_ROOT=$(cd $(dirname "$0")/.. ; pwd) CODEGEN_VERSION=$(go list -m k8s.io/kube-openapi | awk '{print $2}' | head -1) -CODEGEN_PKG=$(echo `go env GOPATH`"/pkg/mod/k8s.io/kube-openapi@${CODEGEN_VERSION}") +CODEGEN_PKG="${GOPATH}/pkg/mod/k8s.io/kube-openapi@${CODEGEN_VERSION}" VERSION="v1alpha1" go run ${CODEGEN_PKG}/cmd/openapi-gen/openapi-gen.go \ diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index 37fb7052f4..fa2d5fe428 100644 --- a/manifests/crds/rollout-crd.yaml +++ b/manifests/crds/rollout-crd.yaml @@ -276,8 +276,9 @@ spec: pause: properties: duration: - format: int32 - type: integer + anyOf: + - type: integer + - type: string type: object setWeight: format: int32 diff --git a/manifests/install.yaml b/manifests/install.yaml index 5ce0c49edc..d420f93b3e 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -8251,8 +8251,9 @@ spec: pause: properties: duration: - format: int32 - type: integer + anyOf: + - type: integer + - type: string type: object setWeight: format: int32 diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index c3992fecfd..1bdcef45f8 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -8251,8 +8251,9 @@ spec: pause: properties: duration: - format: int32 - type: integer + anyOf: + - type: integer + - type: string type: object setWeight: format: int32 diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go index 5d638d5e8a..82534dace3 100644 --- a/pkg/apis/rollouts/v1alpha1/openapi_generated.go +++ b/pkg/apis/rollouts/v1alpha1/openapi_generated.go @@ -2211,13 +2211,14 @@ func schema_pkg_apis_rollouts_v1alpha1_RolloutPause(ref common.ReferenceCallback "duration": { SchemaProps: spec.SchemaProps{ Description: "Duration the amount of time to wait before moving to the next step.", - Type: []string{"integer"}, - Format: "int32", + Ref: ref("k8s.io/apimachinery/pkg/util/intstr.IntOrString"), }, }, }, }, }, + Dependencies: []string{ + "k8s.io/apimachinery/pkg/util/intstr.IntOrString"}, } } diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 2c72c48cc5..1e4a906037 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -1,6 +1,9 @@ package v1alpha1 import ( + "strconv" + "time" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -321,7 +324,42 @@ const ( type RolloutPause struct { // Duration the amount of time to wait before moving to the next step. // +optional - Duration *int32 `json:"duration,omitempty"` + Duration *intstr.IntOrString `json:"duration,omitempty"` +} + +// DurationSeconds converts the pause duration to seconds +// If Duration is nil 0 is returned +// if Duration values is string and does not contain a valid unit -1 is returned +func (p RolloutPause) DurationSeconds() int32 { + if p.Duration != nil { + if p.Duration.Type == intstr.String { + s, err := strconv.Atoi(p.Duration.StrVal) + if err != nil { + d, err := time.ParseDuration(p.Duration.StrVal) + if err != nil { + return -1 + } + return int32(d.Seconds()) + } + // special case where no unit was specified + return int32(s) + } + return int32(p.Duration.IntVal) + } + return 0 +} + +// DurationFromInt creates duration in seconds from int value +func DurationFromInt(i int) *intstr.IntOrString { + d := intstr.FromInt(i) + return &d +} + +// DurationFromString creates duration from string +// value must be a string representation of an int with optional time unit (see time.ParseDuration) +func DurationFromString(s string) *intstr.IntOrString { + d := intstr.FromString(s) + return &d } // PauseReason reasons that the rollout can pause diff --git a/pkg/apis/rollouts/v1alpha1/types_test.go b/pkg/apis/rollouts/v1alpha1/types_test.go new file mode 100644 index 0000000000..e4be26b521 --- /dev/null +++ b/pkg/apis/rollouts/v1alpha1/types_test.go @@ -0,0 +1,24 @@ +package v1alpha1 + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRolloutPauseDuration(t *testing.T) { + rp := RolloutPause{} + assert.Equal(t, int32(0), rp.DurationSeconds()) + rp.Duration = DurationFromInt(10) + assert.Equal(t, int32(10), rp.DurationSeconds()) + rp.Duration = DurationFromString("10") + assert.Equal(t, int32(10), rp.DurationSeconds()) + rp.Duration = DurationFromString("10s") + assert.Equal(t, int32(10), rp.DurationSeconds()) + rp.Duration = DurationFromString("1h") + assert.Equal(t, int32(3600), rp.DurationSeconds()) + rp.Duration = DurationFromString("1ms") + assert.Equal(t, int32(0), rp.DurationSeconds()) + rp.Duration = DurationFromString("1z") + assert.Equal(t, int32(-1), rp.DurationSeconds()) +} diff --git a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go index e4c3021063..58320566cb 100644 --- a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go @@ -1171,7 +1171,7 @@ func (in *RolloutPause) DeepCopyInto(out *RolloutPause) { *out = *in if in.Duration != nil { in, out := &in.Duration, &out.Duration - *out = new(int32) + *out = new(intstr.IntOrString) **out = **in } return diff --git a/pkg/kubectl-argo-rollouts/cmd/list/list_test.go b/pkg/kubectl-argo-rollouts/cmd/list/list_test.go index b2c911d2b4..783358c4a4 100644 --- a/pkg/kubectl-argo-rollouts/cmd/list/list_test.go +++ b/pkg/kubectl-argo-rollouts/cmd/list/list_test.go @@ -35,7 +35,7 @@ func newCanaryRollout() *v1alpha1.Rollout { }, { Pause: &v1alpha1.RolloutPause{ - Duration: pointer.Int32Ptr(60), + Duration: v1alpha1.DurationFromInt(60), }, }, { diff --git a/pkg/kubectl-argo-rollouts/info/info_test.go b/pkg/kubectl-argo-rollouts/info/info_test.go index e80d84e78f..e8850056d1 100644 --- a/pkg/kubectl-argo-rollouts/info/info_test.go +++ b/pkg/kubectl-argo-rollouts/info/info_test.go @@ -28,7 +28,7 @@ func newCanaryRollout() *v1alpha1.Rollout { }, { Pause: &v1alpha1.RolloutPause{ - Duration: pointer.Int32Ptr(60), + Duration: v1alpha1.DurationFromInt(60), }, }, { diff --git a/rollout/canary.go b/rollout/canary.go index 88b8e540a8..5e264381f3 100644 --- a/rollout/canary.go +++ b/rollout/canary.go @@ -147,7 +147,7 @@ func (c *RolloutController) reconcileCanaryPause(roCtx *canaryContext) bool { if currentStep.Pause.Duration == nil { return true } - c.checkEnqueueRolloutDuringWait(rollout, cond.StartTime, *currentStep.Pause.Duration) + c.checkEnqueueRolloutDuringWait(rollout, cond.StartTime, currentStep.Pause.DurationSeconds()) return true } diff --git a/rollout/canary_test.go b/rollout/canary_test.go index fa00070631..5c0d0298e0 100644 --- a/rollout/canary_test.go +++ b/rollout/canary_test.go @@ -798,7 +798,7 @@ func TestSyncRolloutWaitAddToQueue(t *testing.T) { SetWeight: int32Ptr(10), }, { Pause: &v1alpha1.RolloutPause{ - Duration: int32Ptr(10), + Duration: v1alpha1.DurationFromInt(10), }, }, } @@ -838,7 +838,7 @@ func TestSyncRolloutIgnoreWaitOutsideOfReconciliationPeriod(t *testing.T) { }, { Pause: &v1alpha1.RolloutPause{ - Duration: int32Ptr(int32(3600)), //1 hour + Duration: v1alpha1.DurationFromInt(3600), //1 hour }, }, } @@ -877,7 +877,7 @@ func TestSyncRolloutWaitIncrementStepIndex(t *testing.T) { }, { Pause: &v1alpha1.RolloutPause{ - Duration: int32Ptr(5), + Duration: v1alpha1.DurationFromInt(5), }, }, { Pause: &v1alpha1.RolloutPause{}, @@ -1111,7 +1111,7 @@ func TestResumeRolloutAfterPauseDuration(t *testing.T) { }, { Pause: &v1alpha1.RolloutPause{ - Duration: pointer.Int32Ptr(60), + Duration: v1alpha1.DurationFromInt(60), }, }, { @@ -1159,7 +1159,7 @@ func TestNoResumeAfterPauseDurationIfUserPaused(t *testing.T) { }, { Pause: &v1alpha1.RolloutPause{ - Duration: pointer.Int32Ptr(60), + Duration: v1alpha1.DurationFromInt(60), }, }, { @@ -1200,7 +1200,7 @@ func TestHandleNilNewRSOnScaleAndImageChange(t *testing.T) { }, { Pause: &v1alpha1.RolloutPause{ - Duration: pointer.Int32Ptr(60), + Duration: v1alpha1.DurationFromInt(60), }, }, { diff --git a/rollout/pause.go b/rollout/pause.go index d7331ed711..120ad338eb 100644 --- a/rollout/pause.go +++ b/rollout/pause.go @@ -143,7 +143,7 @@ func (pCtx *pauseContext) CompletedPauseStep(pause v1alpha1.RolloutPause) bool { if pause.Duration != nil { now := metav1.Now() if pauseCondition != nil { - expiredTime := pauseCondition.StartTime.Add(time.Duration(*pause.Duration) * time.Second) + expiredTime := pauseCondition.StartTime.Add(time.Duration(pause.DurationSeconds()) * time.Second) if now.After(expiredTime) { pCtx.log.Info("Rollout has waited the duration of the pause step") return true diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go index e2a6e3a709..64dbfc92a6 100644 --- a/utils/conditions/conditions.go +++ b/utils/conditions/conditions.go @@ -339,7 +339,7 @@ func VerifyRolloutSpec(rollout *v1alpha1.Rollout, prevCond *v1alpha1.RolloutCond if step.SetWeight != nil && (*step.SetWeight < 0 || *step.SetWeight > 100) { return newInvalidSpecRolloutCondition(prevCond, InvalidSpecReason, InvalidSetWeightMessage) } - if step.Pause != nil && step.Pause.Duration != nil && *step.Pause.Duration < 0 { + if step.Pause != nil && step.Pause.DurationSeconds() < 0 { return newInvalidSpecRolloutCondition(prevCond, InvalidSpecReason, InvalidDurationMessage) } } diff --git a/utils/conditions/rollouts_test.go b/utils/conditions/rollouts_test.go index 46fad1e73c..e4a0108d65 100644 --- a/utils/conditions/rollouts_test.go +++ b/utils/conditions/rollouts_test.go @@ -368,7 +368,19 @@ func TestVerifyRolloutSpecCanary(t *testing.T) { name: "Pause duration is not less than 0", steps: []v1alpha1.CanaryStep{{ Pause: &v1alpha1.RolloutPause{ - Duration: pointer.Int32Ptr(-1), + Duration: v1alpha1.DurationFromInt(-1), + }, + }}, + + notValid: true, + reason: InvalidSpecReason, + message: InvalidDurationMessage, + }, + { + name: "Pause duration invalid unit", + steps: []v1alpha1.CanaryStep{{ + Pause: &v1alpha1.RolloutPause{ + Duration: v1alpha1.DurationFromString("10z"), }, }}, From 8f583c3ea9e3d0df0922962090d8f03c1fd25684 Mon Sep 17 00:00:00 2001 From: Kyle Cronin Date: Sun, 1 Mar 2020 13:27:20 -0500 Subject: [PATCH 2/3] fix: unnecessary int32 conversion --- pkg/apis/rollouts/v1alpha1/types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 1e4a906037..47fea15cdb 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -344,7 +344,7 @@ func (p RolloutPause) DurationSeconds() int32 { // special case where no unit was specified return int32(s) } - return int32(p.Duration.IntVal) + return p.Duration.IntVal } return 0 } From 8402c0d6dc3e987de346eb38f30f540f0dd08be9 Mon Sep 17 00:00:00 2001 From: Kyle Cronin Date: Wed, 4 Mar 2020 00:18:03 -0500 Subject: [PATCH 3/3] ignore generated code from codecov --- .codecov.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.codecov.yml b/.codecov.yml index 4d891c5fa1..6ca556d484 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -6,4 +6,6 @@ coverage: threshold: 0.1 project: default: - threshold: 0.1 \ No newline at end of file + threshold: 0.1 +ignore: +- "pkg/apis/rollouts/v1alpha1" \ No newline at end of file