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

quotas: refactor storage limit specification #24785

Merged
merged 3 commits into from
Jan 13, 2025
Merged

Conversation

tgross
Copy link
Member

@tgross tgross commented Jan 6, 2025

In anticipation of having quotas for dynamic host volumes, we want the user experience of the storage limits to feel integrated with the other resource limits. This is currently prevented by reusing the Resources type instead of having a specific type for QuotaResources.

Update the quota limit/usage types to use a QuotaResources that includes a new storage resources quota block. The wire format for the two types are compatible such that we can migrate the existing variables limit in the FSM.

Also fixes improper parallelism in the quota init test where we change working directory to avoid file write conflicts but this breaks when multiple tests are executed in the same process.

Ref: https://github.com/hashicorp/nomad-enterprise/pull/2096

Testing & Reproduction steps

Upgrade testing (from the ENT version of this PR):

  • Created a 3 node cluster on Nomad 1.9.3+ent with a quota that had a variables limit. Applied the quota to a namespace and created variables in that namespace. Did a stepwise upgrade to this branch and verified that the quota spec and quota usage now shows the new storage block.
  • Created a 3 node cluster on Nomad 1.9.3+ent with a quota that had a variables limit. Applied the quota to a namespace and created variables in that namespace. Took a raft snapshot. Stopped all servers and wiped their data dirs. Started the servers back up on this branch and restore the snapshot. Verified that the quota spec and quota usage now shows the new storage block.

Contributor Checklist

  • Changelog Entry If this PR changes user-facing behavior, please generate and add a
    changelog entry using the make cl command.
  • Testing Please add tests to cover any new functionality or to demonstrate bug fixes and
    ensure regressions will be caught.
  • Documentation If the change impacts user-facing functionality such as the CLI, API, UI,
    and job configuration, please update the Nomad website documentation to reflect this. Refer to
    the website README for docs guidelines. Please also consider whether the
    change requires notes within the upgrade guide.

Reviewer Checklist

  • Backport Labels Please add the correct backport labels as described by the internal
    backporting document.
  • Commit Type Ensure the correct merge method is selected which should be "squash and merge"
    in the majority of situations. The main exceptions are long-lived feature branches or merges where
    history should be preserved.
  • Enterprise PRs If this is an enterprise only PR, please add any required changelog entry
    within the public repository.

aimeeu
aimeeu previously approved these changes Jan 6, 2025
tgross added 2 commits January 7, 2025 11:42
In anticipation of having quotas for dynamic host volumes, we want the user
experience of the storage limits to feel integrated with the other resource
limits. This is currently prevented by reusing the `Resources` type instead of
having a specific type for `QuotaResources`.

Update the quota limit/usage types to use a `QuotaResources` that includes a new
storage resources quota block. The wire format for the two types are compatible
such that we can migrate the existing variables limit in the FSM.

Also fixes improper parallelism in the quota init test where we change working
directory to avoid file write conflicts but this breaks when multiple tests are
executed in the same process.

Ref: hashicorp/nomad-enterprise#2096
aimeeu
aimeeu previously approved these changes Jan 8, 2025
gulducat
gulducat previously approved these changes Jan 10, 2025
Copy link
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

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

I quibble with the appropriate usage of "in lieu" but otherwise, LGTM!

.changelog/24785.txt Outdated Show resolved Hide resolved
Comment on lines +317 to +320
case 1:
default:
return nil, errors.New("only one storage block is allowed")
}
Copy link
Member

Choose a reason for hiding this comment

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

this still feels funny when we could avoid the concern, and the extra parsing, by using a map instead of a block, but I suppose consistency with the surrounding context wins out.

@tgross tgross dismissed stale reviews from gulducat and aimeeu via 72969f1 January 10, 2025 18:27
@tgross tgross merged commit 3a11a0b into main Jan 13, 2025
32 checks passed
@tgross tgross deleted the quotas-for-storage-ce branch January 13, 2025 14:25
tgross added a commit to hashicorp/terraform-provider-nomad that referenced this pull request Jan 13, 2025
Updates the Nomad API package for the revisions to quotas in
hashicorp/nomad#24785. This also brings in the dynamic
host volumes API, which we'll need for 1.10.
tgross added a commit to hashicorp/terraform-provider-nomad that referenced this pull request Jan 15, 2025
Updates the Nomad API package for the revisions to quotas in
hashicorp/nomad#24785. This also brings in the dynamic
host volumes API, which we'll need for 1.10.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants