-
Notifications
You must be signed in to change notification settings - Fork 805
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
selector: percent selection should select at least one #3036
selector: percent selection should select at least one #3036
Conversation
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
/run-e2e-tests |
Codecov Report
@@ Coverage Diff @@
## master #3036 +/- ##
==========================================
- Coverage 37.98% 37.94% -0.05%
==========================================
Files 106 106
Lines 9359 9359
==========================================
- Hits 3555 3551 -4
- Misses 5492 5495 +3
- Partials 312 313 +1
Continue to review full report at Codecov.
|
/run-e2e-tests |
1 similar comment
/run-e2e-tests |
percentage := getRandomNumber(maxPercentage + 1) // + 1 because Intn works with half open interval [0,n) and we want [0,n] | ||
num := int(math.Floor(float64(count) * float64(percentage) / 100)) | ||
num := int(math.Ceil(float64(count) * float64(percentage) / 100)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove the maxPercentage+1
and update the comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Consider the special situation where the maxPercentage
is 100
, then the percentage
will be 0-100
(as the getRandomNumber
always returns the number in [0,101)
), which is actually expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iguoyr PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @YangKeao You need to update docs for this.
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 524ab2a
|
What problem does this PR solve?
This bug was originally found in
NetworkChaos
:If the user is using the
maxPercentMode
for both.
and.Target
, and blessed by the goddess of fortune, one of them doesn't select any pod. The other one will be injected for all traffic, which is not expected. After considering some time, I thought it would be reasonable to generate at least one target in the selector (if it's at least able to.)In this PR, I modified two things:
Floor
in selector intoCeil