auto promote canaries #5719

langmartin · 2019-05-16T14:14:00Z

Add a new configuration option, auto_promote, that causes deployments with healthy canary allocations to be promoted automatically. This fixes #3636

website/source/api/json-jobs.html.md

schmichael

I think the core logic looks good! An e2e test would be nice or at least an integration test possibly in the nomad/ server package to test without so much mocking.

Great work!

Backwards compat

I think it's possible for HealthyCanaries to be permanently inaccurate if servers are upgraded mid-deploy. However those deploys could never have AutoPromote=true so I don't think it matters.

api/jobs.go

nomad/deploymentwatcher/deployment_watcher.go

nomad/structs/structs.go

nomad/deploymentwatcher/deployments_watcher_test.go

website/source/api/json-jobs.html.md

website/source/docs/job-specification/update.html.md

schmichael

Auto promote! E2E improvements! Typo fixes! So much to enjoy in one nice concise PR. My last 2 comments are blockers. Merge whenever you feel it's ready!

notnoop

This looks great! Love the tests.

Have a question about autopromotion with multi-taskgroup jobs and few other nitpicks.

e2e/deployment/deployment.go

e2e/deployment/input/deployment_auto0.nomad

e2e/e2eutil/utils.go

notnoop · 2019-05-21T14:45:06Z

CHANGELOG.md

@@ -30,6 +30,7 @@ IMPROVEMENTS:
 * core: Add `-verbose` flag to `nomad status` wrapper command [[GH-5516](https://github.com/hashicorp/nomad/pull/5516)]
 * core: Reduce the size of the raft transaction for plans by only sending fields updated by the plan applier [[GH-5602](https://github.com/hashicorp/nomad/pull/5602)]
 * core: Add ability to filter job deployments by most recent version of job [[GH-5702](https://github.com/hashicorp/nomad/issues/5702)]
+ * core: Add job update `auto_promote` flag, which causes deployments to promote themselves when all canaries become healthy [[GH-5719](https://github.com/hashicorp/nomad/pull/5719)]


I would suggest updating changelog after merging PR as the changelog is common cause of conflicts.

notnoop · 2019-05-21T14:48:41Z

nomad/deploymentwatcher/deployment_watcher.go

+TASKGROUP:
+	for tk, tv := range d.TaskGroups {
+		if !tv.AutoPromote || tv.DesiredCanaries != len(tv.PlacedCanaries) {
+			continue


Mind if you elaborate on auto promotion and multiple task groups? I would have expected auto promotion of a job to take place if all its task group were set for AutoPromote or if AutoPromote was a job level property.

auto_promote will only update the job if all the task groups are healthy and marked for auto_promote, it will otherwise stay in the pending state

notnoop

I have a question about job and task group update merging - but it's lgtm otherwise.

notnoop · 2019-05-22T12:23:43Z

api/jobs.go

@@ -433,6 +435,9 @@ func (u *UpdateStrategy) Copy() *UpdateStrategy {
 		copy.Canary = intToPtr(*u.Canary)
 	}

+	// boolPtrs need to preserve nil unset value
+	copy.AutoPromote = u.AutoPromote


I don't quite follow this comment and why the copying here is different?

I presume that the rest of assignments used boolToPtr to avoid having two structures share the same reference when deep copying the structure; but I don't know for sure and we don't really do reference manipulation here afaik.

good catch, I thought about that wrong. copy and merge should match the old behavior

notnoop · 2019-05-22T12:27:50Z

command/agent/job_endpoint.go

+			au.Merge(taskGroup.Update)
+		} else {
+			au = taskGroup.Update
+		}


I suspect the merging of jobUpdate might be a subtle change in behavior. How about use taskGroup.Update if it's not nil, otherwise use jobUpdate? Either way, we should note it in the docs/changelog/release notes.

I can go either way on this, the current behavior does not match what the docs claim: "If specified at the job level, the configuration will apply to all groups within the job. If multiple update stanzas are specified, they are merged with the group stanza taking the highest precedence and then the job" I think if it's safer to keep the existing behavior, we should change the docs, and if we keep this change we can leave them as is.

Co-Authored-By: Michael Schurter <[email protected]>

Co-Authored-By: Mahmood Ali <[email protected]>

notnoop

awesome!

github-actions · 2023-02-09T02:17:49Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

langmartin requested a review from schmichael May 16, 2019 14:14

preetapan reviewed May 16, 2019

View reviewed changes

website/source/api/json-jobs.html.md Show resolved Hide resolved

schmichael requested changes May 16, 2019

View reviewed changes

schmichael reviewed May 16, 2019

View reviewed changes

website/source/api/json-jobs.html.md Show resolved Hide resolved

langmartin force-pushed the f-auto-promote-canaries branch 3 times, most recently from c02910f to dbee31a Compare May 20, 2019 19:00

langmartin requested review from schmichael and notnoop May 20, 2019 19:00

schmichael reviewed May 20, 2019

View reviewed changes

website/source/docs/job-specification/update.html.md Outdated Show resolved Hide resolved

schmichael reviewed May 20, 2019

View reviewed changes

website/source/docs/job-specification/update.html.md Outdated Show resolved Hide resolved

schmichael approved these changes May 20, 2019

View reviewed changes

langmartin force-pushed the f-auto-promote-canaries branch from e966eb6 to 1ae24b3 Compare May 21, 2019 14:06

notnoop reviewed May 21, 2019

View reviewed changes

langmartin force-pushed the f-auto-promote-canaries branch from 41bb0d5 to 68a9503 Compare May 21, 2019 20:21

notnoop reviewed May 22, 2019

View reviewed changes

langmartin force-pushed the f-auto-promote-canaries branch from 0ee7235 to 6664e63 Compare May 22, 2019 15:05

langmartin added 13 commits May 22, 2019 12:32

new deploymentwatcher/doc.go for package level documentation

55b91ad

state_store typo in a comment

2f23357

deployments_watcher comments

1ba04fc

deployment_watcher when it's ok to autopromote, do so

2753b51

sched reconcile copy AutoPromote to DeploymentState

2165f8b

deployments_watcher_test new TestWatcher_AutoPromoteDeployment

0643a01

add update AutoPromote bool

9621c07

website update auto_promote doc

5ac416a

describe a pending deployment with auto_promote accurately

21dccdf

describe a pending deployment without auto_promote more explicitly

84307d9

e2e util split new alloc and await placement, new WaitForDeployment

8b64574

new e2e deployment test

2422991

e2e utils error format arg match

1a99a0f

langmartin and others added 19 commits May 22, 2019 12:32

api/jobs add AutoPromote to Canonicalize

03c69c8

log error on autoPromoteDeployment failure

2772cc2

structs validate requires Canary for AutoPromote

d867cbf

CHANGELOG add improvement entry

c009381

e2e README typo

78b9aef

e2e utils remove ineffectual assignment of allocs

3a3c442

api/jobs diff tests expect AutoPromote

7d40ffd

Update website/source/docs/job-specification/update.html.md

f4701e6

Co-Authored-By: Michael Schurter <[email protected]>

website more explicit auto_promote instructions

0da3ee5

e2e readme minor changes to command + env val templates and order

1e9f314

e2e add deployment to the list of e2e tests, minor fixes

dd2f16a

deployment_watcher auto promote iff every task group is auto promotable

28dbf73

api use job.update as the default for taskgroup.update

c086b5f

e2e/deployment comment the job files for clarity

a5e3e8e

Update e2e/deployment/deployment.go

a234550

Co-Authored-By: Mahmood Ali <[email protected]>

structs job warnings for taskgroup with mixed auto_promote settings

71338ab

structs comment todo DeploymentStatus & DeploymentStatusDescription

b09b0be

comment replace COMPAT 0.7.0 for job.Update with more current info

9f74b22

structs check TaskGroup.Update for nil

c9a837c

langmartin force-pushed the f-auto-promote-canaries branch from 6664e63 to c9a837c Compare May 22, 2019 16:35

notnoop approved these changes May 22, 2019

View reviewed changes

langmartin merged commit 643d840 into master May 22, 2019

langmartin deleted the f-auto-promote-canaries branch May 22, 2019 17:24

martinklepsch mentioned this pull request May 22, 2019

Simplify deployment code by using Nomad's new auto_promote cljdoc/cljdoc#300

Open

ghost mentioned this pull request Jun 14, 2019

Add support for nomad 0.9.3 update stanza "auto_promote" property hashicorp/terraform-provider-nomad#68

Closed

andyghiuta mentioned this pull request Oct 13, 2019

Update Nomad to 0.9.2+ hashicorp/levant#309

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auto promote canaries #5719

auto promote canaries #5719

langmartin commented May 16, 2019

schmichael left a comment

schmichael left a comment

notnoop left a comment

notnoop May 21, 2019

notnoop May 21, 2019

langmartin May 21, 2019

notnoop left a comment

notnoop May 22, 2019

langmartin May 22, 2019

notnoop May 22, 2019

langmartin May 22, 2019

notnoop left a comment

github-actions bot commented Feb 9, 2023

auto promote canaries #5719

auto promote canaries #5719

Conversation

langmartin commented May 16, 2019

schmichael left a comment

Choose a reason for hiding this comment

Backwards compat

schmichael left a comment

Choose a reason for hiding this comment

notnoop left a comment

Choose a reason for hiding this comment

notnoop May 21, 2019

Choose a reason for hiding this comment

notnoop May 21, 2019

Choose a reason for hiding this comment

langmartin May 21, 2019

Choose a reason for hiding this comment

notnoop left a comment

Choose a reason for hiding this comment

notnoop May 22, 2019

Choose a reason for hiding this comment

langmartin May 22, 2019

Choose a reason for hiding this comment

notnoop May 22, 2019

Choose a reason for hiding this comment

langmartin May 22, 2019

Choose a reason for hiding this comment

notnoop left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 9, 2023