Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow jumping between canary steps #216

Closed
wants to merge 9 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
nextStepIndex default value from 0 to -1
Signed-off-by: yunbo <yunbo10124scut@gmail.com>
Funinu committed Jun 7, 2024
commit 3b034869e5165dcf36edbd34c047b19cd865b0b6
2 changes: 0 additions & 2 deletions api/v1alpha1/deployment_types.go
Original file line number Diff line number Diff line change
@@ -61,8 +61,6 @@ const (
CanaryRollingStyle RollingStyleType = "Canary"
// BlueGreenRollingStyle means rolling in blue-green way, and will NOT create a canary Deployment.
BlueGreenRollingStyle RollingStyleType = "BlueGreen"
// Empty means both Canary and BlueGreen are empty
EmptyRollingStyle RollingStyleType = "Empty"
)

// DeploymentExtraStatus is extra status field for Advanced Deployment
18 changes: 8 additions & 10 deletions api/v1beta1/deployment_types.go
Original file line number Diff line number Diff line change
@@ -38,9 +38,9 @@ const (
// which labels whether the deployment is controlled by advanced-deployment-controller.
AdvancedDeploymentControlLabel = "rollouts.kruise.io/controlled-by-advanced-deployment-controller"

// OriginalSettingAnnotation is annotation for workload in BlueGreen Release,
// it will storage the original setting of the workload, which will be used to restore the workload
OriginalSettingAnnotation = "rollouts.kruise.io/original-setting"
// OriginalDeploymentStrategyAnnotation is annotation for workload in BlueGreen Release,
// it will store the original setting of the workload, which will be used to restore the workload
OriginalDeploymentStrategyAnnotation = "rollouts.kruise.io/original-deployment-strategy"

// MaxProgressSeconds is the value we set for ProgressDeadlineSeconds
// MaxReadySeconds is the value we set for MinReadySeconds, which is one less than ProgressDeadlineSeconds
@@ -62,12 +62,12 @@ type DeploymentStrategy struct {
Partition intstr.IntOrString `json:"partition,omitempty"`
}

// OriginalSetting storages part of the fileds of a workload,
// OriginalDeploymentStrategy stores part of the fileds of a workload,
// so that it can be restored when finalizing.
// It is only used for BlueGreen Release
// Similar to DeploymentStrategy, it is stored in workload annotation
// However, unlike DeploymentStrategy, it is only used to storage and restore
type OriginalSetting struct {
// Similar to DeploymentStrategy, it is an annotation used in workload
// However, unlike DeploymentStrategy, it is only used to store and restore the user's strategy
type OriginalDeploymentStrategy struct {
// The deployment strategy to use to replace existing pods with new ones.
// +optional
// +patchStrategy=retainKeys
@@ -96,8 +96,6 @@ const (
CanaryRollingStyle RollingStyleType = "Canary"
// BlueGreenRollingStyle means rolling in blue-green way, and will NOT create a extra Deployment.
BlueGreenRollingStyle RollingStyleType = "BlueGreen"
// Empty means both Canary and BlueGreen are empty
EmptyRollingStyle RollingStyleType = "Empty"
)

// DeploymentExtraStatus is extra status field for Advanced Deployment
@@ -141,7 +139,7 @@ func SetDefaultDeploymentStrategy(strategy *DeploymentStrategy) {
}
}

func SetDefaultSetting(setting *OriginalSetting) {
func SetDefaultSetting(setting *OriginalDeploymentStrategy) {
if setting.ProgressDeadlineSeconds == nil {
setting.ProgressDeadlineSeconds = new(int32)
*setting.ProgressDeadlineSeconds = 600
8 changes: 0 additions & 8 deletions api/v1beta1/rollout_types.go
Original file line number Diff line number Diff line change
@@ -81,9 +81,6 @@ type RolloutStrategy struct {

// Get the rolling style based on the strategy
func (r *RolloutStrategy) GetRollingStyle() RollingStyleType {
if r.BlueGreen == nil && r.Canary == nil {
return EmptyRollingStyle
}
if r.BlueGreen != nil {
return BlueGreenRollingStyle
}
@@ -105,11 +102,6 @@ func (r *RolloutStrategy) IsCanaryStragegy() bool {
return r.GetRollingStyle() == CanaryRollingStyle || r.GetRollingStyle() == PartitionRollingStyle
}

// r.GetRollingStyle() == EmptyRollingStyle
func (r *RolloutStrategy) IsEmptyRelease() bool {
return r.GetRollingStyle() == EmptyRollingStyle
}

// Get the steps based on the rolling style
func (r *RolloutStrategy) GetSteps() []CanaryStep {
switch r.GetRollingStyle() {
8 changes: 4 additions & 4 deletions api/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 0 additions & 4 deletions pkg/controller/rollout/rollout_progressing_test.go
Original file line number Diff line number Diff line change
@@ -67,10 +67,6 @@ func TestReconcileRolloutProgressing(t *testing.T) {
s.CanaryStatus.StableRevision = "pod-template-hash-v1"
s.CanaryStatus.CanaryRevision = "6f8cc56547"
s.CanaryStatus.CurrentStepIndex = 1
// s.CanaryStatus.NextStepIndex will be initialized as 0 in ReconcileRolloutProgressing
// util.NextBatchIndex(rollout, s.CanaryStatus.CurrentStepIndex), which is 2 here.
// However the ReconcileRolloutProgressing won't update it, and thus expected value of it
// in the next cases will be 0 (zero in zero out), without update. This is as expected
s.CanaryStatus.NextStepIndex = 2
s.CanaryStatus.CurrentStepState = v1beta1.CanaryStepStateUpgrade
return s
2 changes: 1 addition & 1 deletion pkg/util/rollout_utils.go
Original file line number Diff line number Diff line change
@@ -175,7 +175,7 @@ func NextBatchIndex(rollout *rolloutv1beta1.Rollout, CurrentStepIndex int32) int
// Patching NextStepIndex to 0 is meaningless for user, if doing so, nothing happen
// and step jump won't happen
if CurrentStepIndex >= allSteps {
return 0
return -1
}
return CurrentStepIndex + 1
}
68 changes: 53 additions & 15 deletions pkg/webhook/rollout/validating/rollout_create_update_handler.go
Original file line number Diff line number Diff line change
@@ -133,12 +133,11 @@ func (h *RolloutCreateUpdateHandler) validateRolloutUpdate(oldObj, newObj *appsv
if !reflect.DeepEqual(oldObj.Spec.WorkloadRef, newObj.Spec.WorkloadRef) {
return field.ErrorList{field.Forbidden(field.NewPath("Spec.ObjectRef"), "Rollout 'ObjectRef' field is immutable")}
}
// canary strategy
if !reflect.DeepEqual(oldObj.Spec.Strategy.Canary.TrafficRoutings, newObj.Spec.Strategy.Canary.TrafficRoutings) {
return field.ErrorList{field.Forbidden(field.NewPath("Spec.Strategy.Canary.TrafficRoutings"), "Rollout 'Strategy.Canary.TrafficRoutings' field is immutable")}
if !reflect.DeepEqual(oldObj.Spec.Strategy.GetTrafficRouting(), newObj.Spec.Strategy.GetTrafficRouting()) {
return field.ErrorList{field.Forbidden(field.NewPath("Spec.Strategy.Canary|BlueGreen.TrafficRoutings"), "Rollout 'Strategy.Canary|BlueGreen.TrafficRoutings' field is immutable")}
}
if oldObj.Spec.Strategy.Canary.EnableExtraWorkloadForCanary != newObj.Spec.Strategy.Canary.EnableExtraWorkloadForCanary {
return field.ErrorList{field.Forbidden(field.NewPath("Spec.Strategy.Canary"), "Rollout enableExtraWorkloadForCanary is immutable")}
if oldObj.Spec.Strategy.GetRollingStyle() != newObj.Spec.Strategy.GetRollingStyle() {
return field.ErrorList{field.Forbidden(field.NewPath("Spec.Strategy.Canary|BlueGreen"), "Rollout style and enableExtraWorkloadForCanary are immutable")}
}
}

@@ -198,15 +197,32 @@ func validateRolloutSpecObjectRef(workloadRef *appsv1beta1.ObjectRef, fldPath *f
}

func validateRolloutSpecStrategy(strategy *appsv1beta1.RolloutStrategy, fldPath *field.Path) field.ErrorList {
if strategy.Canary == nil && strategy.BlueGreen == nil {
return field.ErrorList{field.Invalid(fldPath, nil, "Canary and BlueGreen cannot both be empty")}
}
if strategy.Canary != nil && strategy.BlueGreen != nil {
return field.ErrorList{field.Invalid(fldPath, nil, "Canary and BlueGreen cannot both be set")}
}
if strategy.BlueGreen != nil {
return validateRolloutSpecBlueGreenStrategy(strategy.BlueGreen, fldPath.Child("BlueGreen"))
}
return validateRolloutSpecCanaryStrategy(strategy.Canary, fldPath.Child("Canary"))
}

type TrafficRule string

const (
TrafficRuleCanary TrafficRule = "Canary"
TrafficRuleBlueGreen TrafficRule = "BlueGreen"
NoTraffic TrafficRule = "NoTraffic"
)

func validateRolloutSpecCanaryStrategy(canary *appsv1beta1.CanaryStrategy, fldPath *field.Path) field.ErrorList {
if canary == nil {
return field.ErrorList{field.Invalid(fldPath, nil, "Canary cannot be empty")}
trafficRule := NoTraffic
if len(canary.TrafficRoutings) > 0 {
trafficRule = TrafficRuleCanary
}

errList := validateRolloutSpecCanarySteps(canary.Steps, fldPath.Child("Steps"), len(canary.TrafficRoutings) > 0)
errList := validateRolloutSpecCanarySteps(canary.Steps, fldPath.Child("Steps"), trafficRule)
if len(canary.TrafficRoutings) > 1 {
errList = append(errList, field.Invalid(fldPath, canary.TrafficRoutings, "Rollout currently only support single TrafficRouting."))
}
@@ -216,6 +232,21 @@ func validateRolloutSpecCanaryStrategy(canary *appsv1beta1.CanaryStrategy, fldPa
return errList
}

func validateRolloutSpecBlueGreenStrategy(blueGreen *appsv1beta1.BlueGreenStrategy, fldPath *field.Path) field.ErrorList {
trafficRule := NoTraffic
if len(blueGreen.TrafficRoutings) > 0 {
trafficRule = TrafficRuleBlueGreen
}
errList := validateRolloutSpecCanarySteps(blueGreen.Steps, fldPath.Child("Steps"), trafficRule)
if len(blueGreen.TrafficRoutings) > 1 {
errList = append(errList, field.Invalid(fldPath, blueGreen.TrafficRoutings, "Rollout currently only support single TrafficRouting."))
}
for _, traffic := range blueGreen.TrafficRoutings {
errList = append(errList, validateRolloutSpecCanaryTraffic(traffic, fldPath.Child("TrafficRouting"))...)
}
return errList
}

func validateRolloutSpecCanaryTraffic(traffic appsv1beta1.TrafficRoutingRef, fldPath *field.Path) field.ErrorList {
errList := field.ErrorList{}
if len(traffic.Service) == 0 {
@@ -240,7 +271,7 @@ func validateRolloutSpecCanaryTraffic(traffic appsv1beta1.TrafficRoutingRef, fld
return errList
}

func validateRolloutSpecCanarySteps(steps []appsv1beta1.CanaryStep, fldPath *field.Path, isTraffic bool) field.ErrorList {
func validateRolloutSpecCanarySteps(steps []appsv1beta1.CanaryStep, fldPath *field.Path, trafficRule TrafficRule) field.ErrorList {
stepCount := len(steps)
if stepCount == 0 {
return field.ErrorList{field.Invalid(fldPath, steps, "The number of Canary.Steps cannot be empty")}
@@ -258,14 +289,21 @@ func validateRolloutSpecCanarySteps(steps []appsv1beta1.CanaryStep, fldPath *fie
return field.ErrorList{field.Invalid(fldPath.Index(i).Child("Replicas"),
s.Replicas, `replicas must be positive number, or a percentage with "0%" < canaryReplicas <= "100%"`)}
}
if !isTraffic {
if trafficRule == NoTraffic || s.Traffic == nil {
continue
}
if s.Traffic != nil {
is := intstr.FromString(*s.Traffic)
weight, err := intstr.GetScaledValueFromIntOrPercent(&is, 100, true)
is := intstr.FromString(*s.Traffic)
weight, err := intstr.GetScaledValueFromIntOrPercent(&is, 100, true)
switch trafficRule {
case TrafficRuleBlueGreen:
// traffic "0%" is allowed in blueGreen strategy
if err != nil || weight < 0 || weight > 100 {
return field.ErrorList{field.Invalid(fldPath.Index(i).Child("steps"), steps, `traffic must be percentage with "0%" <= traffic <= "100%" in blueGreen strategy`)}
}
default:
// traffic "0%" is not allowed in canary strategy
if err != nil || weight <= 0 || weight > 100 {
return field.ErrorList{field.Invalid(fldPath.Index(i).Child("steps"), steps, `traffic must be percentage with "0%" < traffic <= "100%"`)}
return field.ErrorList{field.Invalid(fldPath.Index(i).Child("steps"), steps, `traffic must be percentage with "0%" < traffic <= "100%" in canary strategy`)}
}
}
}
21 changes: 21 additions & 0 deletions test/e2e/deployment_test.go
Original file line number Diff line number Diff line change
@@ -26,6 +26,16 @@ import (

var _ = SIGDescribe("Advanced Deployment", func() {
var namespace string

DumpAllResources := func() {
deploy := &apps.DeploymentList{}
k8sClient.List(context.TODO(), deploy, client.InNamespace(namespace))
fmt.Println(util.DumpJSON(deploy))
rs := &apps.ReplicaSetList{}
k8sClient.List(context.TODO(), rs, client.InNamespace(namespace))
fmt.Println(util.DumpJSON(rs))
}

defaultRetry := wait.Backoff{
Steps: 10,
Duration: 10 * time.Millisecond,
@@ -132,7 +142,12 @@ var _ = SIGDescribe("Advanced Deployment", func() {

CheckReplicas := func(deployment *apps.Deployment, replicas, available, updated int32) {
var clone *apps.Deployment
start := time.Now()
Eventually(func() bool {
if start.Add(time.Minute * 2).Before(time.Now()) {
DumpAllResources()
Expect(true).Should(BeFalse())
}
clone = &apps.Deployment{}
err := GetObject(deployment.Namespace, deployment.Name, clone)
Expect(err).NotTo(HaveOccurred())
@@ -239,6 +254,7 @@ var _ = SIGDescribe("Advanced Deployment", func() {
deployment.Namespace = namespace
Expect(ReadYamlToObject("./test_data/deployment/deployment.yaml", deployment)).ToNot(HaveOccurred())
CreateObject(deployment)
CheckReplicas(deployment, 5, 5, 5)
UpdateDeployment(deployment, "version2")
UpdatePartitionWithCheck(deployment, intstr.FromInt(0))
UpdatePartitionWithCheck(deployment, intstr.FromInt(1))
@@ -255,6 +271,7 @@ var _ = SIGDescribe("Advanced Deployment", func() {
Expect(ReadYamlToObject("./test_data/deployment/deployment.yaml", deployment)).ToNot(HaveOccurred())
deployment.Spec.Replicas = pointer.Int32(10)
CreateObject(deployment)
CheckReplicas(deployment, 10, 10, 10)
UpdateDeployment(deployment, "version2")
UpdatePartitionWithCheck(deployment, intstr.FromString("0%"))
UpdatePartitionWithCheck(deployment, intstr.FromString("40%"))
@@ -287,6 +304,7 @@ var _ = SIGDescribe("Advanced Deployment", func() {
`{"rollingStyle":"Partition","rollingUpdate":{"maxUnavailable":1,"maxSurge":0}}`
deployment.Spec.MinReadySeconds = 10
CreateObject(deployment)
CheckReplicas(deployment, 5, 5, 5)
UpdateDeployment(deployment, "version2")
UpdatePartitionWithCheck(deployment, intstr.FromInt(0))
UpdatePartitionWithoutCheck(deployment, intstr.FromInt(3))
@@ -303,6 +321,7 @@ var _ = SIGDescribe("Advanced Deployment", func() {
deployment.Namespace = namespace
Expect(ReadYamlToObject("./test_data/deployment/deployment.yaml", deployment)).ToNot(HaveOccurred())
CreateObject(deployment)
CheckReplicas(deployment, 5, 5, 5)
UpdateDeployment(deployment, "version2")
UpdatePartitionWithCheck(deployment, intstr.FromInt(0))
UpdatePartitionWithCheck(deployment, intstr.FromInt(2))
@@ -317,6 +336,7 @@ var _ = SIGDescribe("Advanced Deployment", func() {
deployment.Namespace = namespace
Expect(ReadYamlToObject("./test_data/deployment/deployment.yaml", deployment)).ToNot(HaveOccurred())
CreateObject(deployment)
CheckReplicas(deployment, 5, 5, 5)
UpdateDeployment(deployment, "version2")
UpdatePartitionWithCheck(deployment, intstr.FromInt(0))
UpdatePartitionWithCheck(deployment, intstr.FromInt(2))
@@ -335,6 +355,7 @@ var _ = SIGDescribe("Advanced Deployment", func() {
Expect(ReadYamlToObject("./test_data/deployment/deployment.yaml", deployment)).ToNot(HaveOccurred())
deployment.Annotations["rollouts.kruise.io/deployment-strategy"] = `{"rollingUpdate":{"maxUnavailable":0,"maxSurge":1}}`
CreateObject(deployment)
CheckReplicas(deployment, 5, 5, 5)
UpdateDeployment(deployment, "version2", "busybox:not-exists")
UpdatePartitionWithoutCheck(deployment, intstr.FromInt(1))
CheckReplicas(deployment, 6, 5, 1)
19 changes: 13 additions & 6 deletions test/e2e/rollout_test.go
Original file line number Diff line number Diff line change
@@ -204,18 +204,25 @@ var _ = SIGDescribe("Rollout", func() {
}

ResumeRolloutCanary := func(name string) {
clone := &v1alpha1.Rollout{}
Expect(GetObject(name, clone)).NotTo(HaveOccurred())
currentIndex := clone.Status.CanaryStatus.CurrentStepIndex
Eventually(func() bool {
clone := &v1alpha1.Rollout{}
Expect(GetObject(name, clone)).NotTo(HaveOccurred())
if clone.Status.CanaryStatus.CurrentStepState != v1alpha1.CanaryStepStatePaused {
if clone.Status.CanaryStatus.CurrentStepIndex == currentIndex && clone.Status.CanaryStatus.CurrentStepState == v1alpha1.CanaryStepStatePaused {
klog.Info("patch to stepReady")
body := fmt.Sprintf(`{"status":{"canaryStatus":{"currentStepState":"%s"}}}`, v1alpha1.CanaryStepStateReady)
Expect(k8sClient.Status().Patch(context.TODO(), clone, client.RawPatch(types.MergePatchType, []byte(body)))).NotTo(HaveOccurred())
return false
} else {
fmt.Println("resume rollout success, and CurrentStepState", util.DumpJSON(clone.Status))
return true
}

body := fmt.Sprintf(`{"status":{"canaryStatus":{"currentStepState":"%s"}}}`, v1alpha1.CanaryStepStateReady)
Expect(k8sClient.Status().Patch(context.TODO(), clone, client.RawPatch(types.MergePatchType, []byte(body)))).NotTo(HaveOccurred())
return false
}, 10*time.Second, time.Second).Should(BeTrue())
// interval was critical before:
// too small: StepReady could be overidden by StepPaused
// too big: StepReady could progress to StepPaused of next Step
}, 10*time.Second, 2*time.Second).Should(BeTrue())
}

WaitDeploymentAllPodsReady := func(deployment *apps.Deployment) {