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.

Sign up for GitHub

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

Jump to bottom

Allow relaxing deleted pods checking in RC runner #82029

Merged
merged 1 commit into from
Sep 23, 2019
Merged

Allow relaxing deleted pods checking in RC runner #82029

merged 1 commit into from
Sep 23, 2019

Conversation

losipiuk
Copy link
Contributor

@losipiuk losipiuk commented Aug 27, 2019 •
edited
Loading

There is strong probabilty that some pods will be deleted when
we are bombarding cluster with high volume of pods. We do that in
Cluster Autoscaler scalability tests and we want to relax check there.

NONE

/assign @aleksandra-malinowska

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 27, 2019
@losipiuk
Copy link
Contributor Author

/kind failing-test
/sig autoscaling

@k8s-ci-robot k8s-ci-robot added sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 27, 2019
@losipiuk
Copy link
Contributor Author

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 27, 2019
@losipiuk
Copy link
Contributor Author

/kind failing-test

@k8s-ci-robot k8s-ci-robot added kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Aug 27, 2019
@losipiuk
Copy link
Contributor Author

/assign @deads2k

@aleksandra-malinowska
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2019
@aleksandra-malinowska
Copy link
Contributor

/retest

1 similar comment
@losipiuk
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2019
@losipiuk
Copy link
Contributor Author

/retest

@losipiuk
Copy link
Contributor Author

/test pull-kubernetes-conformance-kind-ipv6

@MaciekPytel
Copy link
Contributor

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2019
@losipiuk
Copy link
Contributor Author

/assign @liggitt
Can we merge this one as code freeze is over?

@losipiuk
Copy link
Contributor Author

@liggitt ping on this one :)

liggitt
liggitt reviewed Sep 19, 2019
View reviewed changes
test/utils/runners.go Outdated
@@ -132,6 +132,9 @@ type RCConfig struct {
PriorityClassName string
TerminationGracePeriodSeconds *int64

// If set to true the framework will not check if some pod was deleted from deployment
AllowPodDeletions bool
Copy link
Member

Choose a reason for hiding this comment

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

should this be grouped with MaxContainerFailures and follow a similar pattern (target number or percent instead of a boolean)?

@liggitt
Copy link
Member

liggitt commented Sep 19, 2019

one (non-blocking) question. I don't have a lot of context on this, and there's no example usage in-repo in this PR, so will defer to sig-autoscaling reviewers.

@losipiuk losipiuk force-pushed the lo/runners-scalability branch 2 times, most recently from 7d1399c to 0c8813f Compare September 19, 2019 18:21
@losipiuk losipiuk changed the title Allow disabling deleted pods checking in RC runner Allow relaxing deleted pods checking in RC runner Sep 19, 2019
@losipiuk
Copy link
Contributor Author

one (non-blocking) question. I don't have a lot of context on this, and there's no example usage in-repo in this PR, so will defer to sig-autoscaling reviewers.

I updated the PR following your suggestion.
As for the usage example we have one which lives is internal repository. We are testing scalability of CA on GKE. No OSS usages for now.

@losipiuk losipiuk force-pushed the lo/runners-scalability branch 2 times, most recently from 493a3bf to c2e4fee Compare September 19, 2019 18:26
@losipiuk
Copy link
Contributor Author

/retest

@losipiuk
Allow relaxing deleted pods checking in RC runner
95d1f4d 
There is strong probabilty that some pods will be deleted when
we are bombarding cluster with high volume of pods. We do that in
Cluster Autoscaler scalability tests and we want to relax check there.

Change-Id: Ib7883666c0c952f61914ab51dcf1f5244e1e7e42
@losipiuk
Copy link
Contributor Author

@aleksandra-malinowska can you please take another look after changes?

@aleksandra-malinowska
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2019
@losipiuk
Copy link
Contributor Author

@liggitt I have lgtm from sig-autoscaling (Aleksandra). I still need an approval from one of the owners of modified file.
If you do not feel comfortable with approving that can you suggest a reviewer. I tried at random a bit but it is hard to get attention.

@liggitt
Copy link
Member

liggitt commented Sep 23, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, losipiuk, MaciekPytel

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2019
@losipiuk
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit d62ebe3 into kubernetes:master Sep 23, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.17 milestone Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@draveness draveness Awaiting requested review from draveness

@johnSchnake johnSchnake Awaiting requested review from johnSchnake

Assignees

@soltysh soltysh

@liggitt liggitt

@shyamjvs shyamjvs

@aleksandra-malinowska aleksandra-malinowska

@MaciekPytel MaciekPytel

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.17
Development

Successfully merging this pull request may close these issues.
8 participants
@losipiuk @aleksandra-malinowska @MaciekPytel @liggitt @k8s-ci-robot @soltysh @shyamjvs @deads2k