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

Rolling Deployment in partition-style #115

Merged

Conversation

veophi
Copy link
Member

@veophi veophi commented Jan 18, 2023

Signed-off-by: mingzhou.swx [email protected]

Ⅰ. Describe what this PR does

We can rolling Deployment just like CloneSet/StatefulSet using partition!

Ⅱ. Does this pull request fix one issue?

fixes #15

@kruise-bot kruise-bot requested review from FillZpp and zmberg January 18, 2023 12:37
@veophi veophi force-pushed the rolling-deployment-in-partition-style branch 2 times, most recently from dc47c06 to 07380e0 Compare January 18, 2023 14:02
@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2023

Codecov Report

Attention: Patch coverage is 42.24344% with 242 lines in your changes missing coverage. Please review.

Project coverage is 44.66%. Comparing base (843e8b8) to head (55d9e02).
Report is 50 commits behind head on master.

Files Patch % Lines
pkg/util/controller_finder.go 0.00% 53 Missing ⚠️
pkg/util/patch/patch_utils.go 47.95% 37 Missing and 14 partials ⚠️
pkg/controller/deployment/controller.go 0.00% 27 Missing ⚠️
.../controller/deployment/deployment_event_handler.go 0.00% 24 Missing ⚠️
...bhook/workload/mutating/workload_update_handler.go 45.45% 21 Missing and 3 partials ⚠️
...lease/control/partitionstyle/deployment/control.go 75.00% 15 Missing and 7 partials ⚠️
pkg/util/workloads_utils.go 37.93% 17 Missing and 1 partial ⚠️
...g/controller/batchrelease/batchrelease_executor.go 25.00% 10 Missing and 2 partials ⚠️
pkg/controller/rollout/rollout_canary.go 16.66% 3 Missing and 2 partials ⚠️
...chrelease/control/canarystyle/deployment/stable.go 40.00% 2 Missing and 1 partial ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
- Coverage   44.74%   44.66%   -0.08%     
==========================================
  Files          44       47       +3     
  Lines        4519     4865     +346     
==========================================
+ Hits         2022     2173     +151     
- Misses       2158     2335     +177     
- Partials      339      357      +18     
Flag Coverage Δ
unittests 44.66% <42.24%> (-0.08%) ⬇️

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.

@veophi veophi force-pushed the rolling-deployment-in-partition-style branch from 07380e0 to 96b63c7 Compare January 30, 2023 03:44
@veophi veophi force-pushed the rolling-deployment-in-partition-style branch 2 times, most recently from 7118c6b to 28e4afc Compare February 1, 2023 09:08
@zmberg
Copy link
Member

zmberg commented Feb 2, 2023

/lgtm

release, release.GetObjectKind().GroupVersionKind()))

// Disable the native deployment controller
d.Spec.Strategy = apps.DeploymentStrategy{Type: apps.RecreateDeploymentStrategyType}
Copy link
Member

Choose a reason for hiding this comment

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

webhook will also set strategy to recreate, is it necessary to duplicate the work here, or controller should duplicate the whole work here (also set pause=true) to avoid potential webhook failure

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -190,7 +271,6 @@ func (r *ControllerFinder) getDeployment(namespace string, ref *rolloutv1alpha1.
workload.InRolloutProgressing = true
// workload is continuous release, indicates rollback(v1 -> v2 -> v1)
// delete auto-generated labels
Copy link
Member

Choose a reason for hiding this comment

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

remove comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

d := rc.object.DeepCopy()
strategy.Partition = ctx.DesiredPartition
d.Annotations[v1alpha1.DeploymentStrategyAnnotation] = util.DumpJSON(&strategy)
return rc.client.Patch(context.TODO(), d, client.MergeFrom(rc.object))
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 use StrategicMergeFrom from native k8s objects

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@veophi veophi force-pushed the rolling-deployment-in-partition-style branch from 28e4afc to e658912 Compare February 8, 2023 12:37
@kruise-bot kruise-bot removed the lgtm label Feb 8, 2023
@veophi veophi force-pushed the rolling-deployment-in-partition-style branch from e658912 to 2d834a5 Compare February 8, 2023 12:44
Copy link
Member

@furykerry furykerry left a comment

Choose a reason for hiding this comment

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

/lgtm

@kruise-bot kruise-bot added the lgtm label Feb 9, 2023
@veophi veophi force-pushed the rolling-deployment-in-partition-style branch from 2d834a5 to 2a52b0c Compare February 9, 2023 04:05
@kruise-bot kruise-bot removed the lgtm label Feb 9, 2023
@veophi veophi force-pushed the rolling-deployment-in-partition-style branch 2 times, most recently from 9986217 to 5eb80c6 Compare February 9, 2023 08:15
pkg/controller/deployment/controller.go Outdated Show resolved Hide resolved
pkg/controller/deployment/controller.go Outdated Show resolved Hide resolved
pkg/controller/deployment/controller.go Outdated Show resolved Hide resolved
pkg/controller/deployment/controller.go Outdated Show resolved Hide resolved
@veophi veophi force-pushed the rolling-deployment-in-partition-style branch from 5eb80c6 to b5e93f1 Compare February 9, 2023 10:02
main.go Outdated Show resolved Hide resolved
@veophi veophi force-pushed the rolling-deployment-in-partition-style branch from b5e93f1 to e5613dc Compare February 9, 2023 11:44
@veophi veophi force-pushed the rolling-deployment-in-partition-style branch from e5613dc to 55d9e02 Compare February 9, 2023 11:52
@zmberg
Copy link
Member

zmberg commented Feb 10, 2023

/lgtm

@zmberg
Copy link
Member

zmberg commented Feb 10, 2023

/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 c56e2f3 into openkruise:master Feb 10, 2023
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.

Improve the current Deployment batch release solution
5 participants