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.

Sign up for GitHub

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

Jump to bottom

fix provision_observer2 to delete failing Jobs correctly #114

Merged
merged 1 commit into from
Apr 2, 2024
Merged

fix provision_observer2 to delete failing Jobs correctly #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/controller/provision_observer2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func (p *provisionObserver2) getNodeNameAndStorageClass(ctx context.Context, nam
}

func isProbeJob2(o metav1.OwnerReference) bool {
return o.Kind == "Job" && strings.HasPrefix(o.Name, constants.ProbeNamePrefix)
return o.Kind == "Job" && (strings.HasPrefix(o.Name, constants.MountProbeNamePrefix) || strings.HasPrefix(o.Name, constants.ProvisionProbeNamePrefix))
}

func (p *provisionObserver2) deleteOwnerJobOfPod(ctx context.Context, namespace, podName string) error {
Expand Down
7 changes: 7 additions & 0 deletions test/e2e/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,13 @@ var _ = Describe("PieProbe resource", func() {
}
}
}).Should(Succeed())

// Check that failing Jobs are correctly removed.
Eventually(func(g Gomega) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I consider this test is unstable. I guess the failed jobs can be removed before this Eventually section starts.

How about checking only the deletion of the failed jobs? I mean, it might be good to remove the first Eventually section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you. I updated my patch. The changes are: https://github.com/topolvm/pie/compare/81787eaa12e9bf8ade9d452c4d58ed026af3ec7a..b8496f03e5f5c70a5a03ff72476f85669e1652e7

the failed jobs can be removed before this Eventually section starts.

Actually, even in this situation, the test should pass because the cronjob will create another job, while it can take a long time. I wrote the first Eventually to make the test fail when no jobs are created during the whole run for some reason. But this situation cannot happen because we already check the metrics (on_time=false) for dummy-sc. So, I believe the first Eventually is essentially meaningless.

stdout, _, err := kubectl("get", "job", "-n", ns, "-l", "pie-probe=pie-probe-dummy-sc")
g.Expect(err).NotTo(HaveOccurred())
g.Expect(stdout).Should(BeEmpty())
}).Should(Succeed())
})
})

Expand Down