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

improve finalising logic for canary release #229

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

myname4423
Copy link
Contributor

Ⅰ. Describe what this PR does

rewrite the doCanaryFinalising function, mainly to modify the logic of rollback
before, this function executes the same steps for rollback and other scenarios (including success, pause, disabling):

  1. restore the stable service (remove selector)
  2. finalizingBatchRelease
  3. modify network configuration (ingress/istio/gateway api configuration), and route 100% traffic to stable pods
  4. delete BatchRelease

however, in rollback scenario, running the first step (remove selector) will route some traffic unexpectedly to new version if user does A/B testing. Therefore, in rollback scenario, we instead route all traffic to stable service firstly, like:

  1. route all traffic to stable service (by restoring ingress/istio/gateway api directly)
  2. finalizingBatchRelease
  3. delete BatchRelease
  4. restore stable service, remove canary service

note: i am not intended to modify the behaviours other than rollback (ie. success, pause, disabling), however, i find it is possible to simplify it, by combing the first step and the third step:

  1. call traffic finalising function to (restore stable service-> restore ingress/istio/gateway api -> remove canary service) in one go
  2. finalizingBatchRelease
  3. delete BatchRelease

Ⅱ. Does this pull request fix one issue?

no

Ⅲ. Special notes for reviews

Copy link

codecov bot commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 52.85714% with 33 lines in your changes missing coverage. Please review.

Project coverage is 44.76%. Comparing base (07c1731) to head (10f531d).
Report is 10 commits behind head on master.

Files Patch % Lines
pkg/controller/rollout/rollout_canary.go 54.54% 23 Missing and 7 partials ⚠️
pkg/controller/rollout/rollout_status.go 0.00% 2 Missing ⚠️
pkg/controller/rollout/rollout_progressing.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   43.63%   44.76%   +1.12%     
==========================================
  Files          52       55       +3     
  Lines        5681     6103     +422     
==========================================
+ Hits         2479     2732     +253     
- Misses       2778     2910     +132     
- Partials      424      461      +37     
Flag Coverage Δ
unittests 44.76% <52.85%> (+1.12%) ⬆️

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.

@myname4423 myname4423 force-pushed the improve branch 2 times, most recently from c2ef833 to 733deb4 Compare August 20, 2024 08:42
pkg/controller/rollout/rollout_canary.go Outdated Show resolved Hide resolved
pkg/controller/rollout/rollout_canary.go Outdated Show resolved Hide resolved
@myname4423 myname4423 force-pushed the improve branch 3 times, most recently from 26e1aec to 2695255 Compare September 2, 2024 03:37
pkg/controller/rollout/rollout_canary.go Outdated Show resolved Hide resolved
pkg/controller/rollout/rollout_canary.go Show resolved Hide resolved
api/v1beta1/rollout_types.go Show resolved Hide resolved
Signed-off-by: yunbo <[email protected]>

improve finalising logic for canary release-2

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

/lgtm

@zmberg
Copy link
Member

zmberg commented Sep 4, 2024

/lgtm
/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 d261313 into openkruise:master Sep 4, 2024
21 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