Only expose top N images in NodeStatus
#25328
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-github-robot
added
size/M
| // sort the images from max to min, and only set top N images into the node status. | ||
| sort.Sort(ByImageSize(containerImages)) | ||
| if maxImagesInNodeStatus < len(containerImages) { | ||
| containerImages = containerImages[0 : maxImagesInNodeStatus-1] |
There was a problem hiding this comment.
Actually, this returns maxImagesInNodeStatus-1 images, is that what you want? @resouer
vishh
changed the title
NodeStatus
vishh
added
release-note
k8s-github-robot
added
needs-rebase
resouer
force-pushed
the
sort-images
branch
2 times, most recently
from
9048885 to
d342b22
Compare
k8s-github-robot
added
size/XS
k8s-github-robot
added
size/L
| @@ -40,6 +40,22 @@ func Intn(max int) int { | |||
| return rng.rand.Intn(max) | |||
| } | |||
|
|
|||
| // IntnRange generates an integer in range min->max. | |||
| // By design this should panic if input is invalid, <= 0. | |||
| func IntnRange(min int, max int) int { | |||
|
LGTM. Ok to be merged once the two nits are addressed. Thanks for the PR @resouer |
k8s-github-robot
added
the
needs-rebase
k8s-github-robot
removed
the
needs-rebase
|
Fixed rebase :) |
|
LGTM |
yifan-gu
added
the
lgtm
| // 0 is invalid. | ||
| for min, max := range map[int]int{1: 2, 10: 123, 100: 500} { | ||
| inrange := IntnRange(min, max) | ||
| if inrange < min || inrange > max { |
There was a problem hiding this comment.
Apologies for the drive-by review, but since I'm build cop and looking for safe things to merge...
Generating a single number per test here is probably not sufficient, could take a long time to expose a problem. Suggest doing this 100+ times per test.
Second, these functions both have a bug/undocumented feature, which is that they'll never return the top number in the range. Either document or fix.
k8s-github-robot
removed
the
lgtm
|
updated the rand tests cc @lavalamp |
lavalamp
added
the
lgtm
|
@resouer thanks for humoring me :) LGTM |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit d917ed2. |
|
Automatic merge from submit-queue |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #25209
Sorted the image and only pick set top 50 sized images in node status.
cc @vishh