Prevent pruning and reaping of TaggedVersion jobs #23983
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
philrenaud
force-pushed
the
23921-add-conditional-to-upsertjobversion-to-prevent-a-tagged-version-from-being-pruned
branch
from
5ede458
to
072d969
Compare
tgross
reviewed
Sep 17, 2024
philrenaud
force-pushed
the
23879-nomad-job-plan-version-flag
branch
2 times, most recently
from
2c59fa7
to
81b3e44
Compare
gulducat
force-pushed
the
23921-add-conditional-to-upsertjobversion-to-prevent-a-tagged-version-from-being-pruned
branch
2 times, most recently
from
99020fe
to
a8960b5
Compare
gulducat
force-pushed
the
23921-add-conditional-to-upsertjobversion-to-prevent-a-tagged-version-from-being-pruned
branch
from
a8960b5
to
63caddf
Compare
Ember Test Audit comparison
|
tgross
approved these changes
Sep 25, 2024
nomad/state/state_store_test.go
Outdated
Comment on lines
2946
to
2971
| // take a little detour to test error conditions with the setup so far | ||
| t.Run("errors", func(t *testing.T) { | ||
| // job does not exist | ||
| err := state.UpdateJobVersionTag(nextIndex(state), job.Namespace, &structs.JobApplyTagRequest{ | ||
| JobID: "non-existent-job", | ||
| Tag: &structs.JobTaggedVersion{Name: "tag name"}, | ||
| Version: 0, | ||
| }) | ||
| must.ErrorContains(t, err, `job "non-existent-job" version 0 not found`) | ||
|
|
||
| // version does not exist | ||
| err = state.UpdateJobVersionTag(nextIndex(state), job.Namespace, &structs.JobApplyTagRequest{ | ||
| JobID: job.ID, | ||
| Tag: &structs.JobTaggedVersion{Name: "tag name"}, | ||
| Version: 999, | ||
| }) | ||
| must.ErrorContains(t, err, fmt.Sprintf("job %q version 999 not found", job.ID)) | ||
|
|
||
| // tag name already exists | ||
| err = state.UpdateJobVersionTag(nextIndex(state), job.Namespace, &structs.JobApplyTagRequest{ | ||
| JobID: job.ID, | ||
| Tag: &structs.JobTaggedVersion{Name: "v0"}, | ||
| Version: 3, | ||
| }) | ||
| must.ErrorContains(t, err, fmt.Sprintf(`"v0" already exists on a different version of job %q`, job.ID)) | ||
| }) |
There was a problem hiding this comment.
It's hard to tell because the PRs for this feature have been running for a while now, but don't we already have test assertions for these state store methods in the PR that introduced them? Re-testing them doesn't seem useful.
There was a problem hiding this comment.
Yeah it's a bit tricky to hold it all in mind, but I don't believe any of the parent branches/PRs have any test coverage at this layer (I don't see state_store_test.go in other PRs), and when I started testing the "don't let tagged versions go away" logic, it felt natural to elaborate here since I'd already done the requisite setup.
nomad/state/state_store_test.go
Outdated
Comment on lines
2922
to
2944
| // tag versions 0-2 | ||
| for x := range 3 { | ||
| v := uint64(x) | ||
| name := fmt.Sprintf("v%d", x) | ||
| desc := fmt.Sprintf("version %d", x) | ||
|
|
||
| // apply tag | ||
| must.NoError(t, state.UpdateJobVersionTag(nextIndex(state), job.Namespace, &structs.JobApplyTagRequest{ | ||
| JobID: job.ID, | ||
| Name: name, | ||
| Tag: &structs.JobTaggedVersion{ | ||
| Name: name, | ||
| Description: desc, | ||
| }, | ||
| Version: v, | ||
| })) | ||
|
|
||
| // confirm | ||
| got, err := state.JobVersionByTagName(nil, job.Namespace, job.ID, name) | ||
| must.NoError(t, err) | ||
| must.Eq(t, name, got.TaggedVersion.Name) | ||
| must.Eq(t, desc, got.TaggedVersion.Description) | ||
| } |
There was a problem hiding this comment.
We're testing the cases where we've got a "full" tracked versions when we tag, but not when we don't. It might make more sense to insert < max tracked, tag one, insert the rest, and then tag again. That'll make sure we've covered both paths.
gulducat
deleted the
23921-add-conditional-to-upsertjobversion-to-prevent-a-tagged-version-from-being-pruned
branch
philrenaud
added a commit
that referenced
this pull request
Sep 25, 2024
…mmand (#23889) * First pass at passing a tagname and/or diff version to plan/versions requests * versions API now takes compare_to flags * Job history command output can have tag names and descriptions * compare_to to diff-tag and diff-version, plus adding flags to history command * 0th version now shows a diff if a specific diff target is requested * Addressing some PR comments * Simplify the diff-appending part of jobVersions and hide None-type diffs from CLI * Remove the diff-tag and diff-version parts of nomad job plan, with an eye toward making them a new top-level CLI command soon * Version diff tests * re-implement JobVersionByTagName * Test mods and simplification * Documentation for nomad job history additions * Prevent pruning and reaping of TaggedVersion jobs (#23983) tagged versions should not count against JobTrackedVersions i.e. new job versions being inserted should not evict tagged versions and GC should not delete a job if any of its versions are tagged Co-authored-by: Daniel Bennett <[email protected]> --------- Co-authored-by: Daniel Bennett <[email protected]>
philrenaud
added a commit
that referenced
this pull request
Sep 25, 2024
* Tag and Untag at API level on down, but am I unblocking the wrong thing? * Code and comment cleanup * Unset methods generally now I stare long into the namespace abyss * Namespace passes through with QueryOptions removed from a write requesting struct * Comment and PR review cleanup * Version back to VersionStr * Generally consolidate unset logic into apply for version tagging * Addressed some PR comments * Auth check and RPC forwarding * uint64 instead of pointer for job version after api layer and renamed copy * job tag command split into apply and unset * latest-version convenience handling moved to CLI command level * CLI tests for tagging/untagging * UI parts removed * Add to job table when unsetting job tag on latest version * Vestigial no more * Compare versions by name and version number with the nomad history command (#23889) * First pass at passing a tagname and/or diff version to plan/versions requests * versions API now takes compare_to flags * Job history command output can have tag names and descriptions * compare_to to diff-tag and diff-version, plus adding flags to history command * 0th version now shows a diff if a specific diff target is requested * Addressing some PR comments * Simplify the diff-appending part of jobVersions and hide None-type diffs from CLI * Remove the diff-tag and diff-version parts of nomad job plan, with an eye toward making them a new top-level CLI command soon * Version diff tests * re-implement JobVersionByTagName * Test mods and simplification * Documentation for nomad job history additions * Prevent pruning and reaping of TaggedVersion jobs (#23983) tagged versions should not count against JobTrackedVersions i.e. new job versions being inserted should not evict tagged versions and GC should not delete a job if any of its versions are tagged Co-authored-by: Daniel Bennett <[email protected]> --------- Co-authored-by: Daniel Bennett <[email protected]> * [ui] Version Tags on the job versions page (#24013) * Timeline styles and their buttons modernized, and tags added * styled but not yet functional version blocks * Rough pass at edit/unedit UX * Styles consolidated * better UX around version tag crud, plus adapter and serializers * Mirage and acceptance tests * Modify percy to not show time-based things --------- Co-authored-by: Daniel Bennett <[email protected]>
philrenaud
added a commit
that referenced
this pull request
Sep 25, 2024
* TaggedVersion information in structs, rather than job_endpoint (#23841) * TaggedVersion information in structs, rather than job_endpoint * Test for taggedVersion description length * Some API plumbing * Tag and Untag job versions (#23863) * Tag and Untag at API level on down, but am I unblocking the wrong thing? * Code and comment cleanup * Unset methods generally now I stare long into the namespace abyss * Namespace passes through with QueryOptions removed from a write requesting struct * Comment and PR review cleanup * Version back to VersionStr * Generally consolidate unset logic into apply for version tagging * Addressed some PR comments * Auth check and RPC forwarding * uint64 instead of pointer for job version after api layer and renamed copy * job tag command split into apply and unset * latest-version convenience handling moved to CLI command level * CLI tests for tagging/untagging * UI parts removed * Add to job table when unsetting job tag on latest version * Vestigial no more * Compare versions by name and version number with the nomad history command (#23889) * First pass at passing a tagname and/or diff version to plan/versions requests * versions API now takes compare_to flags * Job history command output can have tag names and descriptions * compare_to to diff-tag and diff-version, plus adding flags to history command * 0th version now shows a diff if a specific diff target is requested * Addressing some PR comments * Simplify the diff-appending part of jobVersions and hide None-type diffs from CLI * Remove the diff-tag and diff-version parts of nomad job plan, with an eye toward making them a new top-level CLI command soon * Version diff tests * re-implement JobVersionByTagName * Test mods and simplification * Documentation for nomad job history additions * Prevent pruning and reaping of TaggedVersion jobs (#23983) tagged versions should not count against JobTrackedVersions i.e. new job versions being inserted should not evict tagged versions and GC should not delete a job if any of its versions are tagged Co-authored-by: Daniel Bennett <[email protected]> --------- Co-authored-by: Daniel Bennett <[email protected]> * [ui] Version Tags on the job versions page (#24013) * Timeline styles and their buttons modernized, and tags added * styled but not yet functional version blocks * Rough pass at edit/unedit UX * Styles consolidated * better UX around version tag crud, plus adapter and serializers * Mirage and acceptance tests * Modify percy to not show time-based things --------- Co-authored-by: Daniel Bennett <[email protected]> * Job revert command and API endpoint can take a string version tag name (#24059) * Job revert command and API endpoint can take a string version tag name * RevertOpts as a signature-modified alternative to Revert() * job revert CLI test * Version pointers in endpoint tests * Dont copy over the tag when a job is reverted to a version with a tag * Convert tag name to version number at CLI level * Client method for version lookup by tag * No longer double-declaring client * [ui] Add tag filter to the job versions page (#24064) * Rough pass at the UI for version diff dropdown * Cleanup and diff fetching via adapter method * TaggedVersion now VersionTag (#24066) --------- Co-authored-by: Daniel Bennett <[email protected]>
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.
Hi this is totally Phil and not Daniel (ok it is Daniel)
This protects tagged versions of jobs from being bumped off as new versions are created, and prevents jobs that have any tagged versions from being GC'd.