Add helm values to configure hostNetwork and additional env vars #1878

tobiasgiese · 2024-09-17T13:04:14Z

We have to run our NFD workers in the host network. Also we need additional env variables such as KUBERNETES_SERVICE_HOST and _PORT. To achieve this we can simply add generic helm values. The default behavior is not changed.

Also exposed all container ports because hostNetwork need this information, see

❯ k explain pod.spec.hostNetwork
[...]
DESCRIPTION:
    Host networking requested for this pod. Use the host's network namespace. If
    this option is set, the ports that will be used must be specified. Default
    to false.

out-of-scope: will make the health port configurable in a follow-up PR to be able to run all daemonsets in the host's network namespace (i.e., avoid port conflicts)

netlify · 2024-09-17T13:04:29Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`af0592b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/66eaf8c8f7927800081bcfa0
😎 Deploy Preview	https://deploy-preview-1878--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

ArangoGutierrez · 2024-09-17T13:11:02Z

deployment/helm/node-feature-discovery/values.yaml

@@ -478,6 +482,8 @@ topologyUpdater:
  enable: false
  args: []
  createCRDs: false
+  additionalEnvs: []


My only comment on this PR would be the name of this Var. maybe envs is just ok?

/assign @marquiz

Also thought about this, but there are already envs in the container. The name additionalEnvs makes it more explicit that those are additional env vars IMO. Also okay to rename it to envs, no hard opinion here

Edit: but if we take a look at args it is the same; there are already args set but the helm value args is for additional arguments. Will rename it.

Yeah, sorry about the hassle. I regret my original PR adding args. But now that's changed

marquiz · 2024-09-18T14:42:41Z

deployment/helm/node-feature-discovery/values.yaml

+  args: []
+  envs: []


Looking at this now, I'd prefer name extraEnvs.

I know envs is in line with args. I recall that I have myself added the args and I don't like the name 🙄. I'd suggest I rename that to extraArgs (in a separate PR) and you rename to extraEnvs in this PR.

WDYT?

Nevertheless,we need to document the new values in docs/deployment/helm.md

Renamed to extraEnvs, rebased and updated the helm documentation

k8s-ci-robot · 2024-09-18T15:37:44Z

@tobiasgiese: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-node-feature-discovery-build-image-cross-generic	`64ae839`	link	true	`/test pull-node-feature-discovery-build-image-cross-generic`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

deployment/helm/node-feature-discovery/templates/worker.yaml

marquiz

Wouldn't we need this for nfd-gc, too?

We have to run our NFD workers in the host network. Also we need additional env variables such as KUBERNETES_SERVICE_HOST and _PORT. To achieve this we can simply add generic helm values. The default behavior is not changed. Signed-off-by: Tobias Giese <[email protected]>

marquiz

Thank you @tobiasgiese for the improvement. This looks good to me, now
/assign @ArangoGutierrez

ArangoGutierrez

/lgtm

k8s-ci-robot · 2024-09-23T07:08:28Z

LGTM label has been added.

Git tree hash: 76881da39702170941dbb7922ead2421d162d726

k8s-ci-robot · 2024-09-23T07:08:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, marquiz, tobiasgiese

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ArangoGutierrez,marquiz]

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

adrianchiris · 2024-09-23T07:20:56Z

deployment/helm/node-feature-discovery/templates/master.yaml

@@ -55,11 +56,16 @@ spec:
            name: grpc
          - containerPort: {{ .Values.master.metricsPort | default "8081" }}
            name: metrics
+          - containerPort: 8082


do we need to make this one configurable for the host network case ?

i.e also add cmdline flag since currently AFAIU value is fixed [1]

[1]

node-feature-discovery/cmd/nfd-worker/main.go

Line 36 in c056163

GrpcHealthPort = 8082

See PR description :)

out-of-scope: will make the health port configurable in a follow-up PR to be able to run all daemonsets in the host's network namespace (i.e., avoid port conflicts)

Will create the PR in the next hour(s)

oh sorry :)

Will be fixed in #1885

k8s-ci-robot requested review from marquiz and TessaIO September 17, 2024 13:04

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 17, 2024

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 17, 2024

ArangoGutierrez requested changes Sep 17, 2024

View reviewed changes

k8s-ci-robot assigned marquiz Sep 17, 2024

tobiasgiese force-pushed the chart-without-cni branch 2 times, most recently from 221ce95 to b545d02 Compare September 17, 2024 13:17

marquiz reviewed Sep 18, 2024

View reviewed changes

marquiz mentioned this pull request Sep 18, 2024

helm: rename args chart value to extraArgs #1880

Merged

tobiasgiese force-pushed the chart-without-cni branch 3 times, most recently from 5861141 to 64ae839 Compare September 18, 2024 15:34

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2024

tobiasgiese force-pushed the chart-without-cni branch from 64ae839 to 9db207c Compare September 18, 2024 15:38

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 18, 2024

tobiasgiese force-pushed the chart-without-cni branch from 9db207c to 025fc5f Compare September 18, 2024 15:39

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 18, 2024

marquiz reviewed Sep 18, 2024

View reviewed changes

deployment/helm/node-feature-discovery/templates/worker.yaml Outdated Show resolved Hide resolved

marquiz reviewed Sep 18, 2024

View reviewed changes

tobiasgiese force-pushed the chart-without-cni branch from 025fc5f to 4f756b7 Compare September 18, 2024 15:57

tobiasgiese force-pushed the chart-without-cni branch from 4f756b7 to af0592b Compare September 18, 2024 15:59

marquiz approved these changes Sep 18, 2024

View reviewed changes

k8s-ci-robot assigned ArangoGutierrez Sep 18, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2024

ArangoGutierrez approved these changes Sep 23, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2024

ArangoGutierrez mentioned this pull request Sep 23, 2024

Release v0.16.5 #1849

Closed

14 tasks

k8s-ci-robot merged commit d858501 into kubernetes-sigs:master Sep 23, 2024
9 checks passed

tobiasgiese deleted the chart-without-cni branch September 23, 2024 07:18

adrianchiris reviewed Sep 23, 2024

View reviewed changes

tobiasgiese mentioned this pull request Sep 23, 2024

Add parameter to configure health endpoint port #1885

Merged

marquiz mentioned this pull request Oct 10, 2024

[release-0.16] Add helm values to configure hostNetwork and additional env vars #1903

Merged

marquiz mentioned this pull request Dec 12, 2024

Release v0.17 #1875

Open

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add helm values to configure hostNetwork and additional env vars #1878

Add helm values to configure hostNetwork and additional env vars #1878

tobiasgiese commented Sep 17, 2024 •

edited

Loading

netlify bot commented Sep 17, 2024 •

edited

Loading

ArangoGutierrez Sep 17, 2024

tobiasgiese Sep 17, 2024 •

edited

Loading

marquiz Sep 18, 2024

marquiz Sep 18, 2024

tobiasgiese Sep 18, 2024

k8s-ci-robot commented Sep 18, 2024

marquiz left a comment

marquiz left a comment

ArangoGutierrez left a comment

k8s-ci-robot commented Sep 23, 2024

k8s-ci-robot commented Sep 23, 2024

adrianchiris Sep 23, 2024

tobiasgiese Sep 23, 2024 •

edited

Loading

adrianchiris Sep 23, 2024

tobiasgiese Sep 23, 2024

Add helm values to configure hostNetwork and additional env vars #1878

Add helm values to configure hostNetwork and additional env vars #1878

Conversation

tobiasgiese commented Sep 17, 2024 • edited Loading

netlify bot commented Sep 17, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

ArangoGutierrez Sep 17, 2024

Choose a reason for hiding this comment

tobiasgiese Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

marquiz Sep 18, 2024

Choose a reason for hiding this comment

marquiz Sep 18, 2024

Choose a reason for hiding this comment

tobiasgiese Sep 18, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 18, 2024

marquiz left a comment

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

ArangoGutierrez left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 23, 2024

k8s-ci-robot commented Sep 23, 2024

adrianchiris Sep 23, 2024

Choose a reason for hiding this comment

tobiasgiese Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

adrianchiris Sep 23, 2024

Choose a reason for hiding this comment

tobiasgiese Sep 23, 2024

Choose a reason for hiding this comment

tobiasgiese commented Sep 17, 2024 •

edited

Loading

netlify bot commented Sep 17, 2024 •

edited

Loading

tobiasgiese Sep 17, 2024 •

edited

Loading

tobiasgiese Sep 23, 2024 •

edited

Loading