Prevent kill_timeout greater than progress_deadline #16761
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Juanadelacuesta
force-pushed
the
b-gh-8487
branch
2 times, most recently
from
af0c8b2
to
e0dfa06
Compare
jrasell
requested changes
Apr 3, 2023
There was a problem hiding this comment.
Looking good, thanks @Juanadelacuesta! It looks like the new validation has meant some tests are now breaking, which need to be fixed up. Lets also add a changelog entry to this PR.
jrasell
requested changes
Apr 4, 2023
nomad/structs/structs.go
Outdated
| @@ -5136,7 +5144,7 @@ func (u *UpdateStrategy) IsEmpty() bool { | |||
| return true | |||
| } | |||
|
|
|||
| return u.MaxParallel == 0 | |||
| return *u == *DefaultUpdateStrategy | |||
There was a problem hiding this comment.
Isn't this doing an equality check rather than checking if the strategy is empty?
nomad/structs/structs.go
Outdated
| @@ -5145,6 +5153,38 @@ func (u *UpdateStrategy) Rolling() bool { | |||
| return u.Stagger > 0 && u.MaxParallel > 0 | |||
| } | |||
|
|
|||
| func (u *UpdateStrategy) Canonicalize() { | |||
There was a problem hiding this comment.
I worry that having this new functionality will mean users upgrading will see their job specification change between versions. I wonder why we need this, when the canonicalization should be handled by before it reaches the RPC.
I took a look at the use of structs.DefaultUpdateStrategy and it seems this only currently gets used by mocks and tests, which suggests we currently rely on the API default instantiation as mentioned above.
.changelog/16761.txt
Outdated
| @@ -0,0 +1,3 @@ | |||
| ```release-note:improvement | |||
| code: Prevent Kill Timeout greater than Progress Deadline on Update block | |||
There was a problem hiding this comment.
code or core? It'd be nice if the parameters were detailed in their jobspec form, as it makes it easier for users to grok.
Juanadelacuesta
force-pushed
the
b-gh-8487
branch
from
ad6cd2f
to
4052150
Compare
Juanadelacuesta
force-pushed
the
b-gh-8487
branch
from
73845bc
to
af22534
Compare
jrasell
approved these changes
Apr 4, 2023
Co-authored-by: James Rasell <jrasell@users.noreply.github.com>
Co-authored-by: James Rasell <jrasell@users.noreply.github.com>
Juanadelacuesta
added
backport/1.3.x
jrasell
added a commit
that referenced
this pull request
Apr 5, 2023
* func: add validation for kill timeout smaller than progress dealine * style: add changelog * style: typo in changelog * style: remove refactored test * Update .changelog/16761.txt Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/structs/structs.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> --------- Co-authored-by: James Rasell <jrasell@users.noreply.github.com>
Juanadelacuesta
added a commit
that referenced
this pull request
Apr 6, 2023
* func: add validation for kill timeout smaller than progress dealine * style: add changelog * style: typo in changelog * style: remove refactored test * Update .changelog/16761.txt * Update nomad/structs/structs.go --------- Co-authored-by: Juana De La Cuesta <juanita.delacuestamorales@hashicorp.com> Co-authored-by: James Rasell <jrasell@users.noreply.github.com>
lgfa29
added
backport/1.4.x
lgfa29
pushed a commit
that referenced
this pull request
May 16, 2023
* func: add validation for kill timeout smaller than progress dealine * style: add changelog * style: typo in changelog * style: remove refactored test * Update .changelog/16761.txt Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/structs/structs.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> --------- Co-authored-by: James Rasell <jrasell@users.noreply.github.com>
lgfa29
added
backport/1.5.x
lgfa29
added a commit
that referenced
this pull request
May 16, 2023
…release/1.4.x (#17205) * test: fix TestJobEndpoint_Scale_BatchJob * Prevent kill_timeout greater than progress_deadline (#16761) * func: add validation for kill timeout smaller than progress dealine * style: add changelog * style: typo in changelog * style: remove refactored test * Update .changelog/16761.txt Co-authored-by: James Rasell <jrasell@users.noreply.github.com> * Update nomad/structs/structs.go Co-authored-by: James Rasell <jrasell@users.noreply.github.com> --------- Co-authored-by: James Rasell <jrasell@users.noreply.github.com> --------- Co-authored-by: Luiz Aoqui <luiz@hashicorp.com> Co-authored-by: Juana De La Cuesta <juanita.delacuestamorales@hashicorp.com> Co-authored-by: James Rasell <jrasell@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the bug reported on #8487
If a kill_timeout is greater than the job's progress_deadline, allocations may keep running (after initial kill signal) for long enough that the deploy fails. However, the allocations that were scheduled by that job will still be pending after job failure, and will be placed and started once the previous allocations do exit. This PR adds a validation to avoid this situation.