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.

Sign up for GitHub

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

Jump to bottom

System and sysbatch jobs always have zero index #16030

Merged
merged 6 commits into from
Feb 2, 2023
Merged

System and sysbatch jobs always have zero index #16030

merged 6 commits into from
Feb 2, 2023

Conversation

tgross
Copy link
Member

@tgross tgross commented Feb 2, 2023 •
edited
Loading

System jobs do not have unique allocation Names because the index is intended to indicated the instance out of a desired count size. Because system jobs do not have an explicit count but the results are based on the targeted nodes, the index is less informative and this was intentionally omitted from the original design. (See #1851). This behavior has confused a lot of users and Nomad engineers, including me!

This changeset includes:

  • Updating the documentation to make this existing behavior clear to users.
  • Added a test condition to the scheduler tests so that the behavior is documented in code.
  • Fixed a bug where per_alloc volumes should not have been allowed for system and sysbatch jobs.

Note that changing this behavior rather than merely documenting it is likely to be a huge lift and not practical in the short term. We'll need to backport this but I'm going to open that as a separate PR against release/1.4.x and backport from there because the current per_alloc code includes a new-in-1.5.0 feature.

tgross added 3 commits February 2, 2023 11:35
@tgross
scheduler: tests demonstrating expected behavior for alloc names
1fb28b5 
Service jobs should have unique allocation Names, derived from the
Job.ID. System jobs do not have unique allocation Names because the index is
intended to indicated the instance out of a desired count size. Because system
jobs do not have an explicit count but the results are based on the targeted
nodes, the index is less informative and this was intentionally omitted from the
original design.
@tgross
docs: make it clear that NOMAD_ALLOC_INDEX is always zero for system/…
cf1b8ce 
…sysbatch jobs
@tgross
volumes: per_alloc is incompatible with system/sysbatch jobs
e6e0874 
System and sysbatch jobs always have a `NOMAD_ALLOC_INDEX` of 0. So
interpolation via `per_alloc` will not work as soon as there's more than one
allocation placed. Validate against this on job submission.
@tgross tgross added theme/docs Documentation issues and enhancements type/bug theme/storage labels Feb 2, 2023
@tgross tgross added this to the 1.5.0 milestone Feb 2, 2023
@tgross tgross requested review from lgfa29 and shoenig February 2, 2023 17:33
shoenig
shoenig approved these changes Feb 2, 2023
View reviewed changes
Copy link
Contributor

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

LGTM! just the code golf suggestion

scheduler/scheduler_system_test.go Outdated Show resolved Hide resolved
scheduler/generic_sched_test.go Outdated Show resolved Hide resolved
@tgross
Copy link
Member Author

tgross commented Feb 2, 2023 •
edited
Loading

I'm going to pick up these suggestions @shoenig but I'm also realizing that the code that creates the names is incredibly misleading, because it's never called for service jobs but suggests that it is (there are even tests for it!). I'm going to fix this up here as well.

@tgross
Copy link
Member Author

tgross commented Feb 2, 2023

I'm going to go ahead with merging this without touching that test code after all. This PR covers the problem with the alloc index. But when I started messing with the test code that uses a service job to test the system diffs (oops!) it breaks in weird ways that seem unrelated to this bug entirely.

@tgross tgross merged commit ba20138 into main Feb 2, 2023
@tgross tgross deleted the system-alloc-index branch February 2, 2023 21:18
@tgross tgross mentioned this pull request Feb 2, 2023
Merged
tgross added a commit that referenced this pull request Feb 2, 2023
@tgross
System and sysbatch jobs always have zero index (#16030)
bf372ae 
Service jobs should have unique allocation Names, derived from the
Job.ID. System jobs do not have unique allocation Names because the index is
intended to indicated the instance out of a desired count size. Because system
jobs do not have an explicit count but the results are based on the targeted
nodes, the index is less informative and this was intentionally omitted from the
original design.

Update docs to make it clear that NOMAD_ALLOC_INDEX is always zero for
system/sysbatch jobs

Validate that `volume.per_alloc` is incompatible with system/sysbatch jobs.
System and sysbatch jobs always have a `NOMAD_ALLOC_INDEX` of 0. So
interpolation via `per_alloc` will not work as soon as there's more than one
allocation placed. Validate against this on job submission.
tgross added a commit that referenced this pull request Feb 2, 2023
@tgross
System and sysbatch jobs always have zero index (#16030)
ae3781e 
Service jobs should have unique allocation Names, derived from the
Job.ID. System jobs do not have unique allocation Names because the index is
intended to indicated the instance out of a desired count size. Because system
jobs do not have an explicit count but the results are based on the targeted
nodes, the index is less informative and this was intentionally omitted from the
original design.

Update docs to make it clear that NOMAD_ALLOC_INDEX is always zero for
system/sysbatch jobs

Validate that `volume.per_alloc` is incompatible with system/sysbatch jobs.
System and sysbatch jobs always have a `NOMAD_ALLOC_INDEX` of 0. So
interpolation via `per_alloc` will not work as soon as there's more than one
allocation placed. Validate against this on job submission.
@tgross tgross mentioned this pull request Feb 2, 2023
Merged
@tgross tgross mentioned this pull request Feb 2, 2023
Merged
tgross added a commit that referenced this pull request Feb 2, 2023
@tgross
System and sysbatch jobs always have zero index (#16030)
2ac2e76 
Service jobs should have unique allocation Names, derived from the
Job.ID. System jobs do not have unique allocation Names because the index is
intended to indicated the instance out of a desired count size. Because system
jobs do not have an explicit count but the results are based on the targeted
nodes, the index is less informative and this was intentionally omitted from the
original design.

Update docs to make it clear that NOMAD_ALLOC_INDEX is always zero for
system/sysbatch jobs

Validate that `volume.per_alloc` is incompatible with system/sysbatch jobs.
System and sysbatch jobs always have a `NOMAD_ALLOC_INDEX` of 0. So
interpolation via `per_alloc` will not work as soon as there's more than one
allocation placed. Validate against this on job submission.
tgross added a commit that referenced this pull request Feb 3, 2023
@tgross
System and sysbatch jobs always have zero index (#16030)
7d04689 
Service jobs should have unique allocation Names, derived from the
Job.ID. System jobs do not have unique allocation Names because the index is
intended to indicated the instance out of a desired count size. Because system
jobs do not have an explicit count but the results are based on the targeted
nodes, the index is less informative and this was intentionally omitted from the
original design.

Update docs to make it clear that NOMAD_ALLOC_INDEX is always zero for
system/sysbatch jobs

Validate that `volume.per_alloc` is incompatible with system/sysbatch jobs.
System and sysbatch jobs always have a `NOMAD_ALLOC_INDEX` of 0. So
interpolation via `per_alloc` will not work as soon as there's more than one
allocation placed. Validate against this on job submission.
tgross added a commit that referenced this pull request Feb 3, 2023
@tgross
System and sysbatch jobs always have zero index (#16030) (#16043)
587e080 
Service jobs should have unique allocation Names, derived from the
Job.ID. System jobs do not have unique allocation Names because the index is
intended to indicated the instance out of a desired count size. Because system
jobs do not have an explicit count but the results are based on the targeted
nodes, the index is less informative and this was intentionally omitted from the
original design.

Update docs to make it clear that NOMAD_ALLOC_INDEX is always zero for
system/sysbatch jobs

Validate that `volume.per_alloc` is incompatible with system/sysbatch jobs.
System and sysbatch jobs always have a `NOMAD_ALLOC_INDEX` of 0. So
interpolation via `per_alloc` will not work as soon as there's more than one
allocation placed. Validate against this on job submission.
tgross added a commit that referenced this pull request Feb 3, 2023
@tgross
System and sysbatch jobs always have zero index (#16030) (#16041)
38e1ba1 
Service jobs should have unique allocation Names, derived from the
Job.ID. System jobs do not have unique allocation Names because the index is
intended to indicated the instance out of a desired count size. Because system
jobs do not have an explicit count but the results are based on the targeted
nodes, the index is less informative and this was intentionally omitted from the
original design.

Update docs to make it clear that NOMAD_ALLOC_INDEX is always zero for
system/sysbatch jobs

Validate that `volume.per_alloc` is incompatible with system/sysbatch jobs.
System and sysbatch jobs always have a `NOMAD_ALLOC_INDEX` of 0. So
interpolation via `per_alloc` will not work as soon as there's more than one
allocation placed. Validate against this on job submission.
tgross added a commit that referenced this pull request Feb 3, 2023
@tgross
System and sysbatch jobs always have zero index (#16030) (#16044)
fdd37d1 
Service jobs should have unique allocation Names, derived from the
Job.ID. System jobs do not have unique allocation Names because the index is
intended to indicated the instance out of a desired count size. Because system
jobs do not have an explicit count but the results are based on the targeted
nodes, the index is less informative and this was intentionally omitted from the
original design.

Update docs to make it clear that NOMAD_ALLOC_INDEX is always zero for
system/sysbatch jobs

Validate that `volume.per_alloc` is incompatible with system/sysbatch jobs.
System and sysbatch jobs always have a `NOMAD_ALLOC_INDEX` of 0. So
interpolation via `per_alloc` will not work as soon as there's more than one
allocation placed. Validate against this on job submission.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shoenig shoenig shoenig approved these changes

@lgfa29 lgfa29 lgfa29 approved these changes

Assignees
No one assigned
Labels
theme/docs Documentation issues and enhancements theme/storage type/bug
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@tgross @shoenig @lgfa29