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

Distinct Property supports arbitrary limit #2942

Merged
merged 2 commits into from
Aug 1, 2017
Merged

Distinct Property supports arbitrary limit #2942

merged 2 commits into from
Aug 1, 2017

Conversation

dadgar
Copy link
Contributor

@dadgar dadgar commented Jul 31, 2017

This PR enhances the distinct_property constraint such that a limit can be
specified in the RTarget/value parameter. This allows constraints such as:

 distinct_property = "${meta.rack}"
 value = "2"
}

This restricts any given rack from running more than 2 allocations from the
task group.

This PR also adds better validation of the constraints.

Fixes #1146

This PR enhances the distinct_property constraint such that a limit can
be specified in the RTarget/value parameter. This allows constraints
such as:

```
constraint {
  distinct_property = "${meta.rack}"
  value = "2"
}
```

This restricts any given rack from running more than 2 allocations from
the task group.

Fixes #1146
@dadgar dadgar force-pushed the f-distinct-count branch from 26ea7c1 to 4e71ba2 Compare July 31, 2017 23:52
p.populateExisting(constraint)

// Populate the proposed when setting the constraint. We do this because
// when detecting if we can inplace update an allocation we stage an
// eviction and then select. This means the plan has an eviction before a
// single select has finished.
p.PopulateProposed()

Copy link
Member

Choose a reason for hiding this comment

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

nit: remove

continue
}

return false, fmt.Sprintf("distinct_property: %s=%s already used", p.constraint.LTarget, nValue)
// Don't clear below 0.
combinedUse[propertyValue] = helper.Uint64Max(0, combined-clearedCount)
Copy link
Member

Choose a reason for hiding this comment

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

Since you're using uint if combined < clearedCount, the value will wrap and be extremely large instead of getting clamped to 0.

I think this should be:

if combined >= clearedCount {
  combinedUse[propertyValue] = combined-clearedCount
} else {
  combinedUse[propertyValue] = 0
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

@jippi
Copy link
Contributor

jippi commented Aug 1, 2017

Will this allow me to run a system job, and say "but only 3 allocations per AWS AZ" ?

@dadgar
Copy link
Contributor Author

dadgar commented Aug 1, 2017

@jippi System job is really intended to run on all machines, so it is somewhat abusive. I would do one of two things:

  • Run a service job per AZ with count = 3.
  • Run a service job targeting all the AZs and set the count = (# of AZs * 3) and then use this distinct_property with value = 3.

@dadgar dadgar merged commit 2e7d8ad into master Aug 1, 2017
@dadgar dadgar deleted the f-distinct-count branch August 1, 2017 21:19
@ygersie
Copy link
Contributor

ygersie commented Aug 26, 2017

I think this commit was the last piece to get similar functionality as this PR #2168
right?

@dadgar
Copy link
Contributor Author

dadgar commented Aug 29, 2017

@dropje86 Sorry they are actually slightly different as this is a limit and that is a balance.

@github-actions
Copy link

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 Mar 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants