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

Implemented NodeAffinity #695

Merged
merged 1 commit into from
Sep 24, 2021
Merged

Implemented NodeAffinity #695

merged 1 commit into from
Sep 24, 2021

Conversation

ellistarn
Copy link
Contributor

@ellistarn ellistarn commented Sep 21, 2021 •
edited
Loading

1. Issue, if available:
#482

2. Description of changes:

  • Taint and Label generation will no longer replace existing taints/labels defined on the provisioner (layered constraints vs overrides)
  • Preferences are currently respected as hard constraints (may be relaxed in the future)
  • [aws] Capacity Type is now Capacity Types, prioritize spot if able. This will default to on-demand if unspecified.

3. Does this change impact docs?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link

netlify bot commented Sep 21, 2021 •
edited
Loading

✔️ Deploy Preview for karpenter-docs-prod canceled.

🔨 Explore the source changes: 05cb0d2

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/614e1785f26f4200084eab85

@ellistarn ellistarn force-pushed the nodeaffinity branch 7 times, most recently from e398cb4 to 02d3fdf Compare September 22, 2021 23:56
@JacobGabrielson JacobGabrielson self-requested a review September 23, 2021 16:39
JacobGabrielson
JacobGabrielson reviewed Sep 23, 2021
View reviewed changes
Copy link
Contributor

@JacobGabrielson JacobGabrielson left a comment

Choose a reason for hiding this comment

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

Looking good so far... reviewed about 17/29 so far

pkg/cloudprovider/aws/cloudprovider.go Outdated Show resolved Hide resolved
pkg/cloudprovider/aws/cloudprovider.go Outdated Show resolved Hide resolved
pkg/cloudprovider/aws/instance.go Outdated Show resolved Hide resolved
JacobGabrielson
JacobGabrielson reviewed Sep 23, 2021
View reviewed changes
Copy link
Contributor

@JacobGabrielson JacobGabrielson left a comment

Choose a reason for hiding this comment

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

Looks good o'erall. A few minor comments.

pkg/controllers/allocation/scheduling/constraints.go Outdated Show resolved Hide resolved
pkg/controllers/allocation/scheduling/scheduler.go Show resolved Hide resolved
pkg/scheduling/nodeaffinity.go Outdated Show resolved Hide resolved
pkg/scheduling/nodeaffinity.go Outdated Show resolved Hide resolved
pkg/scheduling/taints.go Show resolved Hide resolved
pkg/utils/functional/functional.go
}
if slices[0] == nil {
return IntersectStringSlice(slices[1:]...)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not also check here for slices[0] to be an empty slice, and then just short-circuit everything else and return an empty slice, since it's like multiplying by zero?

pkg/utils/functional/suite_test.go Outdated Show resolved Hide resolved
pkg/utils/functional/suite_test.go Show resolved Hide resolved
pkg/utils/functional/suite_test.go Show resolved Hide resolved
pkg/utils/functional/suite_test.go Show resolved Hide resolved
@ellistarn ellistarn changed the title [WIP] Implemented NodeAffinity Implemented NodeAffinity Sep 23, 2021
@ellistarn ellistarn force-pushed the nodeaffinity branch 9 times, most recently from 28ab8de to ca8ffd9 Compare September 24, 2021 00:38
bwagner5
bwagner5 reviewed Sep 24, 2021
View reviewed changes
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

Looking good, I'm a little over half way through reviewing. But will have to look at the rest a bit later.

pkg/apis/provisioning/v1alpha4/register.go Show resolved Hide resolved
pkg/cloudprovider/aws/apis/v1alpha1/constraints_validation.go Outdated Show resolved Hide resolved
pkg/cloudprovider/aws/instance.go
// either unconstrained (nil), or a valid WellKnownLabel. Provisioner
// defaulting logic will currently default to [on-demand] if unspecifed.
capacityType := v1alpha1.CapacityTypeSpot
if capacityTypes != nil && !functional.ContainsString(capacityTypes, v1alpha1.CapacityTypeSpot) {
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting, I guess the ICE Cache could key on capacity-type too in order to support a native fallback to OD if we can't get spot capacity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah totally.

bwagner5
bwagner5 previously approved these changes Sep 24, 2021
View reviewed changes
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

A few small comments, but looks great!

pkg/controllers/allocation/scheduling/constraints.go
if pod.Spec.Affinity.PodAffinity != nil {
errs = multierr.Append(errs, fmt.Errorf("pod affinity is not supported"))
}
if pod.Spec.Affinity.PodAntiAffinity != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to fail hard like this? Systems like kops will deploy default components like coredns with PodAntiAffinity, so this effectively makes them a lot more difficult to get working. I can see the other side of it where you don't want users relying on PodAntiAffinity if it will not work too and don't want them to have notice a log message necessarily. I'm not sure what the right thing to do here is, wdut?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until we respect the constraints, we basically have to not schedule the pods. I can imagine eventually trying to implement pod affinity, but minimally it will be incremental effort over this. FWIW, the current behavior is to not provision if constraints are not supported.

pkg/controllers/allocation/scheduling/topology.go Outdated Show resolved Hide resolved
bwagner5
bwagner5 previously approved these changes Sep 24, 2021
View reviewed changes
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

lgtm

@ellistarn ellistarn dismissed stale reviews from JacobGabrielson and bwagner5 via 05cb0d2 September 24, 2021 18:22
bwagner5
bwagner5 approved these changes Sep 24, 2021
View reviewed changes
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

lgtm

@ellistarn ellistarn merged commit 6989466 into aws:main Sep 24, 2021
@ellistarn ellistarn deleted the nodeaffinity branch September 24, 2021 18:33
@joebowbeer joebowbeer mentioned this pull request Dec 8, 2021
Closed
@natherz97 natherz97 mentioned this pull request Jan 28, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwagner5 bwagner5 bwagner5 approved these changes

@JacobGabrielson JacobGabrielson JacobGabrielson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ellistarn @JacobGabrielson @bwagner5