From b564e0ef263c1b3c8b46dfb7022ec055bf22ac75 Mon Sep 17 00:00:00 2001 From: Ryan Zhang Date: Fri, 5 Feb 2021 12:08:19 -0800 Subject: [PATCH] add more code to complete the rollout plan (#1024) --- .../v1alpha1/rollout_plan_types.go | 13 +- .../v1alpha1/rollout_state.go | 64 ++++++++-- .../core.oam.dev_applicationdeployments.yaml | 6 +- .../crds/standard.oam.dev_rollouttraits.yaml | 6 +- .../core.oam.dev_applicationdeployments.yaml | 6 +- .../crds/standard.oam.dev_rollouttraits.yaml | 6 +- .../common/rollout/rollout_plan_controller.go | 79 ++++++++---- .../rollout/workloads/cloneset_controller.go | 116 +++++++++++++----- .../common/rollout/workloads/common.go | 39 ++++++ .../common/rollout/workloads/controller.go | 4 + pkg/webhook/common/rollout/rollout_plan.go | 72 +++++++++-- .../applicationdeployment/validation.go | 5 +- 12 files changed, 322 insertions(+), 94 deletions(-) create mode 100644 pkg/controller/common/rollout/workloads/common.go diff --git a/apis/standard.oam.dev/v1alpha1/rollout_plan_types.go b/apis/standard.oam.dev/v1alpha1/rollout_plan_types.go index bd212c52d..d30afd82f 100644 --- a/apis/standard.oam.dev/v1alpha1/rollout_plan_types.go +++ b/apis/standard.oam.dev/v1alpha1/rollout_plan_types.go @@ -60,14 +60,13 @@ const ( // BatchInRollingState still rolling the batch, the batch rolling is not completed yet BatchInRollingState BatchRollingState = "batchInRolling" // BatchVerifyingState verifying if the application is ready to roll. - // This happens when it's either manual or automatic with analysis BatchVerifyingState BatchRollingState = "batchVerifying" // BatchRolloutFailedState indicates that the batch didn't get the manual or automatic approval BatchRolloutFailedState BatchRollingState = "batchVerifyFailed" + // BatchFinalizingState indicates that all the pods in the are available, we can move on to the next batch + BatchFinalizingState BatchRollingState = "batchFinalizing" // BatchReadyState indicates that all the pods in the are upgraded and its state is ready BatchReadyState BatchRollingState = "batchReady" - // BatchFinalizeState indicates that all the pods in the are available, we can move on to the next batch - BatchFinalizeState BatchRollingState = "batchFinalize" ) // RolloutPlan fines the details of the rollout plan @@ -88,7 +87,10 @@ type RolloutPlan struct { NumBatches *int32 `json:"numBatches,omitempty"` // The exact distribution among batches. - // mutually exclusive to NumBatches + // mutually exclusive to NumBatches. + // The total number cannot exceed the targetSize or the size of the source resource + // We will IGNORE the last batch's replica field if it's a percentage since round errors can lead to inaccurate sum + // We highly recommend to leave the last batch's replica field empty // +optional RolloutBatches []RolloutBatch `json:"rolloutBatches,omitempty"` @@ -103,7 +105,7 @@ type RolloutPlan struct { // +optional Paused bool `json:"paused,omitempty"` - // RolloutWebhooks provides a way for the rollout to interact with an external process + // RolloutWebhooks provide a way for the rollout to interact with an external process // +optional RolloutWebhooks []RolloutWebhook `json:"rolloutWebhooks,omitempty"` @@ -117,6 +119,7 @@ type RolloutPlan struct { type RolloutBatch struct { // Replicas is the number of pods to upgrade in this batch // it can be an absolute number (ex: 5) or a percentage of total pods + // we will ignore the percentage of the last batch to just fill the gap // +optional // it is mutually exclusive with the PodList field Replicas intstr.IntOrString `json:"replicas,omitempty"` diff --git a/apis/standard.oam.dev/v1alpha1/rollout_state.go b/apis/standard.oam.dev/v1alpha1/rollout_state.go index 197916e59..9f8f5ad87 100644 --- a/apis/standard.oam.dev/v1alpha1/rollout_state.go +++ b/apis/standard.oam.dev/v1alpha1/rollout_state.go @@ -49,7 +49,7 @@ const ( // this events comes after we have examine the pod readiness check and traffic shifting if needed OneBatchAvailableEvent RolloutEvent = "OneBatchAvailable" - // BatchRolloutApprovedEvent indicates that we are waiting for the approval of the + // BatchRolloutApprovedEvent indicates that we got the approval manually BatchRolloutApprovedEvent RolloutEvent = "BatchRolloutApprovedEvent" // BatchRolloutFailedEvent indicates that we are waiting for the approval of the @@ -59,7 +59,7 @@ const ( WorkloadModifiedEvent RolloutEvent = "WorkloadModifiedEvent" ) -// These are valid conditions of pod. +// These are valid conditions of the rollout. const ( // RolloutSpecVerified indicates that the rollout spec matches the resource we have in the cluster RolloutSpecVerified runtimev1alpha1.ConditionType = "RolloutSpecVerified" @@ -69,6 +69,18 @@ const ( RolloutInProgress runtimev1alpha1.ConditionType = "Ready" // RolloutSucceed means that the rollout is done. RolloutSucceed runtimev1alpha1.ConditionType = "Succeed" + // BatchInitialized + BatchInitialized runtimev1alpha1.ConditionType = "BatchInitialized" + // BatchInRolled + BatchInRolled runtimev1alpha1.ConditionType = "BatchInRolled" + // BatchVerified + BatchVerified runtimev1alpha1.ConditionType = "BatchVerified" + // BatchRolloutFailed + BatchRolloutFailed runtimev1alpha1.ConditionType = "BatchRolloutFailed" + // BatchFinalized + BatchFinalized runtimev1alpha1.ConditionType = "BatchFinalized" + // BatchReady + BatchReady runtimev1alpha1.ConditionType = "BatchReady" ) // NewPositiveCondition creates a positive condition type @@ -104,7 +116,22 @@ func (r *RolloutStatus) getRolloutConditionType() runtimev1alpha1.ConditionType return RolloutInitialized case RollingInBatchesState: - return RolloutInProgress + switch r.BatchRollingState { + case BatchInitializingState: + return BatchInitialized + + case BatchVerifyingState: + return BatchVerified + + case BatchFinalizingState: + return BatchFinalized + + case BatchReadyState: + return BatchReady + + default: + return RolloutInProgress + } case FinalisingState: return RolloutSucceed @@ -148,6 +175,7 @@ func (r *RolloutStatus) StateTransition(event RolloutEvent) { case VerifyingState: if event == RollingSpecVerifiedEvent { r.RollingState = InitializingState + r.SetConditions(NewPositiveCondition(r.getRolloutConditionType())) return } panic(fmt.Errorf(invalidRollingStateTransition, rollingState, event)) @@ -155,6 +183,7 @@ func (r *RolloutStatus) StateTransition(event RolloutEvent) { case InitializingState: if event == RollingInitializedEvent { r.RollingState = RollingInBatchesState + r.SetConditions(NewPositiveCondition(r.getRolloutConditionType())) return } panic(fmt.Errorf(invalidRollingStateTransition, rollingState, event)) @@ -166,6 +195,7 @@ func (r *RolloutStatus) StateTransition(event RolloutEvent) { case FinalisingState: if event == RollingFinalizedEvent { r.RollingState = RolloutSucceedState + r.SetConditions(NewPositiveCondition(r.getRolloutConditionType())) return } panic(fmt.Errorf(invalidRollingStateTransition, rollingState, event)) @@ -173,6 +203,7 @@ func (r *RolloutStatus) StateTransition(event RolloutEvent) { case RolloutSucceedState: if event == WorkloadModifiedEvent { r.RollingState = VerifyingState + r.SetConditions(NewPositiveCondition(r.getRolloutConditionType())) return } if event == RollingFinalizedEvent { @@ -184,6 +215,7 @@ func (r *RolloutStatus) StateTransition(event RolloutEvent) { case RolloutFailedState: if event == WorkloadModifiedEvent { r.RollingState = VerifyingState + r.SetConditions(NewPositiveCondition(r.getRolloutConditionType())) return } if event == RollingFailedEvent { @@ -203,6 +235,7 @@ func (r *RolloutStatus) batchStateTransition(event RolloutEvent) { if event == BatchRolloutFailedEvent { r.BatchRollingState = BatchRolloutFailedState r.RollingState = RolloutFailedState + r.SetConditions(NewNegativeCondition(r.getRolloutConditionType(), "failed")) return } switch batchRollingState { @@ -220,13 +253,15 @@ func (r *RolloutStatus) batchStateTransition(event RolloutEvent) { } if event == BatchRolloutVerifyingEvent { r.BatchRollingState = BatchVerifyingState + r.SetConditions(NewPositiveCondition(r.getRolloutConditionType())) return } panic(fmt.Errorf(invalidBatchRollingStateTransition, batchRollingState, event)) case BatchVerifyingState: if event == OneBatchAvailableEvent { - r.BatchRollingState = BatchReadyState + r.BatchRollingState = BatchFinalizingState + r.SetConditions(NewPositiveCondition(r.getRolloutConditionType())) return } if event == BatchRolloutVerifyingEvent { @@ -235,21 +270,24 @@ func (r *RolloutStatus) batchStateTransition(event RolloutEvent) { } panic(fmt.Errorf(invalidBatchRollingStateTransition, batchRollingState, event)) - case BatchReadyState: - if event == BatchRolloutApprovedEvent { - r.BatchRollingState = BatchFinalizeState - return - } - panic(fmt.Errorf(invalidBatchRollingStateTransition, batchRollingState, event)) - - case BatchFinalizeState: + case BatchFinalizingState: if event == FinishedOneBatchEvent { - r.BatchRollingState = BatchInitializingState + r.BatchRollingState = BatchReadyState + r.SetConditions(NewPositiveCondition(r.getRolloutConditionType())) return } if event == AllBatchFinishedEvent { // transition out of the batch loop r.RollingState = FinalisingState + r.SetConditions(NewPositiveCondition(r.getRolloutConditionType())) + return + } + panic(fmt.Errorf(invalidBatchRollingStateTransition, batchRollingState, event)) + + case BatchReadyState: + if event == BatchRolloutApprovedEvent { + r.BatchRollingState = BatchInitializingState + r.SetConditions(NewPositiveCondition(r.getRolloutConditionType())) return } panic(fmt.Errorf(invalidBatchRollingStateTransition, batchRollingState, event)) diff --git a/charts/vela-core/crds/core.oam.dev_applicationdeployments.yaml b/charts/vela-core/crds/core.oam.dev_applicationdeployments.yaml index 436d9dcf3..600221c5c 100644 --- a/charts/vela-core/crds/core.oam.dev_applicationdeployments.yaml +++ b/charts/vela-core/crds/core.oam.dev_applicationdeployments.yaml @@ -108,7 +108,7 @@ spec: description: Paused the rollout, default is false type: boolean rolloutBatches: - description: The exact distribution among batches. mutually exclusive to NumBatches + description: The exact distribution among batches. mutually exclusive to NumBatches. The total number cannot exceed the targetSize or the size of the source resource We will IGNORE the last batch's replica field if it's a percentage since round errors can lead to inaccurate sum We highly recommend to leave the last batch's replica field empty items: description: RolloutBatch is used to describe how the each batch rollout should be properties: @@ -210,7 +210,7 @@ spec: anyOf: - type: integer - type: string - description: 'Replicas is the number of pods to upgrade in this batch it can be an absolute number (ex: 5) or a percentage of total pods it is mutually exclusive with the PodList field' + description: 'Replicas is the number of pods to upgrade in this batch it can be an absolute number (ex: 5) or a percentage of total pods we will ignore the percentage of the last batch to just fill the gap it is mutually exclusive with the PodList field' x-kubernetes-int-or-string: true type: object type: array @@ -218,7 +218,7 @@ spec: description: RolloutStrategy defines strategies for the rollout plan type: string rolloutWebhooks: - description: RolloutWebhooks provides a way for the rollout to interact with an external process + description: RolloutWebhooks provide a way for the rollout to interact with an external process items: description: RolloutWebhook holds the reference to external checks used for canary analysis properties: diff --git a/charts/vela-core/crds/standard.oam.dev_rollouttraits.yaml b/charts/vela-core/crds/standard.oam.dev_rollouttraits.yaml index b768135ef..96b9e8f03 100644 --- a/charts/vela-core/crds/standard.oam.dev_rollouttraits.yaml +++ b/charts/vela-core/crds/standard.oam.dev_rollouttraits.yaml @@ -98,7 +98,7 @@ spec: description: Paused the rollout, default is false type: boolean rolloutBatches: - description: The exact distribution among batches. mutually exclusive to NumBatches + description: The exact distribution among batches. mutually exclusive to NumBatches. The total number cannot exceed the targetSize or the size of the source resource We will IGNORE the last batch's replica field if it's a percentage since round errors can lead to inaccurate sum We highly recommend to leave the last batch's replica field empty items: description: RolloutBatch is used to describe how the each batch rollout should be properties: @@ -200,7 +200,7 @@ spec: anyOf: - type: integer - type: string - description: 'Replicas is the number of pods to upgrade in this batch it can be an absolute number (ex: 5) or a percentage of total pods it is mutually exclusive with the PodList field' + description: 'Replicas is the number of pods to upgrade in this batch it can be an absolute number (ex: 5) or a percentage of total pods we will ignore the percentage of the last batch to just fill the gap it is mutually exclusive with the PodList field' x-kubernetes-int-or-string: true type: object type: array @@ -208,7 +208,7 @@ spec: description: RolloutStrategy defines strategies for the rollout plan type: string rolloutWebhooks: - description: RolloutWebhooks provides a way for the rollout to interact with an external process + description: RolloutWebhooks provide a way for the rollout to interact with an external process items: description: RolloutWebhook holds the reference to external checks used for canary analysis properties: diff --git a/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationdeployments.yaml b/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationdeployments.yaml index dc7edb12c..8e77ed672 100644 --- a/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationdeployments.yaml +++ b/legacy/charts/vela-core-legacy/crds/core.oam.dev_applicationdeployments.yaml @@ -108,7 +108,7 @@ spec: description: Paused the rollout, default is false type: boolean rolloutBatches: - description: The exact distribution among batches. mutually exclusive to NumBatches + description: The exact distribution among batches. mutually exclusive to NumBatches. The total number cannot exceed the targetSize or the size of the source resource We will IGNORE the last batch's replica field if it's a percentage since round errors can lead to inaccurate sum We highly recommend to leave the last batch's replica field empty items: description: RolloutBatch is used to describe how the each batch rollout should be properties: @@ -210,7 +210,7 @@ spec: anyOf: - type: integer - type: string - description: 'Replicas is the number of pods to upgrade in this batch it can be an absolute number (ex: 5) or a percentage of total pods it is mutually exclusive with the PodList field' + description: 'Replicas is the number of pods to upgrade in this batch it can be an absolute number (ex: 5) or a percentage of total pods we will ignore the percentage of the last batch to just fill the gap it is mutually exclusive with the PodList field' x-kubernetes-int-or-string: true type: object type: array @@ -218,7 +218,7 @@ spec: description: RolloutStrategy defines strategies for the rollout plan type: string rolloutWebhooks: - description: RolloutWebhooks provides a way for the rollout to interact with an external process + description: RolloutWebhooks provide a way for the rollout to interact with an external process items: description: RolloutWebhook holds the reference to external checks used for canary analysis properties: diff --git a/legacy/charts/vela-core-legacy/crds/standard.oam.dev_rollouttraits.yaml b/legacy/charts/vela-core-legacy/crds/standard.oam.dev_rollouttraits.yaml index 92da354d9..64148c813 100644 --- a/legacy/charts/vela-core-legacy/crds/standard.oam.dev_rollouttraits.yaml +++ b/legacy/charts/vela-core-legacy/crds/standard.oam.dev_rollouttraits.yaml @@ -98,7 +98,7 @@ spec: description: Paused the rollout, default is false type: boolean rolloutBatches: - description: The exact distribution among batches. mutually exclusive to NumBatches + description: The exact distribution among batches. mutually exclusive to NumBatches. The total number cannot exceed the targetSize or the size of the source resource We will IGNORE the last batch's replica field if it's a percentage since round errors can lead to inaccurate sum We highly recommend to leave the last batch's replica field empty items: description: RolloutBatch is used to describe how the each batch rollout should be properties: @@ -200,7 +200,7 @@ spec: anyOf: - type: integer - type: string - description: 'Replicas is the number of pods to upgrade in this batch it can be an absolute number (ex: 5) or a percentage of total pods it is mutually exclusive with the PodList field' + description: 'Replicas is the number of pods to upgrade in this batch it can be an absolute number (ex: 5) or a percentage of total pods we will ignore the percentage of the last batch to just fill the gap it is mutually exclusive with the PodList field' x-kubernetes-int-or-string: true type: object type: array @@ -208,7 +208,7 @@ spec: description: RolloutStrategy defines strategies for the rollout plan type: string rolloutWebhooks: - description: RolloutWebhooks provides a way for the rollout to interact with an external process + description: RolloutWebhooks provide a way for the rollout to interact with an external process items: description: RolloutWebhook holds the reference to external checks used for canary analysis properties: diff --git a/pkg/controller/common/rollout/rollout_plan_controller.go b/pkg/controller/common/rollout/rollout_plan_controller.go index 125047695..3df2613c8 100644 --- a/pkg/controller/common/rollout/rollout_plan_controller.go +++ b/pkg/controller/common/rollout/rollout_plan_controller.go @@ -14,6 +14,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" + "github.com/oam-dev/kubevela/pkg/controller/common" "github.com/oam-dev/kubevela/pkg/controller/common/rollout/workloads" "github.com/oam-dev/kubevela/pkg/oam" ) @@ -79,7 +80,7 @@ func (r *Controller) Reconcile(ctx context.Context) (res reconcile.Result, statu } }() - wc, err := r.GetWorkloadController() + workloadController, err := r.GetWorkloadController() if err != nil { r.rolloutStatus.RolloutFailed(err.Error()) r.recorder.Event(r.parentController, event.Warning("Unsupported workload", err)) @@ -88,18 +89,18 @@ func (r *Controller) Reconcile(ctx context.Context) (res reconcile.Result, statu switch r.rolloutStatus.RollingState { case v1alpha1.VerifyingState: - status = *wc.Verify(ctx) + status = *workloadController.Verify(ctx) case v1alpha1.InitializingState: // TODO: call the pre-rollout webhooks - status = *wc.Initialize(ctx) + status = *workloadController.Initialize(ctx) case v1alpha1.RollingInBatchesState: - status = r.reconcileBatchInRolling(ctx, wc) + status = r.reconcileBatchInRolling(ctx, workloadController) case v1alpha1.FinalisingState: // TODO: call the post-rollout webhooks - status = *wc.Finalize(ctx) + status = *workloadController.Finalize(ctx) case v1alpha1.RolloutSucceedState: // Nothing to do @@ -115,7 +116,7 @@ func (r *Controller) Reconcile(ctx context.Context) (res reconcile.Result, statu } // reconcile logic when we are in the middle of rollout -func (r *Controller) reconcileBatchInRolling(ctx context.Context, wc workloads.WorkloadController) ( +func (r *Controller) reconcileBatchInRolling(ctx context.Context, workloadController workloads.WorkloadController) ( status v1alpha1.RolloutStatus) { if r.rolloutSpec.Paused { @@ -125,7 +126,7 @@ func (r *Controller) reconcileBatchInRolling(ctx context.Context, wc workloads.W } // makes sure that the current batch and replica count in the status are validate - replicas, err := wc.Size(ctx) + replicas, err := workloadController.Size(ctx) if err != nil { r.rolloutStatus.RolloutRetry(err.Error()) return r.rolloutStatus @@ -138,21 +139,22 @@ func (r *Controller) reconcileBatchInRolling(ctx context.Context, wc workloads.W case v1alpha1.BatchInRollingState: // still rolling the batch, the batch rolling is not completed yet - status = *wc.RolloutOneBatchPods(ctx) + status = *workloadController.RolloutOneBatchPods(ctx) case v1alpha1.BatchVerifyingState: - // verifying if the application is ready to roll. - // This happens when it's either manual or automatic with analysis - // TODO: call the post-batch webhooks if there are any + // verifying if the application is ready to roll + // need to check if they meet the availability requirements in the rollout spec. + // TODO: evaluate any metrics/analysis + status = *workloadController.CheckOneBatchPods(ctx) + + case v1alpha1.BatchFinalizingState: + // all the pods in the are available + r.finalizeOneBatch() case v1alpha1.BatchReadyState: - // all the pods in the are upgraded and its state is ready - // need to check if they meet the availability requirements in the rollout spec - status = *wc.CheckOneBatchPods(ctx) - - case v1alpha1.BatchFinalizeState: - // indicates that all the pods in the are available, we can move on to the next batch - r.rolloutStatus.CurrentBatch++ + // all the pods in the are upgraded and their state are ready + // wait to move to the next batch if there are any + r.tryMovingToNextBatch() default: panic(fmt.Sprintf("illegal status %+v", r.rolloutStatus)) @@ -161,6 +163,36 @@ func (r *Controller) reconcileBatchInRolling(ctx context.Context, wc workloads.W return status } +// check if we can move to the next batch +func (r *Controller) tryMovingToNextBatch() { + if r.rolloutSpec.BatchPartition == nil || *r.rolloutSpec.BatchPartition > r.rolloutStatus.CurrentBatch { + klog.InfoS("ready to rollout the next batch", "current batch", r.rolloutStatus.CurrentBatch) + r.rolloutStatus.CurrentBatch++ + r.rolloutStatus.StateTransition(v1alpha1.BatchRolloutApprovedEvent) + } else { + klog.V(common.LogDebug).InfoS("the current batch is waiting to move on", "current batch", + r.rolloutStatus.CurrentBatch) + } +} + +func (r *Controller) finalizeOneBatch() { + // TODO: call the post-batch webhooks if there are any + currentBatch := int(r.rolloutStatus.CurrentBatch) + if currentBatch == len(r.rolloutSpec.RolloutBatches)-1 { + // this is the last batch, mark the rollout finalized + r.rolloutStatus.StateTransition(v1alpha1.AllBatchFinishedEvent) + r.recorder.Event(r.parentController, event.Normal("all batches rolled out", + fmt.Sprintf("upgrade pod = %d, total ready pod = %d", r.rolloutStatus.UpgradedReplicas, + r.rolloutStatus.UpgradedReadyReplicas))) + } else { + klog.InfoS("finished one batch rollout", "current batch", r.rolloutStatus.CurrentBatch) + // th + r.recorder.Event(r.parentController, event.Normal("Batch finalized", + fmt.Sprintf("the batch num = %d is ready", r.rolloutStatus.CurrentBatch))) + r.rolloutStatus.StateTransition(v1alpha1.FinishedOneBatchEvent) + } +} + // verify that the upgradedReplicas and current batch in the status are valid according to the spec func (r *Controller) validateRollingBatchStatus(totalSize int) bool { status := r.rolloutStatus @@ -187,9 +219,14 @@ func (r *Controller) validateRollingBatchStatus(totalSize int) bool { return false } // calculate the upper bound with the current batch - batchSize, _ := intstr.GetValueFromIntOrPercent(&spec.RolloutBatches[currentBatch].Replicas, - totalSize, true) - podCount += batchSize + if currentBatch == len(spec.RolloutBatches)-1 { + // avoid round up problems + podCount = totalSize + } else { + batchSize, _ := intstr.GetValueFromIntOrPercent(&spec.RolloutBatches[currentBatch].Replicas, + totalSize, true) + podCount += batchSize + } // the recorded number should be not as much as the all the pods including the active batch if podCount < upgradedReplicas { klog.ErrorS(fmt.Errorf("the upgraded replica in the status is too large"), "upgraded num status", diff --git a/pkg/controller/common/rollout/workloads/cloneset_controller.go b/pkg/controller/common/rollout/workloads/cloneset_controller.go index 831763430..213870deb 100644 --- a/pkg/controller/common/rollout/workloads/cloneset_controller.go +++ b/pkg/controller/common/rollout/workloads/cloneset_controller.go @@ -13,6 +13,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" + "github.com/oam-dev/kubevela/pkg/controller/common" "github.com/oam-dev/kubevela/pkg/oam" ) @@ -58,40 +59,50 @@ func (c *CloneSetController) Size(ctx context.Context) (int32, error) { // Verify verifies that the target rollout resource is consistent with the rollout spec func (c *CloneSetController) Verify(ctx context.Context) *v1alpha1.RolloutStatus { - if c.fetchCloneSet(ctx) != nil { + var verifyErr error + defer func() { + if verifyErr != nil { + klog.Error(verifyErr) + c.recorder.Event(c.parentController, event.Warning("VerifyFailed", verifyErr)) + } + }() + + if verifyErr = c.fetchCloneSet(ctx); verifyErr != nil { return c.rolloutStatus } // make sure that there are changes in the pod template targetHash := c.cloneSet.Status.UpdateRevision if targetHash == c.rolloutStatus.LastAppliedPodTemplateIdentifier { - err := fmt.Errorf("there is no difference between the source and target, hash = %s", targetHash) - klog.Error(err) - c.rolloutStatus.RolloutFailed(err.Error()) - c.recorder.Event(c.parentController, event.Warning("VerifyFailed", err)) + verifyErr = fmt.Errorf("there is no difference between the source and target, hash = %s", targetHash) + c.rolloutStatus.RolloutFailed(verifyErr.Error()) return c.rolloutStatus } // record the new pod template hash c.rolloutStatus.NewPodTemplateIdentifier = targetHash // check if the rollout spec is compatible with the current state - // 1. the rollout batch is either automatic or zero - if c.rolloutSpec.BatchPartition != nil && *c.rolloutSpec.BatchPartition != 0 { - err := fmt.Errorf("the rollout plan has to start from zero, partition= %d", *c.rolloutSpec.BatchPartition) - klog.Error(err) - c.rolloutStatus.RolloutFailed(err.Error()) - c.recorder.Event(c.parentController, event.Warning("VerifyFailed", err)) + totalReplicas, _ := c.Size(ctx) + + // check if the target spec is the same as the Cloneset replicas + if verifyErr = c.verifyBatchSizes(totalReplicas); verifyErr != nil { + c.rolloutStatus.RolloutFailed(verifyErr.Error()) return c.rolloutStatus } - // 2. the number of old version in the Cloneset equals to the total number - totalReplicas, _ := c.Size(ctx) + + // the rollout batch partition is either automatic or zero + if c.rolloutSpec.BatchPartition != nil && *c.rolloutSpec.BatchPartition != 0 { + verifyErr = fmt.Errorf("the rollout plan has to start from zero, partition= %d", *c.rolloutSpec.BatchPartition) + c.rolloutStatus.RolloutFailed(verifyErr.Error()) + return c.rolloutStatus + } + + // the number of old version in the Cloneset equals to the total number oldVersionPod, _ := intstr.GetValueFromIntOrPercent(c.cloneSet.Spec.UpdateStrategy.Partition, int(totalReplicas), true) if oldVersionPod != int(totalReplicas) { - err := fmt.Errorf("the cloneset was still in the middle of updating, number of old pods= %d", oldVersionPod) - klog.Error(err) - c.rolloutStatus.RolloutFailed(err.Error()) - c.recorder.Event(c.parentController, event.Warning("VerifyFailed", err)) + verifyErr = fmt.Errorf("the cloneset was still in the middle of updating, number of old pods= %d", oldVersionPod) + c.rolloutStatus.RolloutFailed(verifyErr.Error()) return c.rolloutStatus } @@ -126,12 +137,14 @@ func (c *CloneSetController) RolloutOneBatchPods(ctx context.Context) *v1alpha1. IntVal: cloneSetSize - int32(newPodTarget)} // patch the Cloneset if err := c.client.Patch(ctx, c.cloneSet, clonePatch, client.FieldOwner(c.parentController.GetUID())); err != nil { - c.recorder.Event(c.parentController, event.Warning("Failed to update the Cloneset", err)) + c.recorder.Event(c.parentController, event.Warning("Failed to patch update the Cloneset", err)) c.rolloutStatus.RolloutRetry(err.Error()) - c.rolloutStatus.StateTransition(v1alpha1.BatchRolloutContinueEvent) return c.rolloutStatus } // record the upgrade + klog.InfoS("upgraded one batch", "current batch", c.rolloutStatus.CurrentBatch) + c.recorder.Event(c.parentController, event.Normal("Rollout", + fmt.Sprintf("upgraded the batch num = %d", c.rolloutStatus.CurrentBatch))) c.rolloutStatus.StateTransition(v1alpha1.BatchRolloutVerifyingEvent) c.rolloutStatus.UpgradedReplicas = int32(newPodTarget) return c.rolloutStatus @@ -148,37 +161,64 @@ func (c *CloneSetController) CheckOneBatchPods(ctx context.Context) *v1alpha1.Ro if currentBatch.MaxUnavailable != nil { unavail, _ = intstr.GetValueFromIntOrPercent(currentBatch.MaxUnavailable, int(cloneSetSize), true) } - klog.InfoS("checking the rolling out progress", "new pod count target", newPodTarget, - "new ready pod count", readyPodCount, "max unavailable pod allowed", unavail) + klog.V(common.LogDebug).InfoS("checking the rolling out progress", "current batch", currentBatch, + "new pod count target", newPodTarget, "new ready pod count", readyPodCount, + "max unavailable pod allowed", unavail) c.rolloutStatus.UpgradedReadyReplicas = int32(readyPodCount) if unavail+readyPodCount >= newPodTarget { // record the successful upgrade + klog.InfoS("pods are ready", "current batch", currentBatch) + c.recorder.Event(c.parentController, event.Normal("Batch Available", + fmt.Sprintf("the batch num = %d is available", c.rolloutStatus.CurrentBatch))) c.rolloutStatus.StateTransition(v1alpha1.OneBatchAvailableEvent) } else { // continue to verify + klog.V(common.LogDebug).InfoS("the batch is not ready yet", "current batch", currentBatch) c.rolloutStatus.StateTransition(v1alpha1.BatchRolloutVerifyingEvent) } return c.rolloutStatus } -// Finalize makes sure the Cloneset is all upgraded and +// FinalizeOneBatch makes sure that the rollout status are updated correctly +func (c *CloneSetController) FinalizeOneBatch(ctx context.Context) *v1alpha1.RolloutStatus { + // nothing to do for now + return c.rolloutStatus +} + +// Finalize makes sure the Cloneset is all upgraded func (c *CloneSetController) Finalize(ctx context.Context) *v1alpha1.RolloutStatus { if c.fetchCloneSet(ctx) != nil { return c.rolloutStatus } - // mark the rollout finalized - c.recorder.Event(c.parentController, event.Normal("Finalized", "Rollout resource are finalized")) + c.rolloutStatus.StateTransition(v1alpha1.RollingFinalizedEvent) + return c.rolloutStatus } -// The functions below are helper functions +/* -------------------- +The functions below are helper functions +--------------------- */ +// check if the replicas in all the rollout batches add up to the right number +func (c *CloneSetController) verifyBatchSizes(totalReplicas int32) error { + // the target size has to be the same as the cloneset size + if c.rolloutSpec.TargetSize != nil && *c.rolloutSpec.TargetSize != totalReplicas { + return fmt.Errorf("the rollout plan is attempting to scale the cloneset, target = %d, cloneset size = %d", + *c.rolloutSpec.TargetSize, totalReplicas) + } + // use a common function to check if the sum of all the batches can match the cloneset size + err := VerifySumOfBatchSizes(c.rolloutSpec, totalReplicas) + if err != nil { + return err + } + return nil +} + func (c *CloneSetController) fetchCloneSet(ctx context.Context) error { // get the cloneSet workload := kruise.CloneSet{} err := c.client.Get(ctx, c.workloadNamespacedName, &workload) if err != nil { - klog.CalculateMaxSize() if !apierrors.IsNotFound(err) { c.recorder.Event(c.parentController, event.Warning("Failed to get the Cloneset", err)) } @@ -190,16 +230,24 @@ func (c *CloneSetController) fetchCloneSet(ctx context.Context) error { } func (c *CloneSetController) calculateNewPodTarget(cloneSetSize int) int { - currentBatch := c.rolloutStatus.CurrentBatch + currentBatch := int(c.rolloutStatus.CurrentBatch) newPodTarget := 0 - for i, r := range c.rolloutSpec.RolloutBatches { - batchSize, _ := intstr.GetValueFromIntOrPercent(&r.Replicas, cloneSetSize, true) - if i <= int(currentBatch) { - newPodTarget += batchSize - } else { - break + if currentBatch == len(c.rolloutSpec.RolloutBatches)-1 { + // special handle the last batch, we ignore the rest of the batch in case there are rounding errors + klog.InfoS("use the cloneset size as the total pod target for the last rolling batch", + "current batch", currentBatch, "new version pod target", newPodTarget) + newPodTarget = cloneSetSize + } else { + for i, r := range c.rolloutSpec.RolloutBatches { + batchSize, _ := intstr.GetValueFromIntOrPercent(&r.Replicas, cloneSetSize, true) + if i <= currentBatch { + newPodTarget += batchSize + } else { + break + } } + klog.InfoS("Calculated the number of new version pod", "current batch", currentBatch, + "new version pod target", newPodTarget) } - klog.InfoS("Calculated the number of new version pod", "new version pod target", newPodTarget) return newPodTarget } diff --git a/pkg/controller/common/rollout/workloads/common.go b/pkg/controller/common/rollout/workloads/common.go new file mode 100644 index 000000000..0d66b4c84 --- /dev/null +++ b/pkg/controller/common/rollout/workloads/common.go @@ -0,0 +1,39 @@ +package workloads + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/util/intstr" + + "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" +) + +// VerifySumOfBatchSizes verifies that the the sum of all the batch replicas is valid given the total replica +// each batch replica can be absolute or a percentage +func VerifySumOfBatchSizes(rolloutSpec *v1alpha1.RolloutPlan, totalReplicas int32) error { + // if not set, the sum of all the batch sizes minus the last batch cannot be more than the totalReplicas + // if not set, the sum of all the batch sizes minus the last batch cannot be more than the totalReplicas + totalRollout := 0 + for i := 0; i < len(rolloutSpec.RolloutBatches)-1; i++ { + rb := rolloutSpec.RolloutBatches[i] + batchSize, _ := intstr.GetValueFromIntOrPercent(&rb.Replicas, int(totalReplicas), true) + totalRollout += batchSize + } + if totalRollout >= int(totalReplicas) { + return fmt.Errorf("the rollout plan batch size mismatch, total batch size = %d, totalReplicas size = %d", + totalRollout, totalReplicas) + } + + // include the last batch if it has an int value + // we ignore the last batch percentage since it is very likely to cause rounding errors + lastBatch := rolloutSpec.RolloutBatches[len(rolloutSpec.RolloutBatches)-1] + if lastBatch.Replicas.Type == intstr.Int { + totalRollout += int(lastBatch.Replicas.IntVal) + // now that they should be the same + if totalRollout != int(totalReplicas) { + return fmt.Errorf("the rollout plan batch size mismatch, total batch size = %d, totalReplicas size = %d", + totalRollout, totalReplicas) + } + } + return nil +} diff --git a/pkg/controller/common/rollout/workloads/controller.go b/pkg/controller/common/rollout/workloads/controller.go index e49558f8d..99f9da3fb 100644 --- a/pkg/controller/common/rollout/workloads/controller.go +++ b/pkg/controller/common/rollout/workloads/controller.go @@ -30,6 +30,10 @@ type WorkloadController interface { // it returns the number of pods upgraded in this round CheckOneBatchPods(ctx context.Context) *v1alpha1.RolloutStatus + // FinalizeOneBatch makes sure that the rollout can start the next batch + // it also needs to handle the corner cases around the very last batch + FinalizeOneBatch(ctx context.Context) *v1alpha1.RolloutStatus + // Finalize makes sure the resources are in a good final state. // For example, we may remove the source object to prevent scalar traits to ever work // and we will call the finalize rollout web hooks diff --git a/pkg/webhook/common/rollout/rollout_plan.go b/pkg/webhook/common/rollout/rollout_plan.go index bda2d23aa..8fe3620f3 100644 --- a/pkg/webhook/common/rollout/rollout_plan.go +++ b/pkg/webhook/common/rollout/rollout_plan.go @@ -1,27 +1,85 @@ package rollout import ( + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/apimachinery/pkg/util/validation/field" "github.com/oam-dev/kubevela/apis/standard.oam.dev/v1alpha1" ) // DefaultRolloutPlan set the default values for a rollout plan +// This is called by the mutation webhooks and before the validators func DefaultRolloutPlan(rollout *v1alpha1.RolloutPlan) { - + if rollout.TargetSize != nil && rollout.NumBatches != nil && rollout.RolloutBatches == nil { + // create the rollout batch based on the total size and num batches if it's not set + // leave it for the validator to reject if they are both set + numBatches := int(*rollout.NumBatches) + totalSize := int(*rollout.TargetSize) + // create the batch array + rollout.RolloutBatches = make([]v1alpha1.RolloutBatch, int(*rollout.NumBatches)) + avg := intstr.FromInt(totalSize / numBatches) + total := 0 + for i := 0; i < numBatches-1; i++ { + rollout.RolloutBatches[i].Replicas = avg + total += avg.IntValue() + } + // fill out the last batch + rollout.RolloutBatches[numBatches-1].Replicas = intstr.FromInt(totalSize - total) + } } // ValidateCreate validate the rollout plan -func ValidateCreate(rollout *v1alpha1.RolloutPlan) field.ErrorList { +func ValidateCreate(rollout *v1alpha1.RolloutPlan, rootPath *field.Path) field.ErrorList { var allErrs field.ErrorList - // 1. The total number of num in the batches match the current target resource pod size - // 2. The TargetSize and NumBatches are mutually exclusive to RolloutBatches + // The total number of num in the batches match the current target resource pod size + + // The TargetSize and NumBatches are mutually exclusive to RolloutBatches + if rollout.NumBatches != nil && rollout.RolloutBatches != nil { + allErrs = append(allErrs, field.Duplicate(rootPath.Child("numBatches"), rollout.NumBatches)) + } + + // validate the webhooks + allErrs = append(allErrs, validateWebhook(rollout, rootPath)...) return allErrs } +func validateWebhook(rollout *v1alpha1.RolloutPlan, rootPath *field.Path) (allErrs field.ErrorList) { + // The webhooks in the rollout plan can only be initialize or finalize webhooks + if rollout.RolloutWebhooks != nil { + webhookPath := rootPath.Child("rolloutWebhooks") + for i, rw := range rollout.RolloutWebhooks { + if rw.Type != v1alpha1.InitializeRolloutHook && rw.Type != v1alpha1.FinalizeRolloutHook { + allErrs = append(allErrs, field.Invalid(webhookPath.Index(i), + rw.Type, "the rollout webhook type can only be initialize or finalize webhook")) + } + // TODO: check the URL/name uniqueness? + } + } + + // The webhooks in the rollout batch can only be pre or post batch types + if rollout.RolloutBatches != nil { + batchesPath := rootPath.Child("rolloutBatches") + for i, rb := range rollout.RolloutBatches { + rolloutBatchPath := batchesPath.Index(i) + for j, brw := range rb.BatchRolloutWebhooks { + if brw.Type != v1alpha1.PostBatchRolloutHook && brw.Type != v1alpha1.PreBatchRolloutHook { + allErrs = append(allErrs, field.Invalid(rolloutBatchPath.Child("batchRolloutWebhooks").Index(j), + brw.Type, "the batch webhook type can only be pre or post batch webhook")) + } + // TODO: check the URL/name uniqueness? + } + } + } + return allErrs +} + // ValidateUpdate validate if one can change the rollout plan from the previous psec -func ValidateUpdate(new *v1alpha1.RolloutPlan, prev *v1alpha1.RolloutPlan) field.ErrorList { - // Only a few fields can change after a rollout plan is set - return nil +func ValidateUpdate(new *v1alpha1.RolloutPlan, prev *v1alpha1.RolloutPlan, rootPath *field.Path) field.ErrorList { + // makes sure the new rollout alone is valid + allErrs := ValidateCreate(new, rootPath) + + // TODO: Enforce that only a few fields can change after a rollout plan is set + + return allErrs } diff --git a/pkg/webhook/core.oam.dev/v1alpha2/applicationdeployment/validation.go b/pkg/webhook/core.oam.dev/v1alpha2/applicationdeployment/validation.go index 14079673d..2ca9f7ff1 100644 --- a/pkg/webhook/core.oam.dev/v1alpha2/applicationdeployment/validation.go +++ b/pkg/webhook/core.oam.dev/v1alpha2/applicationdeployment/validation.go @@ -53,7 +53,7 @@ func (h *ValidatingHandler) ValidateCreate(appDeploy *v1alpha2.ApplicationDeploy fldPath.Child("componentList"))...) // validate the rollout plan spec - allErrs = append(allErrs, rollout.ValidateCreate(&appDeploy.Spec.RolloutPlan)...) + allErrs = append(allErrs, rollout.ValidateCreate(&appDeploy.Spec.RolloutPlan, fldPath.Child("rolloutPlan"))...) return allErrs } @@ -113,5 +113,6 @@ func (h *ValidatingHandler) ValidateUpdate(new, old *v1alpha2.ApplicationDeploym if len(errList) > 0 { return errList } - return rollout.ValidateUpdate(&new.Spec.RolloutPlan, &old.Spec.RolloutPlan) + fldPath := field.NewPath("spec").Child("rolloutPlan") + return rollout.ValidateUpdate(&new.Spec.RolloutPlan, &old.Spec.RolloutPlan, fldPath) }