support for nodeSelector and tolerations by krancour · Pull Request #1831 · knative/serving

krancour · 2018-08-09T20:18:39Z

Thought this wouldn't be too controversial, so @jeremyrickard and I took a crack at it.

Proposed Changes

Support for nodeSelector and tolerations in revision spec

krancour · 2018-08-10T13:19:54Z

/test pull-knative-serving-go-coverage

krancour · 2018-08-10T14:34:23Z

I'm really not sure what's up with prow right now. I can't seem to get it to re-run coverage either by bumping it or amending the PR.

Report above shows a 1.2% reduction in coverage. That's a lie. lol. This has 100% coverage. I just can't get the report to refresh.

mattmoor · 2018-08-10T14:50:50Z

@steuhs

krancour · 2018-08-10T15:15:27Z

/retest

cppforlife · 2018-08-20T22:28:35Z

any thoughts about adding affinity as well? it seems that knative/build is also adding it: knative/build#237 (though they didnt add tolerations, only nodeSelector so far)

i could also see node anti-affinity (nodes and azs) being a good default that knative sets up for applications to increase high availability of services. thoughts?

bbrowning

Thanks for taking this on! And extra thanks for the tests that cover the new behavior.

I don't love having to copy all the validateToleration from Kubernetes itself, but I also don't see an obviously better way to handle that.

I do have a couple of requested changes - one trivial and one a bit more work to ensure the spec and conformance tests stay in sync with changes to the api, as described at https://github.com/knative/serving/tree/master/docs/spec

bbrowning · 2018-09-28T01:30:45Z

config/config-network.yaml

  # https://istio.io/docs/tasks/traffic-management/egress/
  #
-  istio.sidecar.includeOutboundIPRanges: "*"
+  istio.sidecar.includeOutboundIPRanges: "10.0.0.1/24"


This change was probably unintentionally included with this PR. I have the same local change and have almost accidentally included it in a PR as well 😄

Whoops. That's embarrassing. Thank for catching it, @bbrowning.

bbrowning · 2018-09-28T01:34:03Z

pkg/apis/serving/v1alpha1/revision_types.go

+	// that node.
+	// More info: https://kubernetes.io/docs/concepts/configuration/assign-pod-node/
+	// +optional
+	NodeSelector map[string]string `json:"nodeSelector,omitempty"`


This api change (and the Tolerations below it) should include a corresponding change to the spec at https://github.com/knative/serving/blob/master/docs/spec/spec.md and the conformance tests at https://github.com/knative/serving/tree/master/test/conformance, if applicable.

@bbrowning I updated docs just now.

I'm really not sure whether the conformance tests are applicable here, and if so, I'm not sure where to begin.

markusthoemmes · 2019-01-25T09:50:59Z

@krancour could you give a brief status here? It seems to be mostly there implementationwise. I'd be happy to help push it through if you don't have time to finish it up.

krancour · 2019-01-25T18:17:43Z

@markusthoemmes sorry... I completely lost track of it. I'll be happy to clean this up and rebase this afternoon.

knative-prow-robot · 2019-01-25T21:12:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

If they are not already assigned, you can assign the PR to them by writing /assign @mdemirhan 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:

OWNERS

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

krancour · 2019-01-25T21:50:51Z

I don't quite understand the "pull-knative-serving-build-tests" failure.

krancour · 2019-01-29T15:11:25Z

/retest

krancour · 2019-01-29T15:30:19Z

@bbrowning @mattmoor can either of you give me some insight on why the pull-knative-serving-build-tests are failing? It's not clear to me.

markusthoemmes · 2019-01-29T16:15:11Z

@krancour it seems like a linting error in a markdown:

I0129 15:12:22.023] ------------------------------------
I0129 15:12:22.039] ---- Linting the markdown files ----
I0129 15:12:22.039] ------------------------------------
I0129 15:12:23.684] docs/spec/spec.md:176: MD009 Trailing spaces

knative-metrics-robot · 2019-01-29T18:02:16Z

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	94.7%	95.8%	1.1

krancour · 2019-01-29T18:39:41Z

@markusthoemmes I'm not sure where you got that from. I couldn't find those same lines in the logs from the tests. Regardless, thank you.

I fixed the lint issue in the docs.

markusthoemmes · 2019-02-08T11:58:45Z

meh. I missed the update and now we're back in merge conflict land again :/

krancour · 2019-02-08T17:20:20Z

I don't have the time to address those right now, unfortunately. It could be a while.

markusthoemmes · 2019-03-20T11:08:33Z

@krancour Do you mind if I pick this up and rebase it? We need to make sure when merging that you get the proper credits for the commits but that should be solvable.

krancour · 2019-03-20T11:23:26Z

@markusthoemmes go for it. Don't worry about credit. I value the change getting in more than credit for making it.

markusthoemmes · 2019-03-20T17:40:30Z

@krancour opened a new PR with the rebased version. Would you please add a comment there that you're okay with me reopening it?

krancour · 2019-04-02T17:49:40Z

Closing because #3467 is more up to date.

krancour added 5 commits August 8, 2018 11:56

add NodeSelector and Tolerations attrs to RevisionSpec

2a3599c

update generated code

c12b184

pass NodeSelector and Tolerations thru to pod spec

fe5d361

add unit tests for controller changes

cd0fd76

add and test validations for new revision fields

b1d4ae4

knative-prow-robot requested review from grantr and mattmoor August 9, 2018 20:18

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 9, 2018

krancour changed the title ~~Rev scheduling~~ support for nodeSelector and tolerations Aug 9, 2018

krancour added 2 commits August 9, 2018 19:53

get revision validation coverage back to 100%

55fa4de

more specificity in revision validation error field paths

5d11517

remove unreachable return from revision validation

6ec3fe7

bbrowning suggested changes Sep 28, 2018

View reviewed changes

krancour added 3 commits January 25, 2019 14:08

reversing inadvertent config change

a80b311

merge master

1689701

docs

320db45

fix lint issue in docs

cfa23d6

markusthoemmes mentioned this pull request Mar 20, 2019

Support for NodeSelector and Tolerations. #3467

Closed

krancour closed this Apr 2, 2019

clubanderson mentioned this pull request Mar 18, 2026

🌱 knative: Using nodeSelector/tolerations with Serving? kubestellar/console-kb#1395

Closed

Conversation

krancour commented Aug 9, 2018

Proposed Changes

Uh oh!

krancour commented Aug 10, 2018

Uh oh!

krancour commented Aug 10, 2018

Uh oh!

mattmoor commented Aug 10, 2018

Uh oh!

krancour commented Aug 10, 2018

Uh oh!

cppforlife commented Aug 20, 2018

Uh oh!

bbrowning left a comment

Choose a reason for hiding this comment

Uh oh!

bbrowning Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

krancour Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

bbrowning Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

krancour Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

markusthoemmes commented Jan 25, 2019

Uh oh!

krancour commented Jan 25, 2019

Uh oh!

knative-prow-robot commented Jan 25, 2019

Uh oh!

krancour commented Jan 25, 2019

Uh oh!

krancour commented Jan 29, 2019

Uh oh!

krancour commented Jan 29, 2019

Uh oh!

markusthoemmes commented Jan 29, 2019

Uh oh!

knative-metrics-robot commented Jan 29, 2019

Uh oh!

krancour commented Jan 29, 2019

Uh oh!

markusthoemmes commented Feb 8, 2019

Uh oh!

krancour commented Feb 8, 2019

Uh oh!

markusthoemmes commented Mar 20, 2019

Uh oh!

krancour commented Mar 20, 2019

Uh oh!

markusthoemmes commented Mar 20, 2019

Uh oh!

krancour commented Apr 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants