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

Set the extended load test override defaults to true #1125

Merged
merged 1 commit into from
Mar 31, 2020
Merged

Set the extended load test override defaults to true #1125

merged 1 commit into from
Mar 31, 2020

Conversation

maniSbindra
Copy link
Contributor

This PR is to resolve issue #1036 . As described in #1036 to resolve the issue changes also need to be made in k8s.io/test-infra, for which PR kubernetes/test-infra#16818 has been created

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 17, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @maniSbindra!

It looks like this is your first PR to kubernetes/perf-tests 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/perf-tests has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 17, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @maniSbindra. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 17, 2020
@k8s-ci-robot k8s-ci-robot requested review from oxddr and wojtek-t March 17, 2020 12:54
@maniSbindra maniSbindra mentioned this pull request Mar 17, 2020
Closed
@maniSbindra
Copy link
Contributor Author

/assign @mm4tt

mm4tt
mm4tt reviewed Mar 17, 2020
View reviewed changes
Copy link
Contributor

@mm4tt mm4tt left a comment

Choose a reason for hiding this comment

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

Thanks! Could you change the PR title to be more direct in terms of what the change does, e.g. something like: "Set the extended load test override defaults to true"?

clusterloader2/testing/load/config.yaml Outdated
{{$ENABLE_CONFIGMAPS := DefaultParam .ENABLE_CONFIGMAPS false}}
{{$ENABLE_DAEMONSETS := DefaultParam .ENABLE_DAEMONSETS false}}
{{$ENABLE_JOBS := DefaultParam .ENABLE_JOBS false}}
{{$ENABLE_PVS := DefaultParam .ENABLE_PVS false}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we disable PVs in kubemark tests, see #803
Because of that, we need to introduce a way to disable it in kubemark tests once we switch the default to true.
Sorry, this should've been also mentioned in the issue.

I think the easiest way to tackle this would be:

  1. Sent a PR that will rename these variables to have CL2_ prefix, e.g. replace ENABLE_PVS to CL2_ENABLE_PVS. If a variable begins with CL2 it's possible to change it value by overriding env. Changing the name should be a no-op change because the name is not referenced anywhere outside this repo
  2. Sent a PR to test-infra and change this file - https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-scalability/sig-scalability-periodic-jobs.yaml by adding the following arg to all kubemark jobs (name contains kubemark) 
    - --env=CL2_ENABLE_PVS=false # TODO(https://github.com/kubernetes/perf-tests/issues/803): Fix me
  3. Once you do it you should be good to go to change the default to false

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, point 1 is done.
Now we need to do point 2 and we'll be good to submit this PR

@maniSbindra maniSbindra changed the title PR resolve issue 1036 Set the extended load test override defaults to true Mar 17, 2020
@mm4tt
Copy link
Contributor

mm4tt commented Mar 18, 2020

Thanks @maniSbindra!
This is really great, waiting for the PR that deals with ENABLE_PVS before approving this one.

@maniSbindra
Copy link
Contributor Author

Thanks @mm4tt will work on that change a bit later today

@maniSbindra maniSbindra mentioned this pull request Mar 18, 2020
Merged
@maniSbindra
Copy link
Contributor Author

@mm4tt I have created the PR #1126

@maniSbindra maniSbindra mentioned this pull request Mar 19, 2020
Merged
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2020
@maniSbindra maniSbindra force-pushed the k8s.io/perf-tests/issue-1036 branch from 90a29ed to 2483c19 Compare March 26, 2020 15:46
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2020
@mm4tt
Copy link
Contributor

mm4tt commented Mar 26, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 26, 2020
@maniSbindra
Copy link
Contributor Author

/test pull-perf-tests-clusterloader2

@maniSbindra
Copy link
Contributor Author

/test pull-perf-tests-clusterloader2-kubemark

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 27, 2020
@maniSbindra
Copy link
Contributor Author

/test pull-perf-tests-clusterloader2

@maniSbindra maniSbindra mentioned this pull request Mar 27, 2020
Merged
@maniSbindra maniSbindra force-pushed the k8s.io/perf-tests/issue-1036 branch from df15daa to 2483c19 Compare March 27, 2020 12:58
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 27, 2020
@mm4tt
Copy link
Contributor

mm4tt commented Mar 27, 2020

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maniSbindra, mm4tt

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 Mar 27, 2020
@mm4tt
Copy link
Contributor

mm4tt commented Mar 27, 2020

/test pull-perf-tests-clusterloader2-kubemark

1 similar comment
@maniSbindra
Copy link
Contributor Author

/test pull-perf-tests-clusterloader2-kubemark

@mm4tt
Copy link
Contributor

mm4tt commented Mar 30, 2020

Sorry, @maniSbindra

I got swamped with other work. Let me see why kubemark test is failing

@mm4tt
Copy link
Contributor

mm4tt commented Mar 30, 2020

The #1145 should fix the failing kubemark tests once it gets merged.

@maniSbindra
Copy link
Contributor Author

/test pull-perf-tests-clusterloader2-kubemark

@k8s-ci-robot k8s-ci-robot merged commit 5badd97 into kubernetes:master Mar 31, 2020
This was referenced Mar 31, 2020
Merged
Merged
@maniSbindra maniSbindra mentioned this pull request Apr 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mm4tt mm4tt mm4tt left review comments

@oxddr oxddr Awaiting requested review from oxddr

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

Assignees

@mm4tt mm4tt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maniSbindra @k8s-ci-robot @mm4tt