feat: add e2e test for ttl seconds after finished in jobset #511

dejanzele · 2024-04-13T22:56:41Z

CHANGELOG

add e2e test for ttl seconds after finished in jobset

This PR is an improvement for #279

k8s-ci-robot · 2024-04-13T22:56:50Z

Hi @dejanzele. 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.

netlify · 2024-04-13T22:57:00Z

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Name	Link
🔨 Latest commit	`4988703`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/661e7b2ad66a7800084b94ee

dejanzele · 2024-04-13T22:57:27Z

/cc @ahg-g

kannon92 · 2024-04-14T12:54:31Z

/ok-to-test

danielvegamyhre · 2024-04-14T17:32:08Z

test/e2e/e2e_test.go

+
+			// Create JobSet.
+			ginkgo.By("creating jobset with ttl seconds after finished")
+			js := pingTestJobSetSubdomain(ns).TTLSecondsAfterFinished(5).Obj()


Let's not have the TTL test also be doing an unrelated networking test, this can cause issues/flakiness unrelated to the TTL feature being tested here. We should isolate the feature being tested to reduce flakiness and make debugging test failures easier.

To avoid flakiness, we need to create the jobset with a finalizer to ensure that the jobset doesn't get deleted before we check that it exists and is complete; and then before we do the JobSetDeleted check, we remove the finalizer.

danielvegamyhre

Thanks for working on this, left one comment.

ahg-g · 2024-04-14T17:50:57Z

test/e2e/e2e_test.go

+			gomega.Expect(k8sClient.Create(ctx, js)).Should(gomega.Succeed())
+
+			// We'll need to retry getting this newly created jobset, given that creation may not immediately happen.
+			gomega.Eventually(k8sClient.Get(ctx, types.NamespacedName{Name: js.Name, Namespace: js.Namespace}, &jobset.JobSet{}), timeout, interval).Should(gomega.Succeed())


if create succeeded above, we shouldn't need to use eventually here

ahg-g · 2024-04-14T17:58:40Z

test/e2e/e2e_test.go

+			ginkgo.By("checking all jobs were created successfully")
+			gomega.Eventually(util.NumJobs, timeout, interval).WithArguments(ctx, k8sClient, js).Should(gomega.Equal(util.NumExpectedJobs(js)))


pls remove this check, we may face a race where the jobset is completed and marked as deleted before we get here.

ahg-g · 2024-04-14T17:59:07Z

test/e2e/e2e_test.go

+
+			// Create JobSet.
+			ginkgo.By("creating jobset with ttl seconds after finished")
+			js := pingTestJobSetSubdomain(ns).TTLSecondsAfterFinished(5).Obj()


To avoid flakiness, we need to create the jobset with a finalizer to ensure that the jobset doesn't get deleted before we check that it exists and is complete; and then before we do the JobSetDeleted check, we remove the finalizer.

dejanzele · 2024-04-16T01:04:43Z

@danielvegamyhre @ahg-g can you take another look please?

ahg-g · 2024-04-16T04:58:01Z

test/util/util.go

+	ginkgo.By("removing jobset finalizers")
+	for i, f := range js.Finalizers {
+		if f == finalizer {
+			js.Finalizers = append(js.Finalizers[:i], js.Finalizers[i+1:]...)
+		}
+	}
+	gomega.Expect(k8sClient.Update(ctx, js)).Should(gomega.Succeed())


wrap this in Eventually to handle potential update conflicts

ahg-g · 2024-04-16T04:59:26Z

test/e2e/e2e_test.go

+
+			// Check jobset is cleaned up after ttl seconds.
+			ginkgo.By("checking jobset is cleaned up after ttl seconds")
+			util.JobSetDeleted(ctx, k8sClient, &fresh, timeout)


use js instead of fresh here

ahg-g · 2024-04-16T05:00:40Z

test/e2e/e2e_test.go

+
+			// We get the latest version of the jobset before removing the finalizer
+			var fresh jobset.JobSet
+			gomega.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: js.Name, Namespace: js.Namespace}, &fresh)).Should(gomega.Succeed())


move this get to inside removeJobSetFinalizer within Eventually (see comment there)

dejanzele · 2024-04-16T14:06:51Z

@ahg-g thanks for the review, I have addressed your comments

ahg-g · 2024-04-16T16:32:07Z

/lgtm
/approve

k8s-ci-robot · 2024-04-16T16:32:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, dejanzele

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:

~~OWNERS~~ [ahg-g]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahg-g · 2024-04-16T16:33:06Z

Thanks! one thing I usually do is to force a bug in the test to check if it fails, just to verify that it is working as expected. I wonder if you did that?

dejanzele · 2024-04-16T22:38:47Z

@ahg-g what kind of bug? i.e. the test will fail if you don't get latest version of the jobset before updating as it will fail due to conflict, or it will fail if you delete the jobset manually in the middle of the test run, I did those two things which forced a failure

k8s-ci-robot requested review from danielvegamyhre and kannon92 April 13, 2024 22:56

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 13, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 13, 2024

k8s-ci-robot requested a review from ahg-g April 13, 2024 22:57

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 Apr 14, 2024

danielvegamyhre reviewed Apr 14, 2024

View reviewed changes

danielvegamyhre requested changes Apr 14, 2024

View reviewed changes

ahg-g reviewed Apr 14, 2024

View reviewed changes

dejanzele force-pushed the feat/jobset-ttl-e2e-test branch 2 times, most recently from 1122e1d to ea522c4 Compare April 16, 2024 00:10

ahg-g reviewed Apr 16, 2024

View reviewed changes

add e2e test for ttl seconds after finished in jobset

4988703

dejanzele force-pushed the feat/jobset-ttl-e2e-test branch from ea522c4 to 4988703 Compare April 16, 2024 13:20

k8s-ci-robot assigned ahg-g Apr 16, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2024

k8s-ci-robot merged commit 8342871 into kubernetes-sigs:main Apr 16, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add e2e test for ttl seconds after finished in jobset #511

feat: add e2e test for ttl seconds after finished in jobset #511

dejanzele commented Apr 13, 2024

k8s-ci-robot commented Apr 13, 2024

netlify bot commented Apr 13, 2024 •

edited

dejanzele commented Apr 13, 2024

kannon92 commented Apr 14, 2024

danielvegamyhre Apr 14, 2024

ahg-g Apr 14, 2024

danielvegamyhre left a comment

ahg-g Apr 14, 2024

ahg-g Apr 14, 2024

ahg-g Apr 14, 2024

dejanzele commented Apr 16, 2024

ahg-g Apr 16, 2024

ahg-g Apr 16, 2024

ahg-g Apr 16, 2024

dejanzele commented Apr 16, 2024

ahg-g commented Apr 16, 2024

k8s-ci-robot commented Apr 16, 2024

ahg-g commented Apr 16, 2024

dejanzele commented Apr 16, 2024 •

edited

		ginkgo.By("checking all jobs were created successfully")
		gomega.Eventually(util.NumJobs, timeout, interval).WithArguments(ctx, k8sClient, js).Should(gomega.Equal(util.NumExpectedJobs(js)))

feat: add e2e test for ttl seconds after finished in jobset #511

feat: add e2e test for ttl seconds after finished in jobset #511

Conversation

dejanzele commented Apr 13, 2024

k8s-ci-robot commented Apr 13, 2024

netlify bot commented Apr 13, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

dejanzele commented Apr 13, 2024

kannon92 commented Apr 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielvegamyhre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dejanzele commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dejanzele commented Apr 16, 2024

ahg-g commented Apr 16, 2024

k8s-ci-robot commented Apr 16, 2024

ahg-g commented Apr 16, 2024

dejanzele commented Apr 16, 2024 • edited

netlify bot commented Apr 13, 2024 •

edited

dejanzele commented Apr 16, 2024 •

edited