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.

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

Tests should use a randomly generated namespace #4629

Closed
mboersma opened this issue May 17, 2021 · 15 comments
Closed

Tests should use a randomly generated namespace #4629

mboersma opened this issue May 17, 2021 · 15 comments
Labels
area/testing Issues or PRs related to testing help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug.
Milestone

Comments

@mboersma
Copy link
Contributor

What steps did you take and what happened:

In refactoring the unit/integration tests for #4588, we noticed that they use the "default" namespace, and not always through the v1.NamespaceDefault constant.

What did you expect to happen:

We should review and unify the use of namespaces in the tests so that each uses a randomly-generated namespace to avoid collision, and so we are not polluting the default namespace. See comments here.

Anything else you would like to add:

Environment:

/kind bug
/area testing

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. area/testing Issues or PRs related to testing labels May 17, 2021
@fabriziopandini
Copy link
Member

+100 to this.
It is also compliant with what we are recommending in https://cluster-api.sigs.k8s.io/developer/testing.html#quick-reference

@fabriziopandini fabriziopandini added this to the v0.4 milestone May 18, 2021
@sbueringer
Copy link
Member

sbueringer commented May 18, 2021

Also the same way as the Kubernetes e2e tests / conformance tests and it's working great there, so it should also be a good fit for us.

@CecileRobertMichon
Copy link
Contributor

/help

@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon:
This request has been marked as needing help from a contributor.

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-help command.

In response to this:

/help

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 help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label May 18, 2021
@hzliangbin
Copy link

/assign

@hzliangbin
Copy link

@mboersma hi, are you working on this already?

@hzliangbin hzliangbin removed their assignment May 19, 2021
@mboersma
Copy link
Contributor Author

@hzliangbin no, please have at it, and thanks!

I looked at it briefly and my only thoughts are:

  • it will be a lot of work
  • maybe we can use testEnv.CreateNamespace(ctx, t.Name())

@sbueringer
Copy link
Member

@hzliangbin I would suggest to implement it first on a relatively small package to establish the correct way to do it and then post a list like here #4588 (comment) in this issue to give an overview and possibly crowdsource it (your choice).

@sbueringer
Copy link
Member

@fabriziopandini Based on the results in #5029 (comment) the PRs helped a lot to make the tests repeatable on a consistently running kind cluster. WDYT about opening a follow-up issue to do the same for other global resources (according to my tests in the linked comment we have the same issue in a few tests which use hard-coded node names)

@fabriziopandini
Copy link
Member

@sbueringer global resources (like nodes, CRDs, etc) or things generated by other components (like Kubeadm config maps or CoreDNS config map) are not good candidates for this effort, but I'm +1 to apply similar changes whenever possible or at least let's make sure we do a proper cleanup after each test
@killianmuldoon ^^

@sbueringer
Copy link
Member

sbueringer commented Jul 30, 2021

@sbueringer global resources (like nodes, CRDs, etc) or things generated by other components (like Kubeadm config maps or CoreDNS config map) are not good candidates for this effort, but I'm +1 to apply similar changes whenever possible or at least let's make sure we do a proper cleanup after each test
@killianmuldoon ^^

Yup, I would highly recommend a well-scoped follow-up issue. I think we can figure out where we have the most issue when running the tests repeatedly against a kind cluster. So far the main problems seem to be:

  • hard-coded node names + we never cleanup the nodes
  • overall a few places where we don't cleanup nodes (and maybe other cluster-wide resources, but not sure)

@sbueringer
Copy link
Member

I'll take a closer look later today and probably close this issue and open a follow-up issue.

@sbueringer
Copy link
Member

sbueringer commented Jul 30, 2021

I went through the repo and grepped for "default" and "NamespaceDefault". It looks like we now only use the default namespace in combination with fake client (as there are ~500 occurrences I didn't look in very close detail). I would close this issue as at least the vast majority is fixed. If someone has a concrete example feel free to reopen / open a new issue.

As a follow-up I will open an issue (roughly in the next 1-2 weeks) which will focus on "can we run all our tests repeatedly using the same kind cluster". This should catch all cases where cluster-wide resources with a hard-coded name are used (namespaces, nodes, ...). I'll also scope it in a way that we make sure we clean up cluster-wide resources, e.g. currently we leak a lot of nodes with random-generated name.

@sbueringer
Copy link
Member

/close

@k8s-ci-robot
Copy link
Contributor

@sbueringer: Closing this issue.

In response to this:

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Issues or PRs related to testing help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

6 participants