Add parameter to configure health endpoint port #1885

tobiasgiese · 2024-09-23T08:44:46Z

Follow-up to #1878

To avoid port conflicts while running in hostNetwork mode we have to make the health ports configurable.

Tested via

❯ helm template \
  --set topologyUpdater.enable=true \
  --set master.healthPort=9001 \
  --set worker.healthPort=9002 \
  --set topologyUpdater.healthPort=9003 \
  deployment/helm/node-feature-discovery | grep -e -health
          - "-health=9003"
        - "-health=9002"
            - "-health=9001"

netlify · 2024-09-23T08:45:02Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`53ddf08`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/66f2bb8bf092700007fac973
😎 Deploy Preview	https://deploy-preview-1885--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.

.gitignore

adrianchiris

minor nit, otherwise looks good imo

cmd/nfd-master/main.go

tobiasgiese · 2024-09-23T11:22:06Z

/cc @marquiz

.gitignore

cmd/nfd-master/main.go

ArangoGutierrez · 2024-09-24T12:28:18Z

cmd/nfd-worker/main.go

@@ -138,6 +136,8 @@ func initFlags(flagset *flag.FlagSet) (*worker.Args, *worker.ConfigOverrideArgs)
 		"Do not publish feature labels")
 	flagset.IntVar(&args.MetricsPort, "metrics", 8081,
 		"Port on which to expose metrics.")
+	flagset.IntVar(&args.GrpcHealthPort, "health", 8082,


same conversation as in master

I kept the helm value as healthPort, WDYT?

adrianchiris

Thx for addressing my comments @tobiasgiese left a few minor nit based on the flag rename.

cmd/nfd-master/main.go

cmd/nfd-topology-updater/main.go

cmd/nfd-worker/main.go

adrianchiris · 2024-09-24T12:58:33Z

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

@@ -58,6 +58,7 @@ master:
  # be removed with it in a future release
  port: 8080
  metricsPort: 8081
+  healthPort: 8082


im ok with this being general health port while we map to grpc-health flag. later on it can map to a different flag if/once needed.

xref #1885 (comment)

Signed-off-by: Tobias Giese <tgiese@nvidia.com>

adrianchiris

/lgtm

tobiasgiese · 2024-09-24T13:18:47Z

/retest

adrianchiris

/lgtm

k8s-ci-robot · 2024-09-24T13:21:19Z

LGTM label has been added.

Git tree hash: 5c6de8e5df03440483409e14c2ff4d531d817b07

tobiasgiese · 2024-09-24T13:21:34Z

/retest

something is broken/flaky here 🤔

failed to remove nfd-builder: no builder "nfd-builder" found
ERROR: failed to remove one or more builders

and

net/http: TLS handshake timeout

Seems like the community prow is broken currently

marquiz

Thank you @tobiasgiese for the contribution. Looks good to me

k8s-ci-robot · 2024-09-24T15:25:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianchiris, 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~~ [marquiz]

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

marquiz · 2024-10-09T08:51:58Z

/cherry-pick release-0.16

k8s-infra-cherrypick-robot · 2024-10-09T08:52:37Z

@marquiz: #1885 failed to apply on top of branch "release-0.16":

Applying: Add parameter to configure health endpoint port
Using index info to reconstruct a base tree...
M	cmd/nfd-master/main.go
M	cmd/nfd-worker/main.go
M	deployment/helm/node-feature-discovery/templates/master.yaml
M	deployment/helm/node-feature-discovery/templates/topologyupdater.yaml
M	deployment/helm/node-feature-discovery/templates/worker.yaml
M	deployment/helm/node-feature-discovery/values.yaml
M	docs/deployment/helm.md
Falling back to patching base and 3-way merge...
Auto-merging docs/deployment/helm.md
CONFLICT (content): Merge conflict in docs/deployment/helm.md
Auto-merging deployment/helm/node-feature-discovery/values.yaml
Auto-merging deployment/helm/node-feature-discovery/templates/worker.yaml
CONFLICT (content): Merge conflict in deployment/helm/node-feature-discovery/templates/worker.yaml
Auto-merging deployment/helm/node-feature-discovery/templates/topologyupdater.yaml
CONFLICT (content): Merge conflict in deployment/helm/node-feature-discovery/templates/topologyupdater.yaml
Auto-merging deployment/helm/node-feature-discovery/templates/master.yaml
CONFLICT (content): Merge conflict in deployment/helm/node-feature-discovery/templates/master.yaml
Auto-merging cmd/nfd-worker/main.go
Auto-merging cmd/nfd-master/main.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add parameter to configure health endpoint port
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.16

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.

marquiz · 2024-10-09T08:52:52Z

Very low risk for backporting, even if it brings new functionality, modifying the source

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

k8s-ci-robot requested review from adrianchiris and ArangoGutierrez September 23, 2024 08:44

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 23, 2024

tobiasgiese mentioned this pull request Sep 23, 2024

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

Merged

tobiasgiese force-pushed the health-port-arg branch from 1e42741 to 4d56312 Compare September 23, 2024 08:48

tobiasgiese commented Sep 23, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

adrianchiris reviewed Sep 23, 2024

View reviewed changes

cmd/nfd-master/main.go Outdated Show resolved Hide resolved

k8s-ci-robot requested a review from marquiz September 23, 2024 11:22

marquiz mentioned this pull request Sep 23, 2024

Release v0.16.5 #1849

Closed

14 tasks

ArangoGutierrez requested changes Sep 24, 2024

View reviewed changes

tobiasgiese force-pushed the health-port-arg branch from 4d56312 to 1e7f6cf Compare September 24, 2024 12:33

adrianchiris reviewed Sep 24, 2024

View reviewed changes

tobiasgiese force-pushed the health-port-arg branch from 1e7f6cf to 0bf26fb Compare September 24, 2024 13:14

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 24, 2024

tobiasgiese force-pushed the health-port-arg branch from 0bf26fb to ed0ca13 Compare September 24, 2024 13:15

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 24, 2024

Add parameter to configure health endpoint port

53ddf08

Signed-off-by: Tobias Giese <tgiese@nvidia.com>

tobiasgiese force-pushed the health-port-arg branch from ed0ca13 to 53ddf08 Compare September 24, 2024 13:15

adrianchiris approved these changes Sep 24, 2024

View reviewed changes

k8s-ci-robot assigned adrianchiris Sep 24, 2024

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

marquiz approved these changes Sep 24, 2024

View reviewed changes

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

k8s-ci-robot merged commit 530bedd into kubernetes-sigs:master Sep 24, 2024
12 checks passed

tobiasgiese deleted the health-port-arg branch September 24, 2024 15:29

marquiz mentioned this pull request Oct 9, 2024

[release-0.16] Add parameter to configure health endpoint port #1902

Merged

This was referenced Oct 14, 2024

Template exposed health port in helm chart #1904

Merged

Add separate helm values for the liveness and readiness probes #1913

Merged

marquiz mentioned this pull request Dec 12, 2024

Release v0.17 #1875

Closed

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parameter to configure health endpoint port #1885

Add parameter to configure health endpoint port #1885

tobiasgiese commented Sep 23, 2024 •

edited

Loading

netlify bot commented Sep 23, 2024 •

edited

Loading

adrianchiris left a comment

tobiasgiese commented Sep 23, 2024

ArangoGutierrez Sep 24, 2024

tobiasgiese Sep 24, 2024

adrianchiris left a comment

adrianchiris Sep 24, 2024

tobiasgiese Sep 24, 2024

adrianchiris left a comment •

edited

Loading

tobiasgiese commented Sep 24, 2024

adrianchiris left a comment

k8s-ci-robot commented Sep 24, 2024

tobiasgiese commented Sep 24, 2024 •

edited

Loading

marquiz left a comment

k8s-ci-robot commented Sep 24, 2024

marquiz commented Oct 9, 2024

k8s-infra-cherrypick-robot commented Oct 9, 2024

marquiz commented Oct 9, 2024

Add parameter to configure health endpoint port #1885

Add parameter to configure health endpoint port #1885

Conversation

tobiasgiese commented Sep 23, 2024 • edited Loading

netlify bot commented Sep 23, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

adrianchiris left a comment

Choose a reason for hiding this comment

tobiasgiese commented Sep 23, 2024

ArangoGutierrez Sep 24, 2024

Choose a reason for hiding this comment

tobiasgiese Sep 24, 2024

Choose a reason for hiding this comment

adrianchiris left a comment

Choose a reason for hiding this comment

adrianchiris Sep 24, 2024

Choose a reason for hiding this comment

tobiasgiese Sep 24, 2024

Choose a reason for hiding this comment

adrianchiris left a comment • edited Loading

Choose a reason for hiding this comment

tobiasgiese commented Sep 24, 2024

adrianchiris left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 24, 2024

tobiasgiese commented Sep 24, 2024 • edited Loading

marquiz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 24, 2024

marquiz commented Oct 9, 2024

k8s-infra-cherrypick-robot commented Oct 9, 2024

marquiz commented Oct 9, 2024

tobiasgiese commented Sep 23, 2024 •

edited

Loading

netlify bot commented Sep 23, 2024 •

edited

Loading

adrianchiris left a comment •

edited

Loading

tobiasgiese commented Sep 24, 2024 •

edited

Loading