Skip to content

Support for NodeSelector and Tolerations.#3467

Closed
markusthoemmes wants to merge 1 commit intoknative:masterfrom
markusthoemmes:rev-scheduling
Closed

Support for NodeSelector and Tolerations.#3467
markusthoemmes wants to merge 1 commit intoknative:masterfrom
markusthoemmes:rev-scheduling

Conversation

@markusthoemmes
Copy link
Contributor

@markusthoemmes markusthoemmes commented Mar 20, 2019

Fixes #1816
Closes #1831

Took this over from @krancour to rebase to master and push it through.

Proposed Changes

  • Support for nodeSelector and tolerations in revision spec

Release Note

Support for nodeSelector and tolerations in revision spec

@googlebot
Copy link

googlebot commented Mar 20, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers labels Mar 20, 2019
knative-prow-robot
knative-prow-robot reviewed Mar 20, 2019
View reviewed changes
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@markusthoemmes: 0 warnings.

Details

In response to this:

Fixes #1816
Closes #1831

Took this over from @krancour to rebase to master and push it through.

Proposed Changes

  • Support for nodeSelector and tolerations in revision spec

Release Note

Support for nodeSelector and tolerations in revision spec

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented Mar 20, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: markusthoemmes
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@markusthoemmes
Copy link
Contributor Author

markusthoemmes commented Mar 20, 2019

/assign @mattmoor

I think this needs superpowers to merge.

@krancour
Copy link
Contributor

krancour commented Mar 20, 2019

Confirming that I'm good with this PR that includes my commits. Is there a slash command to convey that??

@krancour @markusthoemmes
Support for NodeSelector and Tolerations.
2d8abed 
Co-authored-by: Markus Thömmes <markusthoemmes@me.com>
@knative-metrics-robot
Copy link

knative-metrics-robot commented Mar 20, 2019

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/serving/v1alpha1/revision_validation.go 96.5% 97.1% 0.6

@dgerd
Copy link

dgerd commented Mar 20, 2019

Confirming that I'm good with this PR that includes my commits. Is there a slash command to convey that??

There is no slash command. It requires a project owner to override the bot and make the merge happen as Markus suggested.

@markusthoemmes
Copy link
Contributor Author

markusthoemmes commented Mar 21, 2019

/test pull-knative-serving-upgrade-tests

2 similar comments
@markusthoemmes
Copy link
Contributor Author

markusthoemmes commented Mar 21, 2019

/test pull-knative-serving-upgrade-tests

@markusthoemmes
Copy link
Contributor Author

markusthoemmes commented Mar 21, 2019

/test pull-knative-serving-upgrade-tests

@markusthoemmes
Copy link
Contributor Author

markusthoemmes commented Mar 21, 2019

Can't wait for the 0.5 release...

/test pull-knative-serving-upgrade-tests

@markusthoemmes
Copy link
Contributor Author

markusthoemmes commented Mar 21, 2019

/test pull-knative-serving-upgrade-tests

@mattmoor
Copy link
Member

mattmoor commented Mar 21, 2019

I'm not sure we ever adequately discussed this at the WG (or v1beta1 task force), and I know this makes a number of folks uneasy (myself included). Nodeful concepts like these feel like really leaky Ops abstractions to allow in our API.

I'll add this to the taskforce backlog.

dprotaso
dprotaso reviewed Mar 21, 2019
View reviewed changes
pkg/apis/serving/v1alpha1/revision_validation.go
Paths: []string{"operator"},
}
}
// validate toleration effect, empty toleration effect means match all taint
Copy link
Member

@dprotaso dprotaso Mar 21, 2019

Choose a reason for hiding this comment

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

Perma-linking for reference

https://github.com/kubernetes/kubernetes/blob/ad4727440cc4a5be5c9714ffc0baefc11ce6c7f6/pkg/apis/core/validation/validation.go#L2774

dprotaso
dprotaso reviewed Mar 21, 2019
View reviewed changes
pkg/apis/serving/v1alpha1/revision_validation.go
// k8s.io/kubernetes/pkg/apis/core/validation. Although relevant functions from
// that package are exported, they're not usable here because they are for
// unversioned resources.
func validateToleration(toleration corev1.Toleration) *apis.FieldError {
Copy link
Member

@dprotaso dprotaso Mar 21, 2019

Choose a reason for hiding this comment

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

Perma-linking for reference:

https://github.com/kubernetes/kubernetes/blob/ad4727440cc4a5be5c9714ffc0baefc11ce6c7f6/pkg/apis/core/validation/validation.go#L2834

dprotaso
dprotaso reviewed Mar 21, 2019
View reviewed changes
pkg/apis/serving/v1alpha1/revision_validation.go
}

if err := validateNodeSelector(rs.NodeSelector); err != nil {
return err.ViaField("nodeSelector")
Copy link
Member

@dprotaso dprotaso Mar 21, 2019

Choose a reason for hiding this comment

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

Append to errs instead of returning?

pkg/apis/serving/v1alpha1/revision_validation.go
}
for i, toleration := range rs.Tolerations {
if err := validateToleration(toleration); err != nil {
return err.ViaField(
Copy link
Member

@dprotaso dprotaso Mar 21, 2019

Choose a reason for hiding this comment

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

Append to errs instead of returning?

pkg/reconciler/v1alpha1/revision/resources/deploy.go
ServiceAccountName: rev.Spec.ServiceAccountName,
TerminationGracePeriodSeconds: &revisionTimeout,
NodeSelector: rev.Spec.NodeSelector,
Tolerations: rev.Spec.Tolerations,
Copy link
Member

@dprotaso dprotaso Mar 21, 2019

Choose a reason for hiding this comment

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

Can we confirm these are mutable.

If not we will need changes to the checkAndUpdateDeployment logic here:

serving/pkg/reconciler/v1alpha1/revision/cruds.go

Lines 66 to 68 in bbe4c2f

// Preserve the label selector since it's immutable.
// TODO(dprotaso): determine other immutable properties.
deployment.Spec.Selector = have.Spec.Selector

@krancour krancour mentioned this pull request Apr 2, 2019
Closed
@markusthoemmes
Copy link
Contributor Author

markusthoemmes commented May 8, 2019

Closing this for now until we have stronger opinions on really needing this.

@kaleo211
Copy link

kaleo211 commented Jul 12, 2019

This is a nice feature to have :)

This was referenced Feb 27, 2026
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dprotaso dprotaso dprotaso left review comments

@knative-prow-robot knative-prow-robot knative-prow-robot left review comments

@dgerd dgerd Awaiting requested review from dgerd

Assignees

@mattmoor mattmoor

Labels

area/API API objects and controllers size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Using nodeSelector/tolerations with Serving?

9 participants

@markusthoemmes @googlebot @knative-prow-robot @krancour @knative-metrics-robot @dgerd @mattmoor @kaleo211 @dprotaso