Skip to content

Cleanup the "extended load test" overrides #1036

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

Closed
mm4tt opened this issue Feb 7, 2020 · 14 comments
Closed

Cleanup the "extended load test" overrides #1036

mm4tt opened this issue Feb 7, 2020 · 14 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.

Comments

@mm4tt
Copy link
Contributor

mm4tt commented Feb 7, 2020

The extended load tests has been implemented and enabled everywhere as a part of #704.
We've been running these tests in many places (also at 5k node scales) for a few months. I believe we have enough confidence to make all the changes the defaults and cleanup all the gating logic we have.

We should get rid of all these overrides: https://github.com/kubernetes/perf-tests/tree/master/clusterloader2/testing/load/experimental/overrides
And clean-up all places where they are used (e.g. 10+ occurrences in k8s.io/test-infra)

@mm4tt
Copy link
Contributor Author

mm4tt commented Feb 7, 2020

/good-first-issue

@k8s-ci-robot
Copy link
Contributor

@mm4tt:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

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 good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Feb 7, 2020
@wojtek-t
Copy link
Member

To clarify - the things to do are:

  • update default for things that are enabled everywhere (PVs are still disabled in couple tests)
  • then remove the overwrides
  • cleanup ability to override those things

@maniSbindra
Copy link
Contributor

Hi @wojtek-t I would like to work on this issue.

I found 126 occurrences of "testing/load/experimental/override" in 7 files within k8s.io/test-infra. All these occurrences use --test-cmd-args=--testoverrides=./testing/load/experimental/overrides/ with one of the files listed in https://github.com/kubernetes/perf-tests/tree/master/clusterloader2/testing/load/experimental/overrides .

I wanted a bit more clarification regarding changes and tests I can use to verify the changes. So as apart of PR to resolve this issue should I

  • Remove the --test-cmd-args=--testoverrides=./testing/load/experimental/overrides/ switches from k8s.io/test-infra ?
  • Remove the override files ?
  • Modify the ./clusterloader2/pkg/config/template_provider.go to cleanup ability to override
    ?
  • Would it be possible to share more details related to "update default for things that are enabled everywhere"

Thanks,
Mani

@mm4tt
Copy link
Contributor Author

mm4tt commented Mar 16, 2020

Hey @maniSbindra, thanks for taking a look at this!

To give you an example of what "update defaults" means, let's take a look at the
https://github.com/kubernetes/perf-tests/blob/master/clusterloader2/testing/load/experimental/overrides/enable_configmaps.yaml
It overrides the ENABLE_CONFIGMAPS variable to true.
We'd like to change the default of ENABLE_CONFIGMAPS to true so the override is not needed anymore. To do that, you should find all places where this variable is defined and change the default to true, e.g. this one

{{$ENABLE_CONFIGMAPS := DefaultParam .ENABLE_CONFIGMAPS false}}

Once you do that, the override will be no-op and it will be safe to delete it (first delete usage in test-infra, then delete the override file in perf-test).

@maniSbindra
Copy link
Contributor

maniSbindra commented Mar 16, 2020

Thank you @mm4tt. I will work on this.

@maniSbindra
Copy link
Contributor

Hi @mm4tt I believe this issue can be closed now.

Thanks,
Mani

@wojtek-t
Copy link
Member

wojtek-t commented Apr 2, 2020

There is one more thing - for stuff that we don't disable anywhere (e.g. secrets), we should also cleanup the config file:
https://github.com/kubernetes/perf-tests/blob/master/clusterloader2/testing/load/config.yaml

[The simpler it will be, the better]

@maniSbindra
Copy link
Contributor

Hi just wanted to confirm my understanding. Currently only places (which I could find) in test-infra where we use not use the default values of these override variables are where we set CL2_ENABLE_PVS=false in sig-scalability-presubmit-jobs.yaml, and where we set CL2_ENABLE_NETWORKPOLICIES=true in sig-scalability-experimental-periodic-jobs.yaml. Does this mean that for the remaining 5 override variables (ENABLE_CONFIGMAPS, ENABLE_DAEMONSETS, ENABLE_JOBS, ENABLE_SECRETS, ENABLE_STATEFULSETS) we cleanup the config file mentioned?

@wojtek-t
Copy link
Member

wojtek-t commented Apr 2, 2020

Yes - that's exactly what I'm suggesting.

@maniSbindra
Copy link
Contributor

Hi @wojtek-t for this issue to be closed, do I need to submit a PR deleting the file experimental-config.yaml?

@wojtek-t
Copy link
Member

Yes - it's out-of-date anyway. So let's please remove it and close this issue then.

@maniSbindra
Copy link
Contributor

Hi @wojtek-t ,

Can this issue be closed now that #1183 has been merged.

Thanks,
Mani

@wojtek-t
Copy link
Member

wojtek-t commented May 3, 2020

Yes - we can close it. Thanks!

@wojtek-t wojtek-t closed this as completed May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants