Makes auto reverts robust against previously stable job that fail to start correctly upon reverting #3496
Conversation
…ting. This prevents infinite reverting when reverting to a job version that was previously stable, but not so after attempting a revert.
…ack failed status
preetapan
changed the title
nomad/structs/structs.go
Outdated
| // DeploymentStatusDescriptionRollbackFailed is used to get the status description of | ||
| // a deployment when rolling back to an older job is not possible because it has the same specification | ||
| func DeploymentStatusDescriptionRollbackFailed(baseDescription string, jobVersion uint64, jobVersionOld uint64) string { | ||
| return fmt.Sprintf("%s - rolling back to job version %d failed because it had the same specification as job version %d ", baseDescription, jobVersion, jobVersionOld) |
There was a problem hiding this comment.
" - not rolling back to stable job version %d as current job has same specification"? If we can get across the same message in less the better. Also want to avoid saying the roll back failed since it didn't, we just won't do it.
There was a problem hiding this comment.
yeah I am not a fan of my wording, I like your suggestion, will fix
| } else { | ||
| desc = structs.DeploymentStatusDescriptionRollbackFailed(desc, j.Version, w.j.Version) | ||
| j = nil | ||
| } |
There was a problem hiding this comment.
Need to do the same on SetAllocHealth and FailDeployment
| @@ -371,7 +371,13 @@ func (w *deploymentWatcher) watch() { | |||
| // Description should include that the job is being rolled back to | |||
| // version N | |||
| if j != nil { | |||
| desc = structs.DeploymentStatusDescriptionRollback(desc, j.Version) | |||
| // only revert if job being changed has a different spec | |||
There was a problem hiding this comment.
- Capitalize the start of your comments.
- Can we add more detail in the comment explaining how this can avoid an infinite revert cycle.
| }, func(err error) { | ||
| t.Fatal(err) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Don't we need to assert the job version hasn't changed?
…, and other code review comments.
…e to identical spec
…c to AllocHealth and DeploymentStatus endpoints.
| @@ -184,6 +184,18 @@ func (w *deploymentWatcher) SetAllocHealth( | |||
| w.setLatestEval(index) | |||
| return nil | |||
| } | |||
| func (w *deploymentWatcher) handleRollbackValidity(j *structs.Job, desc string) (*structs.Job, string) { | |||
There was a problem hiding this comment.
Comment on the inputs and outputs
|
|
||
| // verify that the job version hasn't changed after upsert | ||
| m.state.JobByID(nil, structs.DefaultNamespace, j.ID) | ||
| if j.Version != 0 { |
preetapan
force-pushed
the
b-auto-revert-stable
branch
from
0c33cc8
to
04feeef
Compare
|
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. |
No description provided.