Allow to set NodeSelector via `spec.deployments.nodeSelector` by nak3 · Pull Request #658 · knative/operator

nak3 · 2021-06-10T05:26:27Z

Part of #5

This patch adds spec.deployments.nodeSelector to set nodeSelector on
each deployment.

For example, when the following CR is created,

apiVersion: operator.knative.dev/v1alpha1
kind: KnativeServing
metadata:
  name: ks
  namespace: knative-serving
spec:
  high-availability:
    replicas: 1
  deployments:
  - name: webhook
    nodeSelector:
      foo: ba

The webhook deployment has spec.template.spec.nodeSelector.

$ kubectl get deploy -n knative-serving  webhook -o jsonpath={.spec.template.spec.nodeSelector}
{"foo":"bar"}

/cc @houshengbo @markusthoemmes @matzew

Part of #5 This patch adds `spec.deployments.nodeSelector` to set nodeSelector on each deployment. For example, when the following CR is created, ``` apiVersion: operator.knative.dev/v1alpha1 kind: KnativeServing metadata: name: ks namespace: knative-serving spec: high-availability: replicas: 1 deployments: - name: webhook nodeSelector: foo: bar ``` The webhook deployment has `spec.template.spec.nodeSelector`. ``` $ kubectl get deploy -n knative-serving webhook -o jsonpath={.spec.template.spec.nodeSelector} {"foo":"bar"} ```

knative-metrics-robot · 2021-06-10T05:43:52Z

The following is the coverage report on the affected files.
Say /test pull-knative-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/deployments_override.go	84.4%	85.7%	1.3

markusthoemmes

Looks spot on to me!

/hold

For feedback from others.

knative-prow-robot · 2021-06-10T06:38:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes, nak3

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~~ [markusthoemmes]

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

houshengbo · 2021-06-10T15:09:32Z

+
+func replaceNodeSelector(override *v1alpha1.DeploymentOverride, deployment *appsv1.Deployment) {
+	if len(override.NodeSelector) > 0 {
+		deployment.Spec.Template.Spec.NodeSelector = override.NodeSelector


This feature is fine.
My question is whether to overwrite or replace for the nodeselector.
NodeSelector is a key-value map, the same to Labels and Annotations.
For Labels and Annotations, we add the override into the existing map, but for NodeSelector, we replace the existing with the override. Why not keep the existing keys, that is not in override for NodeSelector?
For example,
If override is {"key1": "val1"}, there is an existing map {"key": "val"}, we will end up with {"key1": "val1"}, not {"key1": "val1", "key": "val"}. The existing "key" is gone.

I don't think it's useful to merge node selectors. IIRC they're in an AND combination, so adding more would likely result in no nodes matching anymore. Plus: We don't have preset node selectors at all.

Yes I made this by design because of the reason Markus mentioned.

Then I am ok with this PR.

nak3 · 2021-06-12T01:28:36Z

/hold cancel

nak3 · 2021-06-12T01:29:59Z

I will update the doc here https://knative.dev/docs/install/operator/configuring-serving-cr/#override-system-deployments

nak3 · 2021-06-12T03:02:53Z

/test pull-knative-operator-serving-upgrade-tests

* Add `nodeSelector` to override system deployments by operator This patch adds docs for knative/operator#658. It adds `nodeSelector` to override system deployments by operator. * Fix broken format * Update docs/admin/install/operator/configuring-serving-cr.md Co-authored-by: Ashleigh Brennan <abrennan@redhat.com> * Update docs/admin/install/operator/configuring-serving-cr.md Co-authored-by: Ashleigh Brennan <abrennan@redhat.com> * Update docs/admin/install/operator/configuring-serving-cr.md Co-authored-by: Ashleigh Brennan <abrennan@redhat.com> * Update docs/admin/install/operator/configuring-serving-cr.md Co-authored-by: Ashleigh Brennan <abrennan@redhat.com> Co-authored-by: Ashleigh Brennan <abrennan@redhat.com>

…e#3789) * Add `nodeSelector` to override system deployments by operator This patch adds docs for knative/operator#658. It adds `nodeSelector` to override system deployments by operator. * Fix broken format * Update docs/admin/install/operator/configuring-serving-cr.md Co-authored-by: Ashleigh Brennan <abrennan@redhat.com> * Update docs/admin/install/operator/configuring-serving-cr.md Co-authored-by: Ashleigh Brennan <abrennan@redhat.com> * Update docs/admin/install/operator/configuring-serving-cr.md Co-authored-by: Ashleigh Brennan <abrennan@redhat.com> * Update docs/admin/install/operator/configuring-serving-cr.md Co-authored-by: Ashleigh Brennan <abrennan@redhat.com> Co-authored-by: Ashleigh Brennan <abrennan@redhat.com>

knative-prow-robot requested review from houshengbo, markusthoemmes and matzew June 10, 2021 05:26

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 10, 2021

google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 10, 2021

run hack/update-codegen.sh

b04a277

markusthoemmes approved these changes Jun 10, 2021

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2021

knative-prow-robot assigned markusthoemmes Jun 10, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2021

houshengbo reviewed Jun 10, 2021

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2021

knative-prow-robot merged commit 8cb752e into knative:main Jun 12, 2021

nak3 deleted the add-nodeselector branch June 12, 2021 07:09

nak3 mentioned this pull request Jun 14, 2021

Add nodeSelector to override system deployments by operator knative/docs#3789

Merged

chenrui333 mentioned this pull request May 27, 2026

Expose nodeSelector in knative-operator Helm chart #2305

Open

Navigation Menu

Allow to set NodeSelector via `spec.deployments.nodeSelector` - #658

Allow to set NodeSelector via `spec.deployments.nodeSelector`#658
knative-prow-robot merged 2 commits into
knative:mainfrom
nak3:add-nodeselector

nak3 commented Jun 10, 2021 •

edited

Loading

Uh oh!

knative-metrics-robot commented Jun 10, 2021

Uh oh!

markusthoemmes left a comment

Uh oh!

knative-prow-robot commented Jun 10, 2021

Uh oh!

houshengbo Jun 10, 2021 •

edited

Loading

Uh oh!

markusthoemmes Jun 10, 2021

Uh oh!

nak3 Jun 11, 2021

Uh oh!

houshengbo Jun 11, 2021

Uh oh!

nak3 commented Jun 12, 2021

Uh oh!

nak3 commented Jun 12, 2021

Uh oh!

nak3 commented Jun 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

nak3 commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-metrics-robot commented Jun 10, 2021

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Jun 10, 2021

Uh oh!

houshengbo Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

nak3 Jun 11, 2021

Choose a reason for hiding this comment

Uh oh!

houshengbo Jun 11, 2021

Choose a reason for hiding this comment

Uh oh!

nak3 commented Jun 12, 2021

Uh oh!

nak3 commented Jun 12, 2021

Uh oh!

nak3 commented Jun 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nak3 commented Jun 10, 2021 •

edited

Loading

houshengbo Jun 10, 2021 •

edited

Loading