From a6bda8f4ceffce4451864af16301a24f2bccb547 Mon Sep 17 00:00:00 2001 From: Juana De La Cuesta Date: Wed, 22 Mar 2023 12:15:58 +0100 Subject: [PATCH] fix: remove defer and inline unlock for speed optimization --- nomad/leader.go | 5 ++-- nomad/leader_test.go | 61 +++++++++++++++++++------------------------- nomad/periodic.go | 4 ++- 3 files changed, 32 insertions(+), 38 deletions(-) diff --git a/nomad/leader.go b/nomad/leader.go index 283c9da3c34c..b7e960626b66 100644 --- a/nomad/leader.go +++ b/nomad/leader.go @@ -790,7 +790,7 @@ func (s *Server) restorePeriodicDispatcher() error { return fmt.Errorf("force run of periodic job %q failed: %v", job.NamespacedID(), err) } - logger.Debug("periodic job force runned during leadership establishment", "job", job.NamespacedID()) + logger.Debug("periodic job force run during leadership establishment", "job", job.NamespacedID()) } return nil @@ -800,8 +800,9 @@ func (s *Server) restorePeriodicDispatcher() error { // 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) { + if job.Periodic.ProhibitOverlap { - running, err := s.RunningChildren(job) + running, err := s.periodicDispatcher.dispatcher.RunningChildren(job) if err != nil { return false, fmt.Errorf("failed to determine if periodic job has running children %q error %q", job.NamespacedID(), err) } diff --git a/nomad/leader_test.go b/nomad/leader_test.go index 5b8f4a8e71e2..1955c4b706cb 100644 --- a/nomad/leader_test.go +++ b/nomad/leader_test.go @@ -439,12 +439,13 @@ func TestLeader_PeriodicDispatcher_Restore_NoEvals(t *testing.T) { } type mockJobEvalDispatcher struct { - called, children bool - evalToReturn *structs.Evaluation + forceEvalCalled, children bool + evalToReturn *structs.Evaluation + JobEvalDispatcher } func (mjed *mockJobEvalDispatcher) DispatchJob(job *structs.Job) (*structs.Evaluation, error) { - mjed.called = true + mjed.forceEvalCalled = true return mjed.evalToReturn, nil } @@ -452,7 +453,7 @@ func (mjed *mockJobEvalDispatcher) RunningChildren(job *structs.Job) (bool, erro return mjed.children, nil } -func testPeriodicJobWithOverlapEnabled(times ...time.Time) *structs.Job { +func testPeriodicJob_OverlapEnabled(times ...time.Time) *structs.Job { job := testPeriodicJob(times...) job.Periodic.ProhibitOverlap = true return job @@ -489,8 +490,9 @@ func TestLeader_PeriodicDispatcher_Restore_Evals(t *testing.T) { eval, _ := s1.periodicDispatcher.createEval(job, past) md := &mockJobEvalDispatcher{ - children: false, - evalToReturn: eval, + children: false, + evalToReturn: eval, + JobEvalDispatcher: s1, } s1.periodicDispatcher.dispatcher = md @@ -525,7 +527,7 @@ func TestLeader_PeriodicDispatcher_Restore_Evals(t *testing.T) { t.Fatalf("restorePeriodicDispatcher did not force launch") } - if md.called != true { + if md.forceEvalCalled != true { t.Fatalf("failed to force job evaluation") } } @@ -545,7 +547,7 @@ func TestLeader_PeriodicDispatcher_No_Overlaps_No_Running_Job(t *testing.T) { past := now.Add(-1 * time.Second) future := now.Add(10 * time.Second) - job := testPeriodicJobWithOverlapEnabled(past, now, future) + job := testPeriodicJob_OverlapEnabled(past, now, future) req := structs.JobRegisterRequest{ Job: job, WriteRequest: structs.WriteRequest{ @@ -571,7 +573,7 @@ func TestLeader_PeriodicDispatcher_No_Overlaps_No_Running_Job(t *testing.T) { s1.periodicDispatcher.SetEnabled(false) // Sleep till after the job should have been launched. - time.Sleep(5 * time.Second) + time.Sleep(3 * time.Second) // Restore the periodic dispatcher. s1.periodicDispatcher.SetEnabled(true) @@ -597,9 +599,10 @@ func TestLeader_PeriodicDispatcher_No_Overlaps_No_Running_Job(t *testing.T) { t.Fatalf("restorePeriodicDispatcher did not force launch") } - if md.called != true { + if md.forceEvalCalled != true { t.Fatalf("failed to force job evaluation") } + t.Fail() } func TestLeader_PeriodicDispatcher_No_Overlaps_Running_Job(t *testing.T) { @@ -611,23 +614,15 @@ func TestLeader_PeriodicDispatcher_No_Overlaps_Running_Job(t *testing.T) { defer cleanupS1() testutil.WaitForLeader(t, s1.RPC) - md := &mockJobEvalDispatcher{ - children: true, - evalToReturn: nil, - } - - s1.periodicDispatcher.dispatcher = md - // Inject a periodic job that triggered once in the past, should trigger now // and once in the future. now := time.Now() past := now.Add(-1 * time.Second) future := now.Add(10 * time.Second) - job := testPeriodicJob(past, now, future) + job := testPeriodicJob_OverlapEnabled(past, now, future) req := structs.JobRegisterRequest{ - Job: job, - PolicyOverride: true, + Job: job, WriteRequest: structs.WriteRequest{ Namespace: job.Namespace, }, @@ -638,13 +633,20 @@ func TestLeader_PeriodicDispatcher_No_Overlaps_Running_Job(t *testing.T) { } // Create an eval for the past launch. - s1.periodicDispatcher.createEval(job, past) + eval, _ := s1.periodicDispatcher.createEval(job, past) + + md := &mockJobEvalDispatcher{ + children: true, + evalToReturn: eval, + } + + s1.periodicDispatcher.dispatcher = md // Flush the periodic dispatcher, ensuring that no evals will be created. s1.periodicDispatcher.SetEnabled(false) // Sleep till after the job should have been launched. - time.Sleep(5 * time.Second) + time.Sleep(3 * time.Second) // Restore the periodic dispatcher. s1.periodicDispatcher.SetEnabled(true) @@ -659,19 +661,8 @@ func TestLeader_PeriodicDispatcher_No_Overlaps_Running_Job(t *testing.T) { t.Fatalf("periodic job not restored") } - // Check that an eval was made. - ws := memdb.NewWatchSet() - last, err := s1.fsm.State().PeriodicLaunchByID(ws, job.Namespace, job.ID) - if err != nil || last == nil { - t.Fatalf("failed to get periodic launch time: %v", err) - } - - if last.Launch == past { - t.Fatalf("restorePeriodicDispatcher did not force launch") - } - - if md.called != false { - t.Fatalf("job evaluation forced when job is already running") + if md.forceEvalCalled != false { + t.Fatalf("evaluation forced with job already running") } } diff --git a/nomad/periodic.go b/nomad/periodic.go index 3ac79a1ec1e6..30d3468ab827 100644 --- a/nomad/periodic.go +++ b/nomad/periodic.go @@ -333,7 +333,6 @@ func (p *PeriodicDispatch) run(ctx context.Context, updateCh <-chan struct{}) { // based on the passed launch time. func (p *PeriodicDispatch) dispatch(job *structs.Job, launchTime time.Time) { p.l.Lock() - defer p.l.Unlock() nextLaunch, err := job.Periodic.Next(launchTime) if err != nil { @@ -348,16 +347,19 @@ func (p *PeriodicDispatch) dispatch(job *structs.Job, launchTime time.Time) { running, err := p.dispatcher.RunningChildren(job) if err != nil { p.logger.Error("failed to determine if periodic job has running children", "job", job.NamespacedID(), "error", err) + p.l.Unlock() return } if running { p.logger.Debug("skipping launch of periodic job because job prohibits overlap", "job", job.NamespacedID()) + p.l.Unlock() return } } p.logger.Debug(" launching job", "job", job.NamespacedID(), "launch_time", launchTime) + p.l.Unlock() p.createEval(job, launchTime) }