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

update api for future bluegreen #214

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Conversation

myname4423
Copy link
Contributor

@myname4423 myname4423 commented May 24, 2024

Ⅰ. Describe what this PR does

this PR does the following update/modification:

  1. add the bluegreen Strategy and bluegreen status to rollout v1beta1 api
  2. add some helper functions to Get and Set value of strategy and status for strategy-agnostic access
  3. change the BatchRelease.spec.releasePlan.ExtraWorkload field to Rollingstyle to support bluegreen style
  4. add NextStepIndex and FinalisingStep fields to canary status and bluegreen status for later use, with related function added and unit test updated

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Special notes for reviews

@kruise-bot kruise-bot requested review from furykerry and zmberg May 24, 2024 03:54
@myname4423 myname4423 force-pushed the bluegreen branch 2 times, most recently from 0a5cc67 to c1676b6 Compare May 24, 2024 08:47
Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 35.13514% with 48 lines in your changes missing coverage. Please review.

Project coverage is 43.41%. Comparing base (07c1731) to head (c1676b6).
Report is 2 commits behind head on master.

Current head c1676b6 differs from pull request most recent head 9c50efa

Please upload reports for the commit 9c50efa to get more accurate results.

Files Patch % Lines
pkg/controller/rollout/rollout_status.go 0.00% 24 Missing ⚠️
pkg/controller/rollout/rollout_progressing.go 48.00% 11 Missing and 2 partials ⚠️
pkg/util/rollout_utils.go 0.00% 6 Missing ⚠️
...g/controller/batchrelease/batchrelease_executor.go 61.53% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
- Coverage   43.63%   43.41%   -0.22%     
==========================================
  Files          52       52              
  Lines        5681     5721      +40     
==========================================
+ Hits         2479     2484       +5     
- Misses       2778     2813      +35     
  Partials      424      424              
Flag Coverage Δ
unittests 43.41% <35.13%> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CanaryReplicas int32 `json:"updatedReplicas"`
// CanaryReadyReplicas the numbers of ready canary revision pods
CanaryReadyReplicas int32 `json:"updatedReadyReplicas"`
// FinalisingStep the step of finalising
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz also change the internal field name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

api/v1beta1/rollout_types.go Show resolved Hide resolved
@@ -231,10 +328,22 @@ const (
// Terminating Reason
TerminatingReasonInTerminating = "InTerminating"
TerminatingReasonCompleted = "Completed"

// Finalise Reason
FinaliseReasonSuccess = "Success"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz comment each FinalizeReason

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

type FinalisingStepType string

const (
FinalisingStepTypePreparing FinalisingStepType = "Preparing"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz comment about each step and the common workflow that involves multiple steps

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, and workflow related comment will be referenced in the upcoming pull request (PR) and included in
a documentation as well.

}
allSteps := int32(len(rollout.Spec.Strategy.GetSteps()))
if CurrentStepIndex >= allSteps {
return 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz comment about the wrapping logic, why the next index of last step is 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we set the NextStepIndex of last step to -1, so as to avoid confusion

}
dst.Annotations[RolloutStyleAnnotation] = strings.ToLower(string(srcV1beta1.Spec.ReleasePlan.RollingStyle))
dst.Spec.ReleasePlan.RollingStyle = RollingStyleType(srcV1beta1.Spec.ReleasePlan.RollingStyle)
// if srcV1beta1.Spec.ReleasePlan.EnableExtraWorkloadForCanary {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unused code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

api/v1alpha1/rollout_types.go Show resolved Hide resolved
@myname4423 myname4423 force-pushed the bluegreen branch 2 times, most recently from fc3e5e1 to 36d5d43 Compare May 27, 2024 13:40
@myname4423 myname4423 force-pushed the bluegreen branch 2 times, most recently from 2d045c9 to ba359f3 Compare May 29, 2024 09:47
// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OriginalSetting -> DeploymentStrategy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated


// 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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rollouts.kruise.io/original-setting -> rollouts.kruise.io/original-deployment-strategy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -52,13 +62,42 @@ type DeploymentStrategy struct {
Partition intstr.IntOrString `json:"partition,omitempty"`
}

// OriginalSetting storages part of the fileds of a workload,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// DeploymentStrategy stores deployment strategy related fields of a workload

Copy link
Contributor Author

@myname4423 myname4423 Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@@ -37,6 +37,16 @@ const (
// AdvancedDeploymentControlLabel is label for deployment,
// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz replace all storage vb with store

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

// Get the rolling style based on the strategy
func (r *RolloutStrategy) GetRollingStyle() RollingStyleType {
if r.BlueGreen == nil && r.Canary == nil {
return EmptyRollingStyle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should include a webhook to ensure that either bluegreen or canary field is set

Copy link
Contributor Author

@myname4423 myname4423 Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

included

}
allSteps := int32(len(rollout.Spec.Strategy.GetSteps()))
if CurrentStepIndex >= allSteps {
return 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we set the NextStepIndex of last step to -1, so as to avoid confusion

api/v1alpha1/rollout_types.go Show resolved Hide resolved
api/v1beta1/batchrelease_plan_types.go Show resolved Hide resolved
@@ -243,27 +348,123 @@ type CanaryStatus struct {
RolloutHash string `json:"rolloutHash,omitempty"`
// StableRevision indicates the revision of stable pods
StableRevision string `json:"stableRevision,omitempty"`
// pod template hash is used as service selector label
PodTemplateHash string `json:"podTemplateHash"`
// CurrentStepIndex defines the current step of the rollout is on. If the current step index is null, the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current step index is int, it cannot be null, maybe you mean is zero here ?

api/v1beta1/rollout_types.go Show resolved Hide resolved
api/v1beta1/rollout_types.go Show resolved Hide resolved
pkg/util/rollout_utils.go Show resolved Hide resolved
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to allow set 0% for canary strategy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we didn't allow set 0% from the beginning. i indeed tried to allow this for canary strategy, however, we need consider much more scenarios about traffic, which make the code complex and hard to maintain

@myname4423 myname4423 force-pushed the bluegreen branch 3 times, most recently from 7de1c40 to f3b29f3 Compare June 11, 2024 09:07
return br
},
},
}

for _, cs := range cases {
for i, cs := range cases {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why only select one case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my miss, updated

// RollingStyle can be "Canary", "Partiton" or "BlueGreen"
RollingStyle RollingStyleType `json:"rollingStyle,omitempty"`
// When user verifies that the canary version is ready, we will remove the canary deployment and release the deployment workload-demo in full.
// Current only support k8s native deployment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz comment the relationship with rollingStyle and enableExtraWorkloadForCanary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@furykerry
Copy link
Member

plz squash commits into one

Signed-off-by: yunbo <[email protected]>

add status conversion

Signed-off-by: yunbo <[email protected]>

nextStepIndex default value from 0 to -1

Signed-off-by: yunbo <[email protected]>

restore the enableExtra field in BR

Signed-off-by: yunbo <[email protected]>
@zmberg
Copy link
Member

zmberg commented Jun 12, 2024

/lgtm

@zmberg
Copy link
Member

zmberg commented Jun 12, 2024

/approve

@kruise-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kruise-bot kruise-bot merged commit 1e8af4a into openkruise:master Jun 12, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants