Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check that JobPlanResponse Diff Type is None before checking for changes on getExitCode #14492

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

gabivlj
Copy link
Contributor

@gabivlj gabivlj commented Sep 8, 2022

Hi everyone!

  • This PR is strongly related to nomad plan for periodic tasks #2012, where if you:
    nomad plan ...
    and there are new allocations it should give an exit status code of 1.
  • However if you do:
    nomad run ...
    and then:
    nomad plan ...
    It still returns an exit status code of 1.

These changes prevent that from happening, as if there wasn't any edits to the deployment file resp.Diff.Type is 'None', and it gives an exit status code of 0.
I'm wondering if this is the right solution for this problem or we are searching for a better one.

@hashicorp-cla
Copy link

hashicorp-cla commented Sep 8, 2022

CLA assistant check
All committers have signed the CLA.

@th0m
Copy link
Contributor

th0m commented Sep 12, 2022

@tgross unless we have missed something (which we might have 😁) this seems to be a straightforward way of fixing #2012

@th0m
Copy link
Contributor

th0m commented Sep 21, 2022

@jrasell do you think you could have a look at this one? It should be relatively quick to review. Thank you

@tgross tgross self-requested a review October 3, 2022 20:01
@tgross tgross added the type/bug label Oct 3, 2022
@tgross tgross self-assigned this Oct 3, 2022
@tgross tgross added theme/batch Issues related to batch jobs and scheduling theme/cli labels Oct 3, 2022
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @gabivlj!

test
$ nomad job plan ./jobs/periodic.nomad
+ Job: "example"
+ Task Group: "group" (1 create)
  + Task: "task" (forces create)

Scheduler dry-run:
- All tasks successfully allocated.
- If submitted now, next periodic launch would be at 2022-10-03T21:30:00-07:00 (8h11m20s from now).

Job Modify Index: 0
To submit the job with version verification run:

nomad job run -check-index 0 ./jobs/periodic.nomad

When running the job with the check-index flag, the job will only be run if the
job modify index given matches the server-side version. If the index has
changed, another user has modified the job and the plan's results are
potentially invalid.

$ echo $?
1

$ nomad job run ./jobs/periodic.nomad
Job registration successful

Approximate next launch time: 2022-10-03T21:30:00-07:00 (8h11m15s from now)

$ nomad job plan ./jobs/periodic.nomad
Job: "example"
Task Group: "group" (1 create)
  Task: "task"

Scheduler dry-run:
- All tasks successfully allocated.
- If submitted now, next periodic launch would be at 2022-10-03T21:30:00-07:00 (8h11m12s from now).

Job Modify Index: 11
To submit the job with version verification run:

nomad job run -check-index 11 ./jobs/periodic.nomad

When running the job with the check-index flag, the job will only be run if the
job modify index given matches the server-side version. If the index has
changed, another user has modified the job and the plan's results are
potentially invalid.

$ echo $?
0

This just missed the merge window for Nomad 1.4.0, but I'll make sure it gets merged once that goes out and it'll land in the first regular release of Nomad 1.4.x.

In the meantime, if you want you can add a changelog file to this PR. It'll be at .changelog/14492.txt and have the format you can see in .changelog/14749.txt. For content, probably something like:

cli: Fixed a bug where plans for periodic jobs would return exit code 1 when the job was already register

(If you don't get to it before I'm ready to merge this, no worries. I see the PR has been open a little bit and it'll only take me a few minutes to add it.)

@tgross tgross added this to the 1.4.x milestone Oct 3, 2022
@tgross
Copy link
Member

tgross commented Oct 6, 2022

Hi @gabivlj! We had to push out a quick Nomad 1.4.1 patch, but this has now been merged and will go out in the next regular patch release! Thanks again!

@github-actions
Copy link

github-actions bot commented Feb 4, 2023

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
theme/batch Issues related to batch jobs and scheduling theme/cli type/bug
Projects
Development

Successfully merging this pull request may close these issues.

4 participants