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

✨ Cleanup Webhook server setup #1504

Merged

Conversation

aayushrangwala
Copy link
Contributor

@aayushrangwala aayushrangwala commented May 2, 2021

  • Called the webhook server cleanup function

  • Only ignore controlplane cleanup when using existing cluster

  • Added test to check the cleanup for Webhook after Stop()

Fixes #1496

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

Hi @aayushrangwala. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 2, 2021
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

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

Should there be some test that the directory has been properly cleaned up?

@invidian
Copy link
Member

invidian commented May 2, 2021

/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 May 2, 2021
@aayushrangwala
Copy link
Contributor Author

Should there be some test that the directory has been properly cleaned up?

Yes, definitely this needs to be part of unit tests and I was actually about to add too. But then I found there needs to be a comprehensive set of tests for checking the complete Stop() functionality of evntest as well as independent component such as controlplan.Stop() and webhook.Stop().

This seems to be the part of another PR to improve tests

@k8s-ci-robot
Copy link
Contributor

@aayushrangwala: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

  • /test pull-controller-runtime-test-master
  • /test pull-controller-runtime-apidiff-master

Use /test all to run all jobs.

In response to this:

/retest pull-controller-runtime-test-master

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.

@aayushrangwala
Copy link
Contributor Author

/test pull-controller-runtime-test-master

1 similar comment
@aayushrangwala
Copy link
Contributor Author

/test pull-controller-runtime-test-master

@invidian
Copy link
Member

invidian commented May 3, 2021

@aayushrangwala what about something like the following:

	Describe("Stop", func() {
                Context("With existing cluster", func() {
	  	        It("should remove local serving certificate directory", func(done Done) {
			        env = &Environment{UseExistingCluster: pointer.Bool(true)}
			        _, err = env.Start()
			        Expect(err).NotTo(HaveOccurred())
			        Expect(env.Stop()).To(Succeed())
                                Expect(env.WebhookInstallOptions.LocalServingCertDir).ShouldNot(BeADirectory())
			        close(done)
		        }, 30)
                })
	})

I'm not sure if this is going to work, as it seems UseExistingCluster has no tests at all... Maybe it can be added with something like:

if useExisting := os.Getenv("USE_EXISTING_CLUSTER"); useExisting == "" {
        Skip("USE_EXISTING_CLUSTER environment variable required")
}

So at least one can run it locally until there is a CI facility to execute this test automatically.

@@ -65,15 +65,15 @@ var _ = Describe("Test", func() {
Name: crd.GetName(),
}
var placeholder v1beta1.CustomResourceDefinition
err := c.Get(context.TODO(), crdObjectKey, &placeholder)
err = c.Get(context.TODO(), crdObjectKey, &placeholder)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
err = c.Get(context.TODO(), crdObjectKey, &placeholder)
err := c.Get(context.TODO(), crdObjectKey, &placeholder)

Please do not do scope changes for variables like this (This comment doesn't only apply to this line but to all places where you did that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Will change it back. Did that as the part of go-lint with no new variable

Copy link
Member

Choose a reason for hiding this comment

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

not sure what no new variable is but IMHO it is best to keep the scope for variables as small as possible and this kind of change is the very opposite to that. Reduces the odds of accidentally propagating values to the wrong place and makes it easier for the garbage collector

@aayushrangwala aayushrangwala changed the title 🐛 Cleanup Webhook server setup ✨ Cleanup Webhook server setup May 5, 2021
@aayushrangwala aayushrangwala force-pushed the cleanup-webhook-setup branch 2 times, most recently from 885bb2b to 6737748 Compare May 5, 2021 14:09
@aayushrangwala
Copy link
Contributor Author

@aayushrangwala what about something like the following:

	Describe("Stop", func() {
                Context("With existing cluster", func() {
	  	        It("should remove local serving certificate directory", func(done Done) {
			        env = &Environment{UseExistingCluster: pointer.Bool(true)}
			        _, err = env.Start()
			        Expect(err).NotTo(HaveOccurred())
			        Expect(env.Stop()).To(Succeed())
                                Expect(env.WebhookInstallOptions.LocalServingCertDir).ShouldNot(BeADirectory())
			        close(done)
		        }, 30)
                })
	})

I'm not sure if this is going to work, as it seems UseExistingCluster has no tests at all... Maybe it can be added with something like:

if useExisting := os.Getenv("USE_EXISTING_CLUSTER"); useExisting == "" {
        Skip("USE_EXISTING_CLUSTER environment variable required")
}

So at least one can run it locally until there is a CI facility to execute this test automatically.

Yeah, will add a case to check the cleanup but I think we dont need to worry about the existing cluster scenario, because as per the code even for the new cluster the same setup happens for Webhook install. So its better to keep it generic

@aayushrangwala
Copy link
Contributor Author

/test pull-controller-runtime-test-master

3 similar comments
@aayushrangwala
Copy link
Contributor Author

/test pull-controller-runtime-test-master

@aayushrangwala
Copy link
Contributor Author

/test pull-controller-runtime-test-master

@aayushrangwala
Copy link
Contributor Author

/test pull-controller-runtime-test-master

- Called the webhook server cleanup function

- Only ignore controlplane cleanup when using existing cluster

- Added test to check the directory exists post stop
@aayushrangwala
Copy link
Contributor Author

@alvaroaleman Please re-review

Copy link

@cndoit18 cndoit18 left a comment

Choose a reason for hiding this comment

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

LGTM

@k8s-ci-robot
Copy link
Contributor

@cndoit18: changing LGTM is restricted to collaborators

In response to this:

LGTM

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.

Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

thanks!

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aayushrangwala, alvaroaleman, cndoit18

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 May 24, 2021
@k8s-ci-robot k8s-ci-robot merged commit 7181f11 into kubernetes-sigs:master May 24, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.9.x milestone May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

envtest.Environment.Stop() leaves orphan /tmp/envtest-serving-certs-* directories
5 participants