diff --git a/pkg/controller/release/release_controller.go b/pkg/controller/release/release_controller.go index 653f83d07..8928ad602 100644 --- a/pkg/controller/release/release_controller.go +++ b/pkg/controller/release/release_controller.go @@ -273,15 +273,13 @@ func (c *Controller) syncOneReleaseHandler(key string) error { } var condition *shipper.ReleaseCondition - var strategyPatches []StrategyPatch - var trans []ReleaseStrategyStateTransition var relinfo *releaseInfo - var stepAchieved bool + var patches []StrategyPatch + var execRel *shipper.Release // we keep baseRel as a comparison baseline in order to figure out if // we even have to send an update baseRel := rel.DeepCopy() - patches := make([]StrategyPatch, 0) diff := diffutil.NewMultiDiff() defer func() { @@ -357,7 +355,7 @@ func (c *Controller) syncOneReleaseHandler(key string) error { ) diff.Append(releaseutil.SetReleaseCondition(&rel.Status, *condition)) - stepAchieved, strategyPatches, trans, err = c.executeReleaseStrategy(relinfo) + execRel, patches, err = c.executeReleaseStrategy(relinfo, diff) if err != nil { releaseStrategyExecutedCond := releaseutil.NewReleaseCondition( shipper.ReleaseConditionTypeStrategyExecuted, @@ -369,59 +367,7 @@ func (c *Controller) syncOneReleaseHandler(key string) error { goto ApplyChanges } - patches = append(patches, strategyPatches...) - - condition = releaseutil.NewReleaseCondition( - shipper.ReleaseConditionTypeStrategyExecuted, - corev1.ConditionTrue, - "", - "", - ) - diff.Append(releaseutil.SetReleaseCondition(&rel.Status, *condition)) - - if stepAchieved { - strategy := rel.Spec.Environment.Strategy - targetStep := rel.Spec.TargetStep - isLastStep := int(targetStep) == len(strategy.Steps)-1 - previouslyAchievedStep := rel.Status.AchievedStep - - if previouslyAchievedStep == nil || targetStep != previouslyAchievedStep.Step { - targetStepName := strategy.Steps[targetStep].Name - rel.Status.AchievedStep = &shipper.AchievedStep{ - Step: targetStep, - Name: targetStepName, - } - c.recorder.Eventf( - rel, - corev1.EventTypeNormal, - "StrategyApplied", - "step [%d] finished", - targetStep, - ) - } - - if isLastStep { - condition := releaseutil.NewReleaseCondition( - shipper.ReleaseConditionTypeComplete, - corev1.ConditionTrue, - "", - "", - ) - diff.Append(releaseutil.SetReleaseCondition(&rel.Status, *condition)) - } - } - - for _, t := range trans { - c.recorder.Eventf( - rel, - corev1.EventTypeNormal, - "ReleaseStateTransitioned", - "Release %q had its state %q transitioned to %q", - shippercontroller.MetaKey(rel), - t.State, - t.New, - ) - } + rel = execRel ApplyChanges: @@ -454,41 +400,122 @@ func (c *Controller) applicationReleases(rel *shipper.Release) ([]*shipper.Relea return releases, nil } -func (c *Controller) executeReleaseStrategy(relinfo *releaseInfo) (bool, []StrategyPatch, []ReleaseStrategyStateTransition, error) { - releases, err := c.applicationReleases(relinfo.release) +func (c *Controller) executeReleaseStrategy(relinfo *releaseInfo, diff *diffutil.MultiDiff) (*shipper.Release, []StrategyPatch, error) { + rel := relinfo.release.DeepCopy() + + releases, err := c.applicationReleases(rel) if err != nil { - return false, nil, nil, err + return nil, nil, err } - prev, succ, err := releaseutil.GetSiblingReleases(relinfo.release, releases) + prev, succ, err := releaseutil.GetSiblingReleases(rel, releases) if err != nil { - return false, nil, nil, err + return nil, nil, err } var relinfoPrev, relinfoSucc *releaseInfo if prev != nil { relinfoPrev, err = c.buildReleaseInfo(prev) if err != nil { - return false, nil, nil, err + return nil, nil, err } } if succ != nil { relinfoSucc, err = c.buildReleaseInfo(succ) if err != nil { - return false, nil, nil, err + return nil, nil, err } } - executor := NewStrategyExecutor(relinfoPrev, relinfo, relinfoSucc) + isHead := succ == nil + var strategy *shipper.RolloutStrategy + var targetStep int32 + // A head release uses it's local spec-defined strategy, any other release + // follows it's successor state, therefore looking into the forecoming spec. + if isHead { + strategy = rel.Spec.Environment.Strategy + targetStep = rel.Spec.TargetStep + } else { + strategy = succ.Spec.Environment.Strategy + targetStep = succ.Spec.TargetStep + } + + // Looks like a malformed input. Informing about a problem and bailing out. + if targetStep >= int32(len(strategy.Steps)) { + err := fmt.Errorf("no step %d in strategy for Release %q", + targetStep, controller.MetaKey(rel)) + return nil, nil, shippererrors.NewUnrecoverableError(err) + } + + executor := NewStrategyExecutor(strategy, targetStep) - complete, patches, trans, err := executor.Execute() + complete, patches, trans := executor.Execute(relinfoPrev, relinfo, relinfoSucc) if len(patches) == 0 { - klog.V(4).Infof("Strategy verified for release %q, nothing to patch", controller.MetaKey(relinfo.release)) + klog.V(4).Infof("Strategy verified for release %q, nothing to patch", controller.MetaKey(rel)) } else { - klog.V(4).Infof("Strategy has been executed for release %q, applying patches", controller.MetaKey(relinfo.release)) + klog.V(4).Infof("Strategy has been executed for release %q, applying patches", controller.MetaKey(rel)) + } + + condition := releaseutil.NewReleaseCondition( + shipper.ReleaseConditionTypeStrategyExecuted, + corev1.ConditionTrue, + "", + "", + ) + diff.Append(releaseutil.SetReleaseCondition(&rel.Status, *condition)) + + isLastStep := int(targetStep) == len(strategy.Steps)-1 + prevStep := rel.Status.AchievedStep + + if complete { + var achievedStep int32 + var achievedStepName string + if isHead { + achievedStep = targetStep + achievedStepName = strategy.Steps[achievedStep].Name + } else { + achievedStep = int32(len(rel.Spec.Environment.Strategy.Steps)) - 1 + achievedStepName = rel.Spec.Environment.Strategy.Steps[achievedStep].Name + + } + if prevStep == nil || achievedStep != prevStep.Step { + rel.Status.AchievedStep = &shipper.AchievedStep{ + Step: achievedStep, + Name: achievedStepName, + } + c.recorder.Eventf( + rel, + corev1.EventTypeNormal, + "StrategyApplied", + "step [%d] finished", + achievedStep, + ) + } + + if isLastStep { + condition := releaseutil.NewReleaseCondition( + shipper.ReleaseConditionTypeComplete, + corev1.ConditionTrue, + "", + "", + ) + diff.Append(releaseutil.SetReleaseCondition(&rel.Status, *condition)) + } + } + + for _, t := range trans { + c.recorder.Eventf( + rel, + corev1.EventTypeNormal, + "ReleaseStateTransitioned", + "Release %q had its state %q transitioned to %q", + shippercontroller.MetaKey(rel), + t.State, + t.New, + ) } - return complete, patches, trans, err + return rel, patches, nil } func (c *Controller) applyPatch(namespace string, patch StrategyPatch) error { diff --git a/pkg/controller/release/release_controller_test.go b/pkg/controller/release/release_controller_test.go index c7554582a..91a2ee957 100644 --- a/pkg/controller/release/release_controller_test.go +++ b/pkg/controller/release/release_controller_test.go @@ -54,6 +54,16 @@ var vanguard = shipper.RolloutStrategy{ }, } +var fullon = shipper.RolloutStrategy{ + Steps: []shipper.RolloutStrategyStep{ + { + Name: "full on", + Capacity: shipper.RolloutStrategyStepValue{Incumbent: 0, Contender: 100}, + Traffic: shipper.RolloutStrategyStepValue{Incumbent: 0, Contender: 100}, + }, + }, +} + type role int const ( @@ -726,7 +736,7 @@ func (f *fixture) expectReleaseWaitingForCommand(rel *shipper.Release, step int3 fmt.Sprintf(`Normal ReleaseStateTransitioned Release "%s" had its state "WaitingForCommand" transitioned to "True"`, relKey), fmt.Sprintf(`Normal ReleaseStateTransitioned Release "%s" had its state "WaitingForInstallation" transitioned to "False"`, relKey), fmt.Sprintf(`Normal ReleaseStateTransitioned Release "%s" had its state "WaitingForTraffic" transitioned to "False"`, relKey), - fmt.Sprintf(`Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted True]`), + `Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted True]`, } } @@ -2338,8 +2348,6 @@ func TestApplicationExposesStrategyFailureSuccessorIndexOutOfBounds(t *testing.T contender := f.buildContender(namespace, contenderName, totalReplicaCount) incumbent := f.buildIncumbent(namespace, incumbentName, totalReplicaCount) - missingStepMsg := fmt.Sprintf("failed to execute strategy: \"no step 2 in strategy for Release \\\"%s/%s\\\"\"", namespace, contenderName) - // We define 2 steps and will intentionally set target step index out of this bound strategyStaging := shipper.RolloutStrategy{ Steps: []shipper.RolloutStrategyStep{ @@ -2379,7 +2387,9 @@ func TestApplicationExposesStrategyFailureSuccessorIndexOutOfBounds(t *testing.T }, { Type: shipper.ReleaseConditionTypeStrategyExecuted, - Status: corev1.ConditionTrue, + Status: corev1.ConditionFalse, + Reason: conditions.StrategyExecutionFailed, + Message: fmt.Sprintf(`failed to execute strategy: "no step 2 in strategy for Release \"%s/%s\""`, namespace, incumbentName), }, } @@ -2397,7 +2407,7 @@ func TestApplicationExposesStrategyFailureSuccessorIndexOutOfBounds(t *testing.T Type: shipper.ReleaseConditionTypeStrategyExecuted, Status: corev1.ConditionFalse, Reason: conditions.StrategyExecutionFailed, - Message: missingStepMsg, + Message: fmt.Sprintf(`failed to execute strategy: "no step 2 in strategy for Release \"%s/%s\""`, namespace, contenderName), }, } @@ -2434,8 +2444,11 @@ func TestApplicationExposesStrategyFailureSuccessorIndexOutOfBounds(t *testing.T []string{"releases"}, }) f.expectedEvents = append(f.expectedEvents, - `Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted True]`, - fmt.Sprintf(`Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted False StrategyExecutionFailed %s]`, missingStepMsg)) + fmt.Sprintf(`Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted False StrategyExecutionFailed %s]`, + fmt.Sprintf(`failed to execute strategy: "no step 2 in strategy for Release \"%s/%s\""`, namespace, incumbentName)), + fmt.Sprintf(`Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted False StrategyExecutionFailed %s]`, + fmt.Sprintf(`failed to execute strategy: "no step 2 in strategy for Release \"%s/%s\""`, namespace, contenderName)), + ) f.run() } @@ -2770,3 +2783,49 @@ func TestControllerChooseClustersSkipsUnschedulable(t *testing.T) { f.run() } + +func TestIncumbentOutOfRangeTargetStep(t *testing.T) { + namespace := "test-namespace" + incumbentName, contenderName := "test-incumbent", "test-contender" + app := buildApplication(namespace, "test-app") + cluster := buildCluster("minikube") + + f := newFixture(t, app.DeepCopy(), cluster.DeepCopy()) + f.cycles = 2 + + totalReplicaCount := int32(10) + contender := f.buildContender(namespace, contenderName, totalReplicaCount) + incumbent := f.buildIncumbent(namespace, incumbentName, totalReplicaCount) + + // Incumbent spec contains only 1 strategy step but we intentionally + // specify an out-of-range index in order to test if it carefully + // handles indices. + incumbent.release.Spec.TargetStep = 2 + incumbent.release.Spec.Environment.Strategy = &fullon + incumbent.release.Status.AchievedStep.Step = 0 + incumbent.trafficTarget.Spec.Clusters[0].Weight = 50 + incumbent.capacityTarget.Spec.Clusters[0].Percent = 50 + incumbent.capacityTarget.Spec.Clusters[0].TotalReplicaCount = totalReplicaCount + + step := int32(1) + contender.release.Spec.TargetStep = step + contender.capacityTarget.Spec.Clusters[0].Percent = 50 + contender.capacityTarget.Spec.Clusters[0].TotalReplicaCount = totalReplicaCount + contender.trafficTarget.Spec.Clusters[0].Weight = 50 + + f.addObjects( + contender.release.DeepCopy(), + contender.installationTarget.DeepCopy(), + contender.capacityTarget.DeepCopy(), + contender.trafficTarget.DeepCopy(), + + incumbent.release.DeepCopy(), + incumbent.installationTarget.DeepCopy(), + incumbent.capacityTarget.DeepCopy(), + incumbent.trafficTarget.DeepCopy(), + ) + + f.expectReleaseWaitingForCommand(contender.release, step) + + f.run() +} diff --git a/pkg/controller/release/strategy_executor.go b/pkg/controller/release/strategy_executor.go index d4a315650..848232bb8 100644 --- a/pkg/controller/release/strategy_executor.go +++ b/pkg/controller/release/strategy_executor.go @@ -8,23 +8,10 @@ import ( shipper "github.com/bookingcom/shipper/pkg/apis/shipper/v1alpha1" "github.com/bookingcom/shipper/pkg/controller" - shippererrors "github.com/bookingcom/shipper/pkg/errors" "github.com/bookingcom/shipper/pkg/util/conditions" releaseutil "github.com/bookingcom/shipper/pkg/util/release" ) -type StrategyExecutor struct { - curr, prev, succ *releaseInfo -} - -func NewStrategyExecutor(prev, curr, succ *releaseInfo) *StrategyExecutor { - return &StrategyExecutor{ - prev: prev, - curr: curr, - succ: succ, - } -} - type PipelineContinuation bool const ( @@ -49,19 +36,106 @@ type Extra struct { HasIncumbent bool } -func (p *Pipeline) Process(strategy *shipper.RolloutStrategy, targetStep int32, extra Extra, cond conditions.StrategyConditionsMap) (bool, []StrategyPatch, []ReleaseStrategyStateTransition) { - var res []StrategyPatch +func (p *Pipeline) Process(strategy *shipper.RolloutStrategy, step int32, extra Extra, cond conditions.StrategyConditionsMap) (bool, []StrategyPatch, []ReleaseStrategyStateTransition) { + var patches []StrategyPatch var trans []ReleaseStrategyStateTransition - for _, step := range *p { - cont, stepres, steptrans := step(strategy, targetStep, extra, cond) - res = append(res, stepres...) + for _, stage := range *p { + cont, steppatches, steptrans := stage(strategy, step, extra, cond) + patches = append(patches, steppatches...) trans = append(trans, steptrans...) if cont == PipelineBreak { - return false, res, trans + return false, patches, trans } } - return true, res, trans + return true, patches, trans +} + +type StrategyExecutor struct { + strategy *shipper.RolloutStrategy + step int32 +} + +func NewStrategyExecutor(strategy *shipper.RolloutStrategy, step int32) *StrategyExecutor { + return &StrategyExecutor{ + strategy: strategy, + step: step, + } +} + +/* + For each release object: + 0. Ensure release scheduled. + 0.1. Choose clusters. + 0.2. Ensure target objects exist. + 0.2.1. Compare chosen clusters and if different, update the spec. + 1. Find it's ancestor. + 2. For the head release, ensure installation. + 2.1. Simply check installation targets. + 3. For the head release, ensure capacity. + 3.1. Ensure the capacity corresponds to the strategy contender. + 4. For the head release, ensure traffic. + 4.1. Ensure the traffic corresponds to the strategy contender. + 5. For a tail release, ensure traffic. + 5.1. Look at the leader and check it's target traffic. + 5.2. Look at the strategy and figure out the target traffic. + 6. For a tail release, ensure capacity. + 6.1. Look at the leader and check it's target capacity. + 6.2 Look at the strategy and figure out the target capacity. + 7. Make necessary adjustments to the release object. +*/ + +func (e *StrategyExecutor) Execute(prev, curr, succ *releaseInfo) (bool, []StrategyPatch, []ReleaseStrategyStateTransition) { + isHead, hasTail := succ == nil, prev != nil + hasIncumbent := prev != nil || succ != nil + + // There is no really a point in making any changes until the successor + // has completed it's transition, therefore we're hoilding off and aborting + // the pipeline execution. An alternative to this approach could be to make + // an autonomous move purely based on the picture of the world. But due to + // the limited visilibility of what's happening to the successor (as it + // might be following it's successor) it could be that a preliminary action + // would create more noise than help really. + if !isHead { + if !releaseutil.ReleaseAchievedTargetStep(succ.release) { + return false, nil, nil + } + } + + var releaseStrategyConditions []shipper.ReleaseStrategyCondition + if curr.release.Status.Strategy != nil { + releaseStrategyConditions = curr.release.Status.Strategy.Conditions + } + cond := conditions.NewStrategyConditions(releaseStrategyConditions...) + + // the last step is slightly special from others: at this moment shipper + // no longer waits for a command but marks a release as complete. + isLastStep := int(e.step) == len(e.strategy.Steps)-1 + // The reason because isHead is not included in the extra set is mainly + // because the pipeline is picking up 2 distinct tuples of releases + // (curr+succ) and (prev+curr), therefore isHead is supposed to be + // calculated by enforcers. + extra := Extra{ + HasIncumbent: hasIncumbent, + IsLastStep: isLastStep, + } + + pipeline := NewPipeline() + if isHead { + pipeline.Enqueue(genInstallationEnforcer(curr, nil)) + } + pipeline.Enqueue(genCapacityEnforcer(curr, succ)) + pipeline.Enqueue(genTrafficEnforcer(curr, succ)) + + if isHead { + if hasTail { + pipeline.Enqueue(genTrafficEnforcer(prev, curr)) + pipeline.Enqueue(genCapacityEnforcer(prev, curr)) + } + pipeline.Enqueue(genReleaseStrategyStateEnforcer(curr, nil)) + } + + return pipeline.Process(e.strategy, e.step, extra, cond) } func genInstallationEnforcer(curr, succ *releaseInfo) PipelineStep { @@ -274,104 +348,6 @@ func genReleaseStrategyStateEnforcer(curr, succ *releaseInfo) PipelineStep { } } -/* - For each release object: - 0. Ensure release scheduled. - 0.1. Choose clusters. - 0.2. Ensure target objects exist. - 0.2.1. Compare chosen clusters and if different, update the spec. - 1. Find it's ancestor. - 2. For the head release, ensure installation. - 2.1. Simply check installation targets. - 3. For the head release, ensure capacity. - 3.1. Ensure the capacity corresponds to the strategy contender. - 4. For the head release, ensure traffic. - 4.1. Ensure the traffic corresponds to the strategy contender. - 5. For a tail release, ensure traffic. - 5.1. Look at the leader and check it's target traffic. - 5.2. Look at the strategy and figure out the target traffic. - 6. For a tail release, ensure capacity. - 6.1. Look at the leader and check it's target capacity. - 6.2 Look at the strategy and figure out the target capacity. - 7. Make necessary adjustments to the release object. -*/ - -func (e *StrategyExecutor) Execute() (bool, []StrategyPatch, []ReleaseStrategyStateTransition, error) { - isHead, hasTail := e.succ == nil, e.prev != nil - hasIncumbent := e.prev != nil || e.succ != nil - - // There is no really a point in making any changes until the successor - // has completed it's transition, therefore we're hoilding off and aborting - // the pipeline execution. An alternative to this approach could be to make - // an autonomous move purely based on the picture of the world. But due to - // the limited visilibility of what's happening to the successor (as it - // might be following it's successor) it could be that a preliminary action - // would create more noise than help really. - if !isHead { - if !releaseutil.ReleaseAchievedTargetStep(e.succ.release) { - return false, nil, nil, nil - //shippererrors.NewContenderStrategyIncompleteError( - // controller.MetaKey(e.succ.release)) - } - } - - var releaseStrategyConditions []shipper.ReleaseStrategyCondition - if e.curr.release.Status.Strategy != nil { - releaseStrategyConditions = e.curr.release.Status.Strategy.Conditions - } - cond := conditions.NewStrategyConditions(releaseStrategyConditions...) - - var strategy *shipper.RolloutStrategy - var targetStep int32 - - // A head release uses it's local spec-defined strategy, any other release - // follows it's successor state, therefore looking into the forecoming spec. - if isHead { - strategy = e.curr.release.Spec.Environment.Strategy - targetStep = e.curr.release.Spec.TargetStep - } else { - strategy = e.succ.release.Spec.Environment.Strategy - targetStep = e.succ.release.Spec.TargetStep - } - - // Looks like a malformed input. Informing about a problem and bailing out. - if targetStep >= int32(len(strategy.Steps)) { - err := fmt.Errorf("no step %d in strategy for Release %q", - targetStep, controller.MetaKey(e.curr.release)) - return false, nil, nil, shippererrors.NewUnrecoverableError(err) - } - - // the last step is slightly special from others: at this moment shipper - // no longer waits for a command but marks a release as complete. - isLastStep := int(targetStep) == len(strategy.Steps)-1 - // The reason because isHead is not included in the extra set is mainly - // because the is picking up 2 distinct tuples of releases (curr+succ) and - // (prev+curr), therefore isHead is supposed to be calculated by enforcers. - extra := Extra{ - HasIncumbent: hasIncumbent, - IsLastStep: isLastStep, - } - - pipeline := NewPipeline() - if isHead { - pipeline.Enqueue(genInstallationEnforcer(e.curr, nil)) - } - pipeline.Enqueue(genCapacityEnforcer(e.curr, e.succ)) - pipeline.Enqueue(genTrafficEnforcer(e.curr, e.succ)) - - if isHead { - if hasTail { - pipeline.Enqueue(genTrafficEnforcer(e.prev, e.curr)) - pipeline.Enqueue(genCapacityEnforcer(e.prev, e.curr)) - } - pipeline.Enqueue(genReleaseStrategyStateEnforcer(e.curr, nil)) - } - - complete, patches, trans := pipeline.Process(strategy, targetStep, extra, cond) - - return complete, patches, trans, nil -} - func buildContenderStrategyConditionsPatch( name string, cond conditions.StrategyConditionsMap,