Skip to content

Implement k8s multi cluster rollback #5765

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

Merged
merged 7 commits into from
May 8, 2025

Conversation

ffjlabo
Copy link
Member

@ffjlabo ffjlabo commented Apr 21, 2025

What this PR does:

I implemented the rollback stage for the k8s multicluster plugin.

PipeCD

Spec summary

  • Deploy the previous version for each deploy target at the same time.
  • Not pruning to be as safe as possible. (same as k8s plugin)

RFC: https://github.com/pipe-cd/pipecd/blob/master/docs/rfcs/0014-multi-cluster-deployment-for-k8s.md

Why we need it:

We want to support the feature to deploy multi-cluster for k8s.

Which issue(s) this PR fixes:

Part of #5006

Does this PR introduce a user-facing change?:

  • How are users affected by this change:
  • Is this breaking change:
  • How to migrate (if breaking change):

@ffjlabo ffjlabo force-pushed the implement-k8s-multi-cluster-rollback branch from ed689d9 to 9751bd9 Compare April 21, 2025 10:54
Copy link

codecov bot commented Apr 21, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 13 lines in your changes missing coverage. Please review.

Project coverage is 27.23%. Comparing base (e54cd83) to head (9a2c241).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
pkg/plugin/sdk/deployment.go 0.00% 10 Missing ⚠️
...gin/kubernetes_multicluster/deployment/rollback.go 96.29% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5765      +/-   ##
==========================================
+ Coverage   27.14%   27.23%   +0.09%     
==========================================
  Files         506      506              
  Lines       53705    53788      +83     
==========================================
+ Hits        14577    14651      +74     
- Misses      38013    38022       +9     
  Partials     1115     1115              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ffjlabo ffjlabo force-pushed the implement-k8s-multi-cluster-rollback branch from 9751bd9 to 9c68f91 Compare April 23, 2025 06:30
@ffjlabo ffjlabo marked this pull request as ready for review April 23, 2025 07:21
})
}

if err := eg.Wait(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

[ASK] What if one rollback fails and others are still rolling back?

Cancel all of them? Or still continue rolback?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for pointing that out!
The current implementation is cancelling all.

I forgot to implement to continue rollback. will do it.

BTW, I described that in the rfc 🙏
https://github.com/pipe-cd/pipecd/blob/master/docs/rfcs/0014-multi-cluster-deployment-for-k8s.md

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed 362b45e

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, i understood here:

You can select the following behavior by setting the stage.
Rollback if any app fails
Rollback if all apps fail

@ffjlabo ffjlabo force-pushed the implement-k8s-multi-cluster-rollback branch from aa52939 to 362b45e Compare April 24, 2025 08:28
@ffjlabo ffjlabo requested a review from t-kikuc April 24, 2025 09:12
Comment on lines +105 to +108
// success at least one of the rollback succeed
if result.status != sdk.StageStatusFailure {
finalStatus = sdk.StageStatusSuccess
}
Copy link
Member

Choose a reason for hiding this comment

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

[ASK] Which should we choose here? (I don't have a strong opinion)

  • a. SUCCESS if at least one succeeds
  • b. SUCCESS if all succeed

This is difficult... Any choice seems confusing, and I want a status like "Some Success / Some Failure" 😅

If at least one of the rollback processes succeeds, we consider the rollback successful. This ensures that the rollback is executed for other environments even if one of the deployment environments is inaccessible.

https://github.com/pipe-cd/pipecd/blob/master/docs/rfcs/0014-multi-cluster-deployment-for-k8s.md#qicksync

Copy link
Member Author

@ffjlabo ffjlabo Apr 28, 2025

Choose a reason for hiding this comment

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

We should choose a. SUCCESS if at least one succeeds to ensure rollback when one of the cluster has some trouble but want to rollback another cluster.

This is difficult... Any choice seems confusing, and I want a status like "Some Success / Some Failure" 😅

I know your point, but this spec is only for this plugin. 😇
I plan to add a log to show the each rollback status and the whole status Instead of adding status.

t-kikuc
t-kikuc previously approved these changes Apr 28, 2025
@ffjlabo ffjlabo force-pushed the implement-k8s-multi-cluster-rollback branch from 49611f8 to fa2ebd6 Compare May 7, 2025 08:35
Signed-off-by: Yoshiki Fujikane <[email protected]>
@ffjlabo ffjlabo force-pushed the implement-k8s-multi-cluster-rollback branch from 1d20bfb to 9a2c241 Compare May 7, 2025 09:09
@ffjlabo ffjlabo enabled auto-merge (squash) May 7, 2025 09:28
Copy link
Member

@Warashi Warashi left a comment

Choose a reason for hiding this comment

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

👍🏻

Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

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

👍

@ffjlabo ffjlabo merged commit 5d60b70 into master May 8, 2025
19 checks passed
@ffjlabo ffjlabo deleted the implement-k8s-multi-cluster-rollback branch May 8, 2025 07:21
@github-actions github-actions bot mentioned this pull request May 15, 2025
@github-actions github-actions bot mentioned this pull request May 28, 2025
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.

4 participants