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

disallow to configure traffic or match when a step has replicas more than threshold, for partition-style release only #225

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

myname4423
Copy link
Contributor

@myname4423 myname4423 commented Jul 19, 2024

Ⅰ. Describe what this PR does

add check to webhook of creating and updating Rollout, if a partition-style release step has replicas configured more than threshold (defaults to 30%), this step mustn't be configured with traffic strategy (ie. traffic and matches must leave empty).
Note:

  1. user can add an annotation rollouts.kruise.io/partition-replicas-limit to Rollout to configure the threshold, for example, user may set rollouts.kruise.io/partition-replicas-limit: 100% to virtually "disable" this check.
  2. this restriction only works for partition-style release, both for v1alpha1 and v1beta1
  3. this check is only for replicas in percentage format, since we cannot obtain the replicas from workload when doing this check (the workload might not have been submitted yet)
  4. if users use weight field without specifying replicas (a common practice for v1alpha1, which is converted to replicas and traffic fields of v1beta1 with the same value automatically), they will encounter failure if weight is greater than the threshold.

edit: modified the e2e tests to obey this restriction

Ⅱ. Does this pull request fix one issue?

Ⅲ. Special notes for reviews

Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 46.87500% with 68 lines in your changes missing coverage. Please review.

Project coverage is 43.82%. Comparing base (07c1731) to head (9b819b3).
Report is 6 commits behind head on master.

Files Patch % Lines
pkg/controller/rollout/rollout_canary.go 5.26% 34 Missing and 2 partials ⚠️
pkg/trafficrouting/manager.go 44.82% 10 Missing and 6 partials ⚠️
...ollout/validating/rollout_create_update_handler.go 76.92% 4 Missing and 2 partials ⚠️
pkg/util/rollout_utils.go 0.00% 5 Missing ⚠️
...ok/rollout/validating/validate_v1alphal_rollout.go 83.33% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
+ Coverage   43.63%   43.82%   +0.19%     
==========================================
  Files          52       53       +1     
  Lines        5681     6037     +356     
==========================================
+ Hits         2479     2646     +167     
- Misses       2778     2925     +147     
- Partials      424      466      +42     
Flag Coverage Δ
unittests 43.82% <46.87%> (+0.19%) ⬆️

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.

// In partition style, when the number of replicas for the new version is relatively small (i.e., less than PartitionReplicasLimitWithTraffic),
// users can configure traffic strategies (traffic/matches). At this stage, it works similarly to canary style.
// However, once the replicas for the new version exceed a certain proportion, we want the pods from both the new and old versions to receive traffic evenly.
// Instead of having users manually set the proportion, we aim to achieve this through the Service and/or the upper-level load balancing mechanism.
Copy link
Member

Choose a reason for hiding this comment

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

Instead of having users manually set the proportion, we aim to achieve this through the Service and/or the upper-level load balancing mechanism.

this sentence is confusing

Signed-off-by: yunbo <yunbo10124scut@gmail.com>
@furykerry
Copy link
Member

/lgtm

@zmberg
Copy link
Member

zmberg commented Aug 2, 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 78273c2 into openkruise:master Aug 2, 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