Add e2e test for cronjob chained removal #48068

soltysh · 2017-06-26T14:34:14Z

This is test proving #44058 works with cronjobs. This will fail until the aforementioned PR merges.

0xmichalis · 2017-06-26T14:40:04Z

test/e2e/workload/cronjob.go

@@ -75,6 +75,13 @@ var _ = framework.KubeDescribe("CronJob", func() {
 		By("Removing cronjob")
 		err = deleteCronJob(f.ClientSet, f.Namespace.Name, cronJob.Name)


deleteCronJob is not setting any DeleteOptions and the default REST strategy for CJs is orphaning so I am wondering how does this test work?

Yeah, you need to set the policy to DeletePropagationBackground.

0xmichalis · 2017-06-26T14:41:36Z

Unless you are doing a foreground deletion, this test is not going to be stable. Also can you verify that the CronJob is deleted?

caesarxuchao · 2017-06-26T16:58:12Z

test/e2e/workload/cronjob.go

@@ -75,6 +75,13 @@ var _ = framework.KubeDescribe("CronJob", func() {
 		By("Removing cronjob")
 		err = deleteCronJob(f.ClientSet, f.Namespace.Name, cronJob.Name)
 		Expect(err).NotTo(HaveOccurred())
+		By("Ensuring cronjob does not leave jobs nor pods behind")
+		jobs, err = f.ClientSet.Batch().Jobs(f.Namespace.Name).List(metav1.ListOptions{})


This should be a poll and list. The GC takes time.

caesarxuchao · 2017-06-26T16:58:30Z

test/e2e/workload/cronjob.go

+		jobs, err = f.ClientSet.Batch().Jobs(f.Namespace.Name).List(metav1.ListOptions{})
+		Expect(err).NotTo(HaveOccurred())
+		Expect(jobs.Items).To(HaveLen(0))
+		pods, err := f.ClientSet.Core().Pods(f.Namespace.Name).List(metav1.ListOptions{})


Same comment here.

caesarxuchao · 2017-06-26T21:51:57Z

@Kargakis @soltysh we need to reach consensus on whether to use foreground or background GC.

I think in this test, the stable way is using the foreground gc, then polls for the final deletion of the cronjob, and then verify the pods are deleted.

I think @soltysh's intention was to test the GC properly cascades the background GC. For that, i think it is worth a separate test.

0xmichalis · 2017-06-27T08:29:57Z

@caesarxuchao should we need to poll in case of foreground deletion? IIUC, the DELETE call should block until everything is deleted, isn't that the case?

soltysh · 2017-06-27T12:20:40Z

@Kargakis based on this I'm assuming the blocking depends what we set in ownerRef. cronjob has that set so I'll go with foreground deletion as proposed by @caesarxuchao

soltysh · 2017-06-27T14:06:58Z

@Kargakis actually, the foreground removal works without the fix, so we need background removal to verify its role.

soltysh · 2017-06-27T14:19:31Z

@caesarxuchao @Kargakis I've created a separate tests just for cronjobs GC, ptal

soltysh · 2017-06-28T11:24:54Z

@caesarxuchao any objections to merge this in?

caesarxuchao · 2017-06-28T17:43:47Z

test/e2e/garbage_collector.go

+		}
+
+		By("Delete the cronjob")
+		if err := f.ClientSet.BatchV2alpha1().CronJobs(f.Namespace.Name).Delete(cronJob.Name, getNonOrphanOptions()); err != nil {


This could be fixed in another PR. OrphanDependents field is deprecated, set propagationPolicy to Background instead.

caesarxuchao · 2017-06-28T17:46:42Z

test/e2e/garbage_collector.go

+			_, err := f.ClientSet.BatchV2alpha1().CronJobs(f.Namespace.Name).Get(cronJob.Name, metav1.GetOptions{})
+			return err != nil && !errors.IsNotFound(err), nil
+		})
+		By("Verify if cronjob does not leave jobs nor pods behind")


These should be wrapped in a poll. The Cronjob is deleted with getNonOrphanOptions, which is equivalent of the "background" GC policy, which causes the cronjob to be deleted immediately, then GC deletes the dependents in the background.

If you use the "foreground" GC, then GC will delete the cronjob after all dependents are deleted, then these checks make sense.

caesarxuchao · 2017-06-29T01:07:44Z

@soltysh too late for 1.7, changed to 1.8

soltysh · 2017-06-29T11:44:36Z

@caesarxuchao I've updated per your suggestion and I've also split the fix to rs and deployment test swallowing errors to a separate commit.

caesarxuchao · 2017-07-19T00:03:19Z

/lgtm

@soltysh could you rebase? Thanks.

k8s-github-robot · 2017-07-19T00:04:12Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caesarxuchao, soltysh

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~test/OWNERS~~ [soltysh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

soltysh · 2017-08-01T13:55:15Z

@caesarxuchao sorry, I was out for almost a month. I've rebased and addressed that single comment of yours. ptal

soltysh · 2017-08-04T14:03:29Z

Bazel fixed, should be good now.

soltysh · 2017-08-08T11:07:09Z

Based on previous approval I'm re-adding the labels.

k8s-github-robot · 2017-08-08T11:07:56Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-08-08T11:58:16Z

Automatic merge from submit-queue

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2017

soltysh added this to the v1.7 milestone Jun 26, 2017

k8s-github-robot assigned madhusudancs and bowei Jun 26, 2017

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jun 26, 2017

soltysh mentioned this pull request Jun 26, 2017

Make background garbage collection cascading #44058

Merged

soltysh force-pushed the cronjob_removal_e2e branch from 018caa2 to 19dbefd Compare June 26, 2017 14:35

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2017

soltysh added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 26, 2017

soltysh assigned caesarxuchao and unassigned madhusudancs and bowei Jun 26, 2017

0xmichalis reviewed Jun 26, 2017

View reviewed changes

caesarxuchao reviewed Jun 26, 2017

View reviewed changes

soltysh force-pushed the cronjob_removal_e2e branch from 19dbefd to f9d135a Compare June 27, 2017 14:18

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 27, 2017

caesarxuchao reviewed Jun 28, 2017

View reviewed changes

caesarxuchao modified the milestones: v1.7, v1.8 Jun 29, 2017

soltysh force-pushed the cronjob_removal_e2e branch from f9d135a to e0e8f29 Compare June 29, 2017 11:44

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 29, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2017

soltysh force-pushed the cronjob_removal_e2e branch from e0e8f29 to 591db87 Compare August 1, 2017 13:54

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 1, 2017

soltysh added 2 commits August 4, 2017 16:02

Fix swallowed errors in RS and deployment tests

f08f504

Add e2e test for cronjob chained removal

17f8c97

soltysh force-pushed the cronjob_removal_e2e branch from 591db87 to 17f8c97 Compare August 4, 2017 14:03

soltysh added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 8, 2017

k8s-github-robot merged commit 6b529fb into kubernetes:master Aug 8, 2017

soltysh deleted the cronjob_removal_e2e branch August 8, 2017 12:01

jennybuckley mentioned this pull request Mar 6, 2018

Fix error handling in gc e2e test #60671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add e2e test for cronjob chained removal #48068

Add e2e test for cronjob chained removal #48068

soltysh commented Jun 26, 2017

0xmichalis Jun 26, 2017

caesarxuchao Jun 26, 2017

0xmichalis commented Jun 26, 2017 •

edited

Loading

caesarxuchao Jun 26, 2017

caesarxuchao Jun 26, 2017

caesarxuchao commented Jun 26, 2017

0xmichalis commented Jun 27, 2017

soltysh commented Jun 27, 2017

soltysh commented Jun 27, 2017

soltysh commented Jun 27, 2017

soltysh commented Jun 28, 2017

caesarxuchao Jun 28, 2017

caesarxuchao Jun 28, 2017

caesarxuchao Jun 28, 2017

caesarxuchao commented Jun 29, 2017

soltysh commented Jun 29, 2017

caesarxuchao commented Jul 19, 2017

k8s-github-robot commented Jul 19, 2017

soltysh commented Aug 1, 2017

soltysh commented Aug 4, 2017

soltysh commented Aug 8, 2017

k8s-github-robot commented Aug 8, 2017

k8s-github-robot commented Aug 8, 2017

		@@ -75,6 +75,13 @@ var _ = framework.KubeDescribe("CronJob", func() {
		By("Removing cronjob")
		err = deleteCronJob(f.ClientSet, f.Namespace.Name, cronJob.Name)

Add e2e test for cronjob chained removal #48068

Add e2e test for cronjob chained removal #48068

Conversation

soltysh commented Jun 26, 2017

0xmichalis Jun 26, 2017

Choose a reason for hiding this comment

caesarxuchao Jun 26, 2017

Choose a reason for hiding this comment

0xmichalis commented Jun 26, 2017 • edited Loading

caesarxuchao Jun 26, 2017

Choose a reason for hiding this comment

caesarxuchao Jun 26, 2017

Choose a reason for hiding this comment

caesarxuchao commented Jun 26, 2017

0xmichalis commented Jun 27, 2017

soltysh commented Jun 27, 2017

soltysh commented Jun 27, 2017

soltysh commented Jun 27, 2017

soltysh commented Jun 28, 2017

caesarxuchao Jun 28, 2017

Choose a reason for hiding this comment

caesarxuchao Jun 28, 2017

Choose a reason for hiding this comment

caesarxuchao Jun 28, 2017

Choose a reason for hiding this comment

caesarxuchao commented Jun 29, 2017

soltysh commented Jun 29, 2017

caesarxuchao commented Jul 19, 2017

k8s-github-robot commented Jul 19, 2017

soltysh commented Aug 1, 2017

soltysh commented Aug 4, 2017

soltysh commented Aug 8, 2017

k8s-github-robot commented Aug 8, 2017

k8s-github-robot commented Aug 8, 2017

0xmichalis commented Jun 26, 2017 •

edited

Loading