Multiple instances of a periodic job are run simultaneously, when prohibit_overlap is true #16583

Juanadelacuesta · 2023-03-21T10:32:49Z

This PR addresses the bug reported on #11052

When a leader change happens, the periodic dispatcher on the new leader starts by re running all periodic jobs by force, without checking if there is an instance of the said job already.
A new check is introduced that skips the job if prohibit_overlap is set and there is already a instance running.

jrasell

Looking good, thanks so much for taking this on. Most on the suggestions are nitpicky around using must and a couple of stylistic suggestions.

The biggest item is that we need to revert the changes made within nomad/periodic.go due to the problem described in my comment.

There also seem to be failures and panics within the CI tests that we should look into and fix before merging.

The PR will also need a changelog entry before we can merge.

nomad/leader.go

jrasell · 2023-03-22T13:01:51Z

nomad/leader.go

+// isNewEvalNeeded checks if the job allows for overlap and if there are already
+// instances of the job running in order to determine if a new evaluation needs to
+// be created upon periodic dispatcher restore
+func (s *Server) isNewEvalNeeded(job *structs.Job) (bool, error) {


Not to bikeshed too much, but this function is specific to periodic jobs, therefore it might be nice to have a more descriptive name. Otherwise, this seems like a generic job helper function at first glance.

jrasell · 2023-03-22T13:02:56Z

nomad/leader_test.go

@@ -465,7 +487,15 @@ func TestLeader_PeriodicDispatcher_Restore_Evals(t *testing.T) {
 	}

 	// Create an eval for the past launch.
-	s1.periodicDispatcher.createEval(job, past)
+	eval, _ := s1.periodicDispatcher.createEval(job, past)


Let check this error return using must.NoError to be sure.

nomad/leader_test.go

jrasell · 2023-03-22T13:17:44Z

nomad/periodic.go

@@ -278,10 +278,10 @@ func (p *PeriodicDispatch) removeLocked(jobID structs.NamespacedID) error {
 // subsequent eval.
 func (p *PeriodicDispatch) ForceRun(namespace, jobID string) (*structs.Evaluation, error) {
 	p.l.Lock()
+	defer p.l.Unlock()


I think we need to go back to the original style here, as the comment on the createEval function reads "This should not be called with the lock held."

nomad/leader_test.go

tgross · 2023-03-22T14:16:24Z

nomad/periodic.go

@@ -278,10 +278,10 @@ func (p *PeriodicDispatch) removeLocked(jobID structs.NamespacedID) error {
 // subsequent eval.
 func (p *PeriodicDispatch) ForceRun(namespace, jobID string) (*structs.Evaluation, error) {


I realize this is an existing method name, but I wonder if calling this ForceRun rather than ForceEval is misleading.

nomad/leader.go

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

createEval cant be called with the lock on

…hibit_overlap is true (#16583) * Multiple instances of a periodic job are run simultaneously, when prohibit_overlap is true Fixes #11052 When restoring periodic dispatcher, all periodic jobs are forced without checking for previous childre. * Multiple instances of a periodic job are run simultaneously, when prohibit_overlap is true Fixes #11052 When restoring periodic dispatcher, all periodic jobs are forced without checking for previous children. * style: refactor force run function * fix: remove defer and inline unlock for speed optimization * Update nomad/leader.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * style: refactor tests to use must * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/leader_test.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * fix: move back from defer to calling unlock before returning. createEval cant be called with the lock on * style: refactor test to use must * added new entry to changelog and update comments --------- Co-authored-by: James Rasell <jrasell@hashicorp.com> Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

…hibit_overlap is true (#16583) (#16683) Co-authored-by: Juana De La Cuesta <juanita.delacuestamorales@hashicorp.com>

…hibit_overlap is true (#16583) (#16682) Co-authored-by: Juana De La Cuesta <juanita.delacuestamorales@hashicorp.com>

…hibit_overlap is true (#16583) (#16681) Co-authored-by: James Rasell <jrasell@hashicorp.com> Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

vercel bot deployed to Preview – nomad-storybook-and-ui March 21, 2023 10:34 View deployment

Juanadelacuesta force-pushed the b-gh-11052 branch from 7f5b240 to e4cb7db Compare March 21, 2023 11:21

vercel bot deployed to Preview – nomad-storybook-and-ui March 21, 2023 11:27 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui March 21, 2023 15:18 View deployment

Juanadelacuesta force-pushed the b-gh-11052 branch from 8920548 to 9a9faf7 Compare March 21, 2023 18:44

vercel bot deployed to Preview – nomad-storybook-and-ui March 21, 2023 18:49 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui March 22, 2023 11:20 View deployment

Juanadelacuesta force-pushed the b-gh-11052 branch 2 times, most recently from 4137e01 to a6bda8f Compare March 22, 2023 12:16

vercel bot deployed to Preview – nomad-storybook-and-ui March 22, 2023 12:21 View deployment

Juanadelacuesta force-pushed the b-gh-11052 branch from a6bda8f to eec8f23 Compare March 22, 2023 12:31

Juanadelacuesta changed the title ~~WIP~~ Multiple instances of a periodic job are run simultaneously, when prohibit_overlap is true Mar 22, 2023

vercel bot deployed to Preview – nomad-storybook-and-ui March 22, 2023 12:37 View deployment

Juanadelacuesta marked this pull request as ready for review March 22, 2023 12:41

Juanadelacuesta requested review from jrasell and pkazmierczak and removed request for jrasell March 22, 2023 12:48

jrasell requested changes Mar 22, 2023

View reviewed changes

jrasell requested a review from tgross March 22, 2023 13:22

tgross reviewed Mar 22, 2023

View reviewed changes

vercel bot deployed to Preview – nomad-storybook-and-ui March 22, 2023 15:04 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui March 22, 2023 15:22 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui March 22, 2023 15:36 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui March 22, 2023 15:44 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui March 22, 2023 15:55 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui March 22, 2023 16:03 View deployment

Juanadelacuesta requested review from jrasell and tgross March 24, 2023 09:15

vercel bot deployed to Preview – nomad-storybook-and-ui March 24, 2023 09:30 View deployment

Juanadelacuesta and others added 15 commits March 27, 2023 17:01

Update nomad/leader_test.go

23807bd

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

Update nomad/leader_test.go

eb6cd35

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

Update nomad/leader_test.go

f4c24bc

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

Update nomad/leader_test.go

62125b1

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

Update nomad/leader_test.go

b3eacaa

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

Update nomad/leader_test.go

c762dc8

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

style: refactor tests to use must

d182bd5

Update nomad/leader_test.go

dd05f71

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

Update nomad/leader_test.go

f4352f0

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

Update nomad/leader_test.go

2a9a785

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

Update nomad/leader_test.go

f841f4f

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

Update nomad/leader_test.go

186f982

Co-authored-by: James Rasell <jrasell@users.noreply.github.com>

fix: move back from defer to calling unlock before returning.

816e27c

createEval cant be called with the lock on

style: refactor test to use must

3cbb6f4

added new entry to changelog and update comments

75ccde6

Juanadelacuesta force-pushed the b-gh-11052 branch from 407090f to 75ccde6 Compare March 27, 2023 15:04

vercel bot deployed to Preview – nomad-storybook-and-ui March 27, 2023 15:08 View deployment

Juanadelacuesta merged commit e773710 into main Mar 27, 2023

Juanadelacuesta deleted the b-gh-11052 branch March 27, 2023 15:25

jrasell added a commit that referenced this pull request Mar 28, 2023

Multiple instances of a periodic job are run simultaneously, when pro…

9073457

…hibit_overlap is true (#16583) (#16683) Co-authored-by: Juana De La Cuesta <juanita.delacuestamorales@hashicorp.com>

jrasell added a commit that referenced this pull request Mar 28, 2023

Multiple instances of a periodic job are run simultaneously, when pro…

da9e871

…hibit_overlap is true (#16583) (#16682) Co-authored-by: Juana De La Cuesta <juanita.delacuestamorales@hashicorp.com>

jrasell mentioned this pull request Apr 4, 2023

ProhibitOverlap not respected after a restart #14505

Closed

shoenig mentioned this pull request May 16, 2023

manual backport 889c5aa0f7 #17208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple instances of a periodic job are run simultaneously, when prohibit_overlap is true #16583

Multiple instances of a periodic job are run simultaneously, when prohibit_overlap is true #16583

Juanadelacuesta commented Mar 21, 2023 •

edited

Loading

jrasell left a comment •

edited

Loading

jrasell Mar 22, 2023

jrasell Mar 22, 2023

jrasell Mar 22, 2023

tgross Mar 22, 2023

		@@ -278,10 +278,10 @@ func (p *PeriodicDispatch) removeLocked(jobID structs.NamespacedID) error {
		// subsequent eval.
		func (p PeriodicDispatch) ForceRun(namespace, jobID string) (structs.Evaluation, error) {

Multiple instances of a periodic job are run simultaneously, when prohibit_overlap is true #16583

Multiple instances of a periodic job are run simultaneously, when prohibit_overlap is true #16583

Conversation

Juanadelacuesta commented Mar 21, 2023 • edited Loading

jrasell left a comment • edited Loading

Choose a reason for hiding this comment

jrasell Mar 22, 2023

Choose a reason for hiding this comment

jrasell Mar 22, 2023

Choose a reason for hiding this comment

jrasell Mar 22, 2023

Choose a reason for hiding this comment

tgross Mar 22, 2023

Choose a reason for hiding this comment

Juanadelacuesta commented Mar 21, 2023 •

edited

Loading

jrasell left a comment •

edited

Loading