From af2ed66749e1984d31e3c3cc23ef38addc067f5a Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Fri, 10 May 2019 17:15:27 -0500 Subject: [PATCH 1/7] Add flag similar to --all for allocs to be able to filter deployments by latest --- api/jobs.go | 12 +++++- command/agent/job_endpoint.go | 6 ++- command/job_deployments.go | 5 ++- e2e/consul/consul.go | 4 +- e2e/rescheduling/server_side_restarts_test.go | 2 +- nomad/job_endpoint.go | 6 +-- nomad/state/state_store.go | 20 +++++++++- nomad/state/state_store_test.go | 40 ++++++++++++++++++- nomad/structs/structs.go | 4 +- 9 files changed, 82 insertions(+), 17 deletions(-) diff --git a/api/jobs.go b/api/jobs.go index aa74406d429..973250f3034 100644 --- a/api/jobs.go +++ b/api/jobs.go @@ -179,9 +179,17 @@ func (j *Jobs) Allocations(jobID string, allAllocs bool, q *QueryOptions) ([]*Al // Deployments is used to query the deployments associated with the given job // ID. -func (j *Jobs) Deployments(jobID string, q *QueryOptions) ([]*Deployment, *QueryMeta, error) { +func (j *Jobs) Deployments(jobID string, all bool, q *QueryOptions) ([]*Deployment, *QueryMeta, error) { var resp []*Deployment - qm, err := j.client.query("/v1/job/"+jobID+"/deployments", &resp, q) + u, err := url.Parse("/v1/job/" + jobID + "/deployments") + if err != nil { + return nil, nil, err + } + + v := u.Query() + v.Add("all", strconv.FormatBool(all)) + u.RawQuery = v.Encode() + qm, err := j.client.query(u.String(), &resp, q) if err != nil { return nil, nil, err } diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go index e343af64987..c2944023434 100644 --- a/command/agent/job_endpoint.go +++ b/command/agent/job_endpoint.go @@ -219,8 +219,8 @@ func (s *HTTPServer) jobAllocations(resp http.ResponseWriter, req *http.Request, allAllocs, _ := strconv.ParseBool(req.URL.Query().Get("all")) args := structs.JobSpecificRequest{ - JobID: jobName, - AllAllocs: allAllocs, + JobID: jobName, + All: allAllocs, } if s.parse(resp, req, &args.Region, &args.QueryOptions) { return nil, nil @@ -270,8 +270,10 @@ func (s *HTTPServer) jobDeployments(resp http.ResponseWriter, req *http.Request, if req.Method != "GET" { return nil, CodedError(405, ErrInvalidMethod) } + all, _ := strconv.ParseBool(req.URL.Query().Get("all")) args := structs.JobSpecificRequest{ JobID: jobName, + All: all, } if s.parse(resp, req, &args.Region, &args.QueryOptions) { return nil, nil diff --git a/command/job_deployments.go b/command/job_deployments.go index b099a305cee..22f87e86a5d 100644 --- a/command/job_deployments.go +++ b/command/job_deployments.go @@ -71,13 +71,14 @@ func (c *JobDeploymentsCommand) AutocompleteArgs() complete.Predictor { func (c *JobDeploymentsCommand) Name() string { return "job deployments" } func (c *JobDeploymentsCommand) Run(args []string) int { - var json, latest, verbose bool + var json, latest, verbose, all bool var tmpl string flags := c.Meta.FlagSet(c.Name(), FlagSetClient) flags.Usage = func() { c.Ui.Output(c.Help()) } flags.BoolVar(&latest, "latest", false, "") flags.BoolVar(&verbose, "verbose", false, "") + flags.BoolVar(&all, "all", false, "") flags.BoolVar(&json, "json", false, "") flags.StringVar(&tmpl, "t", "", "") @@ -146,7 +147,7 @@ func (c *JobDeploymentsCommand) Run(args []string) int { return 0 } - deploys, _, err := client.Jobs().Deployments(jobID, nil) + deploys, _, err := client.Jobs().Deployments(jobID, all, nil) if err != nil { c.Ui.Error(fmt.Sprintf("Error retrieving deployments: %s", err)) return 1 diff --git a/e2e/consul/consul.go b/e2e/consul/consul.go index 29296db27fc..5809e251774 100644 --- a/e2e/consul/consul.go +++ b/e2e/consul/consul.go @@ -113,7 +113,7 @@ func (tc *ConsulE2ETest) TestCanaryInplaceUpgrades(f *framework.F) { g := NewGomegaWithT(f.T()) g.Eventually(func() []string { - deploys, _, err := jobs.Deployments(jobId, nil) + deploys, _, err := jobs.Deployments(jobId, false, nil) require.Nil(err) healthyDeploys := make([]string, 0, len(deploys)) for _, d := range deploys { @@ -135,7 +135,7 @@ func (tc *ConsulE2ETest) TestCanaryInplaceUpgrades(f *framework.F) { // Eventually have a canary var deploys []*api.Deployment g.Eventually(func() []*api.Deployment { - deploys, _, err = jobs.Deployments(*job.ID, nil) + deploys, _, err = jobs.Deployments(*job.ID, false, nil) require.Nil(err) return deploys }, 2*time.Second, 20*time.Millisecond).Should(HaveLen(2)) diff --git a/e2e/rescheduling/server_side_restarts_test.go b/e2e/rescheduling/server_side_restarts_test.go index 850e01f0f05..f03609d5f14 100644 --- a/e2e/rescheduling/server_side_restarts_test.go +++ b/e2e/rescheduling/server_side_restarts_test.go @@ -53,7 +53,7 @@ var _ = Describe("Server Side Restart Tests", func() { // deploymentStatus is a helper function that returns deployment status of all deployments // sorted by time deploymentStatus = func() []string { - deploys, _, err := jobs.Deployments(*job.ID, nil) + deploys, _, err := jobs.Deployments(*job.ID, false, nil) Expect(err).ShouldNot(HaveOccurred()) var ret []string sort.Slice(deploys, func(i, j int) bool { diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 746c5e3cf0e..9648be01cef 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -951,7 +951,7 @@ func (j *Job) Allocations(args *structs.JobSpecificRequest, queryMeta: &reply.QueryMeta, run: func(ws memdb.WatchSet, state *state.StateStore) error { // Capture the allocations - allocs, err := state.AllocsByJob(ws, args.RequestNamespace(), args.JobID, args.AllAllocs) + allocs, err := state.AllocsByJob(ws, args.RequestNamespace(), args.JobID, args.All) if err != nil { return err } @@ -1042,7 +1042,7 @@ func (j *Job) Deployments(args *structs.JobSpecificRequest, queryMeta: &reply.QueryMeta, run: func(ws memdb.WatchSet, state *state.StateStore) error { // Capture the deployments - deploys, err := state.DeploymentsByJobID(ws, args.RequestNamespace(), args.JobID) + deploys, err := state.DeploymentsByJobID(ws, args.RequestNamespace(), args.JobID, args.All) if err != nil { return err } @@ -1084,7 +1084,7 @@ func (j *Job) LatestDeployment(args *structs.JobSpecificRequest, queryMeta: &reply.QueryMeta, run: func(ws memdb.WatchSet, state *state.StateStore) error { // Capture the deployments - deploys, err := state.DeploymentsByJobID(ws, args.RequestNamespace(), args.JobID) + deploys, err := state.DeploymentsByJobID(ws, args.RequestNamespace(), args.JobID, args.All) if err != nil { return err } diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index 276c6920d29..57454a58cbd 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -481,7 +481,7 @@ func (s *StateStore) deploymentByIDImpl(ws memdb.WatchSet, deploymentID string, return nil, nil } -func (s *StateStore) DeploymentsByJobID(ws memdb.WatchSet, namespace, jobID string) ([]*structs.Deployment, error) { +func (s *StateStore) DeploymentsByJobID(ws memdb.WatchSet, namespace, jobID string, all bool) ([]*structs.Deployment, error) { txn := s.db.Txn(false) // COMPAT 0.7: Upgrade old objects that do not have namespaces @@ -503,8 +503,24 @@ func (s *StateStore) DeploymentsByJobID(ws memdb.WatchSet, namespace, jobID stri if raw == nil { break } - d := raw.(*structs.Deployment) + + // If the allocation belongs to a job with the same ID but a different + // create index and we are not getting all the allocations whose Jobs + // matches the same Job ID then we skip it + + watchCh, existing, err := txn.FirstWatch("jobs", "id", namespace, jobID) + if err != nil { + return nil, fmt.Errorf("job lookup failed: %v", err) + } + ws.Add(watchCh) + var job *structs.Job + if existing != nil { + job = existing.(*structs.Job) + } + if !all && job != nil && d.JobCreateIndex != job.CreateIndex { + continue + } out = append(out, d) } diff --git a/nomad/state/state_store_test.go b/nomad/state/state_store_test.go index 013f9cab932..e272050f794 100644 --- a/nomad/state/state_store_test.go +++ b/nomad/state/state_store_test.go @@ -494,7 +494,7 @@ func TestStateStore_UpsertDeployment(t *testing.T) { // Create a watchset so we can test that upsert fires the watch ws := memdb.NewWatchSet() - _, err := state.DeploymentsByJobID(ws, deployment.Namespace, deployment.ID) + _, err := state.DeploymentsByJobID(ws, deployment.Namespace, deployment.ID, true) if err != nil { t.Fatalf("bad: %v", err) } @@ -530,6 +530,44 @@ func TestStateStore_UpsertDeployment(t *testing.T) { } } +// Tests that deployments of older create index and same job id are not returned +func TestStateStore_OldDeployment(t *testing.T) { + state := testStateStore(t) + job := mock.Job() + job.ID = "job1" + state.UpsertJob(1000, job) + + deploy1 := mock.Deployment() + deploy1.JobID = job.ID + deploy1.JobCreateIndex = job.CreateIndex + + deploy2 := mock.Deployment() + deploy2.JobID = job.ID + deploy2.JobCreateIndex = 11 + + require := require.New(t) + + // Insert both deployments + err := state.UpsertDeployment(1001, deploy1) + require.Nil(err) + + err = state.UpsertDeployment(1002, deploy2) + require.Nil(err) + + ws := memdb.NewWatchSet() + // Should return both deployments + deploys, err := state.DeploymentsByJobID(ws, deploy1.Namespace, job.ID, true) + require.Nil(err) + require.Len(deploys, 2) + + // Should only return deploy1 + deploys, err = state.DeploymentsByJobID(ws, deploy1.Namespace, job.ID, false) + require.Nil(err) + require.Len(deploys, 1) + require.Equal(deploy1.ID, deploys[0].ID) + +} + func TestStateStore_DeleteDeployment(t *testing.T) { state := testStateStore(t) d1 := mock.Deployment() diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 1689422f448..6f592179a3e 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -508,8 +508,8 @@ type EvalOptions struct { // JobSpecificRequest is used when we just need to specify a target job type JobSpecificRequest struct { - JobID string - AllAllocs bool + JobID string + All bool QueryOptions } From 24327ad51601e7319dbdb728f6d21afc2fdbe767 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Mon, 13 May 2019 10:16:36 -0500 Subject: [PATCH 2/7] Lookup job only once, and fix tests --- nomad/job_endpoint_test.go | 1 + nomad/state/state_store.go | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go index c3746c0d846..4a4462b7407 100644 --- a/nomad/job_endpoint_test.go +++ b/nomad/job_endpoint_test.go @@ -3771,6 +3771,7 @@ func TestJobEndpoint_LatestDeployment_Blocking(t *testing.T) { d2 := mock.Deployment() d2.JobID = j.ID require.Nil(state.UpsertJob(50, j), "UpsertJob") + d2.JobCreateIndex = j.CreateIndex // First upsert an unrelated eval time.AfterFunc(100*time.Millisecond, func() { diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index 57454a58cbd..c685010779e 100644 --- a/nomad/state/state_store.go +++ b/nomad/state/state_store.go @@ -489,6 +489,16 @@ func (s *StateStore) DeploymentsByJobID(ws memdb.WatchSet, namespace, jobID stri namespace = structs.DefaultNamespace } + var job *structs.Job + // Read job from state store + _, existing, err := txn.FirstWatch("jobs", "id", namespace, jobID) + if err != nil { + return nil, fmt.Errorf("job lookup failed: %v", err) + } + if existing != nil { + job = existing.(*structs.Job) + } + // Get an iterator over the deployments iter, err := txn.Get("deployment", "job", namespace, jobID) if err != nil { @@ -508,16 +518,6 @@ func (s *StateStore) DeploymentsByJobID(ws memdb.WatchSet, namespace, jobID stri // If the allocation belongs to a job with the same ID but a different // create index and we are not getting all the allocations whose Jobs // matches the same Job ID then we skip it - - watchCh, existing, err := txn.FirstWatch("jobs", "id", namespace, jobID) - if err != nil { - return nil, fmt.Errorf("job lookup failed: %v", err) - } - ws.Add(watchCh) - var job *structs.Job - if existing != nil { - job = existing.(*structs.Job) - } if !all && job != nil && d.JobCreateIndex != job.CreateIndex { continue } From dee16743b464e18737cb5a2f7b25794d8f136081 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Mon, 13 May 2019 18:53:47 -0500 Subject: [PATCH 3/7] Fix test setup to have correct jobcreateindex for deployments --- command/agent/job_endpoint_test.go | 2 ++ command/job_deployments_test.go | 2 ++ nomad/job_endpoint_test.go | 11 ++++++++++- 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index f93396939b5..b99a5926b7a 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -797,6 +797,8 @@ func TestHTTP_JobDeployments(t *testing.T) { state := s.Agent.server.State() d := mock.Deployment() d.JobID = j.ID + d.JobCreateIndex = resp.JobModifyIndex + assert.Nil(state.UpsertDeployment(1000, d), "UpsertDeployment") // Make the HTTP request diff --git a/command/job_deployments_test.go b/command/job_deployments_test.go index 9cb40f056ee..448ddab4b95 100644 --- a/command/job_deployments_test.go +++ b/command/job_deployments_test.go @@ -69,6 +69,7 @@ func TestJobDeploymentsCommand_Run(t *testing.T) { // Inject a deployment d := mock.Deployment() d.JobID = job.ID + d.JobCreateIndex = job.CreateIndex assert.Nil(state.UpsertDeployment(200, d)) // Should now display the deployment @@ -112,6 +113,7 @@ func TestJobDeploymentsCommand_Run_Latest(t *testing.T) { // Inject a deployment d := mock.Deployment() d.JobID = job.ID + d.JobCreateIndex = job.CreateIndex assert.Nil(state.UpsertDeployment(200, d)) // Should now display the deployment diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go index 4a4462b7407..a5204bf1b91 100644 --- a/nomad/job_endpoint_test.go +++ b/nomad/job_endpoint_test.go @@ -3528,6 +3528,9 @@ func TestJobEndpoint_Deployments(t *testing.T) { d1.JobID = j.ID d2.JobID = j.ID require.Nil(state.UpsertJob(1000, j), "UpsertJob") + d1.JobCreateIndex = j.CreateIndex + d2.JobCreateIndex = j.CreateIndex + require.Nil(state.UpsertDeployment(1001, d1), "UpsertDeployment") require.Nil(state.UpsertDeployment(1002, d2), "UpsertDeployment") @@ -3562,6 +3565,8 @@ func TestJobEndpoint_Deployments_ACL(t *testing.T) { d1.JobID = j.ID d2.JobID = j.ID require.Nil(state.UpsertJob(1000, j), "UpsertJob") + d1.JobCreateIndex = j.CreateIndex + d2.JobCreateIndex = j.CreateIndex require.Nil(state.UpsertDeployment(1001, d1), "UpsertDeployment") require.Nil(state.UpsertDeployment(1002, d2), "UpsertDeployment") @@ -3622,7 +3627,7 @@ func TestJobEndpoint_Deployments_Blocking(t *testing.T) { d2 := mock.Deployment() d2.JobID = j.ID require.Nil(state.UpsertJob(50, j), "UpsertJob") - + d2.JobCreateIndex = j.CreateIndex // First upsert an unrelated eval time.AfterFunc(100*time.Millisecond, func() { require.Nil(state.UpsertDeployment(100, d1), "UpsertDeployment") @@ -3671,6 +3676,8 @@ func TestJobEndpoint_LatestDeployment(t *testing.T) { d2.CreateIndex = d1.CreateIndex + 100 d2.ModifyIndex = d2.CreateIndex + 100 require.Nil(state.UpsertJob(1000, j), "UpsertJob") + d1.JobCreateIndex = j.CreateIndex + d2.JobCreateIndex = j.CreateIndex require.Nil(state.UpsertDeployment(1001, d1), "UpsertDeployment") require.Nil(state.UpsertDeployment(1002, d2), "UpsertDeployment") @@ -3708,6 +3715,8 @@ func TestJobEndpoint_LatestDeployment_ACL(t *testing.T) { d2.CreateIndex = d1.CreateIndex + 100 d2.ModifyIndex = d2.CreateIndex + 100 require.Nil(state.UpsertJob(1000, j), "UpsertJob") + d1.JobCreateIndex = j.CreateIndex + d2.JobCreateIndex = j.CreateIndex require.Nil(state.UpsertDeployment(1001, d1), "UpsertDeployment") require.Nil(state.UpsertDeployment(1002, d2), "UpsertDeployment") From ea37019d7f90f218ea616681841407e84d0e5b02 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Tue, 14 May 2019 16:13:41 -0500 Subject: [PATCH 4/7] Fix one more test set up --- command/agent/job_endpoint_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go index b99a5926b7a..a076a9b9d82 100644 --- a/command/agent/job_endpoint_test.go +++ b/command/agent/job_endpoint_test.go @@ -841,6 +841,7 @@ func TestHTTP_JobDeployment(t *testing.T) { state := s.Agent.server.State() d := mock.Deployment() d.JobID = j.ID + d.JobCreateIndex = resp.JobModifyIndex assert.Nil(state.UpsertDeployment(1000, d), "UpsertDeployment") // Make the HTTP request From fa7933645ebef1c498b08ced9055b399e8490de8 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Tue, 14 May 2019 16:13:59 -0500 Subject: [PATCH 5/7] docs --- website/source/api/jobs.html.md | 4 ++++ website/source/docs/commands/job/deployments.html.md.erb | 3 +++ 2 files changed, 7 insertions(+) diff --git a/website/source/api/jobs.html.md b/website/source/api/jobs.html.md index 79a07e98388..fc79cb74e9b 100644 --- a/website/source/api/jobs.html.md +++ b/website/source/api/jobs.html.md @@ -966,6 +966,10 @@ The table below shows this endpoint's support for - `:job_id` `(string: )` - Specifies the ID of the job (as specified in the job file during submission). This is specified as part of the path. +- `all` `(bool: false)` - Specifies whether the list of deployments should + include deployments from a previously registered job with the same ID. This is + possible if the job is deregistered and reregistered. + ### Sample Request ```text diff --git a/website/source/docs/commands/job/deployments.html.md.erb b/website/source/docs/commands/job/deployments.html.md.erb index 28451002a82..56f6ecae59e 100644 --- a/website/source/docs/commands/job/deployments.html.md.erb +++ b/website/source/docs/commands/job/deployments.html.md.erb @@ -34,6 +34,9 @@ of a job to display the list of deployments for. * `-verbose`: Show full information. +* `-all`: Display all deployments matching the job ID, even those from an + older instance of the job. + ## Examples List the deployment for a particular job: From f56c9cdf3eb721aebd54e45216d4fa52a75cd8a6 Mon Sep 17 00:00:00 2001 From: Preetha Date: Wed, 15 May 2019 21:11:52 -0500 Subject: [PATCH 6/7] remove stray newline Co-Authored-By: Danielle --- nomad/state/state_store_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/nomad/state/state_store_test.go b/nomad/state/state_store_test.go index e272050f794..12507b717e2 100644 --- a/nomad/state/state_store_test.go +++ b/nomad/state/state_store_test.go @@ -565,7 +565,6 @@ func TestStateStore_OldDeployment(t *testing.T) { require.Nil(err) require.Len(deploys, 1) require.Equal(deploy1.ID, deploys[0].ID) - } func TestStateStore_DeleteDeployment(t *testing.T) { From 9abd35285fa8d175191a285a35ad384204831d29 Mon Sep 17 00:00:00 2001 From: Preetha Appan Date: Wed, 15 May 2019 21:16:57 -0500 Subject: [PATCH 7/7] Add -all to help text and flags --- command/job_deployments.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/command/job_deployments.go b/command/job_deployments.go index 22f87e86a5d..291bab23aa3 100644 --- a/command/job_deployments.go +++ b/command/job_deployments.go @@ -35,6 +35,10 @@ Deployments Options: -verbose Display full information. + + -all-allocs + Display all deployments matching the job ID, including those + from an older instance of the job. ` return strings.TrimSpace(helpText) } @@ -50,6 +54,7 @@ func (c *JobDeploymentsCommand) AutocompleteFlags() complete.Flags { "-t": complete.PredictAnything, "-latest": complete.PredictNothing, "-verbose": complete.PredictNothing, + "-all": complete.PredictNothing, }) }