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

dynamic host volumes: create/register RPC validation #24482

Open
wants to merge 3 commits into
base: dynamic-host-volumes
Choose a base branch
from

Conversation

tgross
Copy link
Member

@tgross tgross commented Nov 18, 2024

Add several validation steps in the create/register RPCs for dynamic host volumes. We first check that submitted volumes are self-consistent (ex. max capacity is more than min capacity), then that any updates we've made are valid. And we validate against state: preventing claimed volumes from being updated and preventing placement requests for nodes that don't exist.

Ref: #15489

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.

looks great!

nomad/host_volume_endpoint.go Outdated Show resolved Hide resolved
nomad/structs/host_volumes.go Outdated Show resolved Hide resolved
nomad/structs/host_volumes.go Outdated Show resolved Hide resolved
nomad/structs/host_volumes.go Outdated Show resolved Hide resolved
The `HostVolumeByID` state store method didn't add a watch channel to the
watchset, which meant that it would never unblock. The tests missed this because
they were racy, so move the updates for unblocking tests into a `time.After`
call to ensure the queries are blocked before the update happens.
Add several validation steps in the create/register RPCs for dynamic host
volumes. We first check that submitted volumes are self-consistent (ex. max
capacity is more than min capacity), then that any updates we've made are
valid. And we validate against state: preventing claimed volumes from being
updated and preventing placement requests for nodes that don't exist.

Ref: #15489
Comment on lines +509 to +513
func FlattenMultierror(err error) error {
if err == nil {
return nil
}
mErr := err.(*multierror.Error)
Copy link
Member

Choose a reason for hiding this comment

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

I would conditionally type cast this, so a standard error could be passed in, and just returned right away. then the extra if mErr == nil check below seems superfluous given this new nil check up top, eh?

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.

2 participants