deployment: add topology updater helm chart #623

zwpaper · 2021-10-07T17:21:45Z

there are details in the helm chart to help review easier:

NRT disabled by default
CRDs are only created when NRT is enabled and createCRDs is true
NRT needs more permissions, so keep using standalone RBAC like kustomize
NRT CR target NS option is an empty string("") by default, it means the same NS with helm chart installed

k8s-ci-robot · 2021-10-07T17:21:52Z

Hi @zwpaper. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

zwpaper · 2021-10-09T06:29:35Z

/assign @marquiz

I have tested locally, it should work as expected

marquiz

Nice work @zwpaper 👍 Sorry for the delay, but I was secretly hoping to get #608 merged and then align this with it 😇

Some comments but I think this generally looks good.

Ping @eliaskoromilas: you have taken a look at our helm deployment and providing valuable feedback. Maybe you could take a look at this PR, too.

/ok-to-test

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

marquiz · 2021-10-21T08:27:09Z

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

+          - "--kubelet-config-file=/host-var/lib/kubelet/config.yaml"
+          - "--podresources-socket=/host-var/lib/kubelet/pod-resources/kubelet.sock"
+          - "--sleep-interval={{ .Values.topologyUpdater.updateInterval }}"
+          - "--watch-namespace=*"


These should be configurable parameters, similar to what we have for NFD master.

all args now could be configurated by value.yaml,

the --kubelet-config-file and --podresources-socket is using the file mounted from hostpath, so that the config is updating the volume part.

eliaskoromilas · 2021-10-21T12:26:05Z

Thanks @marquiz, actually I do have some comments on this.

I think it would be wise to completely separate the two rbac/serviceaccount configurations.

I would use the existing clusterrole.yaml/clusterrolebinding.yaml/serviceaccount.yaml for the new resources. I think it's more clear than stacking all of them inside a single topologyupdater-rbac.yaml.

The pattern I would use, e.g. in the cluster role binding file, would look like this:

{{- if .Values.rbac.create }}
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: {{ include "node-feature-discovery.fullname" . }}-master
  ...
{{- end }}
---
{{- if .Values.topologyUpdater.rbac.create }}
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: {{ include "node-feature-discovery.fullname" . }}-topology-updater
  ...
{{- end }}

This also means that the related values.yaml configuration would look like:

serviceAccount:
  create: true
  annotations: {}
  name:
rbac:
  create: true

topologyUpdater:
  serviceAccount:
    create: true
    annotations: {}
    name:
  rbac:
    create: true

Of course it's also recommended to move the existing rbac/serviceAccount parameters under master:. This would break compatibility but I think it's acceptable in the current phase.

I also noticed that rbac.serviceAccountName and rbac.serviceAccountAnnotations are not taken into account anywhere inside the templates (and actually could be misleading). These are there since Day 1, should we purge them here/now?

marquiz · 2021-10-21T14:06:45Z

I think it would be wise to completely separate the two rbac/serviceaccount configurations.

Agree 👍

I would use the existing clusterrole.yaml/clusterrolebinding.yaml/serviceaccount.yaml for the new resources. I think it's more clear than stacking all of them inside a single topologyupdater-rbac.yaml.

Makes sense

Of course it's also recommended to move the existing rbac/serviceAccount parameters under master:. This would break compatibility but I think it's acceptable in the current phase.

Perhaps. But I think that should be the subject of another PR.

I also noticed that rbac.serviceAccountName and rbac.serviceAccountAnnotations are not taken into account anywhere inside the templates (and actually could be misleading). These are there since Day 1, should we purge them here/now?

rbac.serviceAccountName is used in templates/_helpers.tpl but 'rbac.serviceAccountAnnotations` really seems to be a leftover and could be dropped. Perhaps better to do that in a separate PR.

eliaskoromilas · 2021-10-21T14:44:34Z

rbac.serviceAccountName is used in templates/_helpers.tpl but 'rbac.serviceAccountAnnotations` really seems to be a leftover and could be dropped. Perhaps better to do that in a separate PR.

I agree addressing this separately. I doubled checked, _helpers.tpl uses .Values.serviceAccount.name and not .Values.rbac.serviceAccountName!

marquiz · 2021-10-21T15:51:42Z

I doubled checked, _helpers.tpl uses .Values.serviceAccount.name and not .Values.rbac.serviceAccountName!

Woops, you're actually right 🤦 So rbac.serviceAccountName in values.yaml should be replaced by rbac.serviceAccount.name (and annotations, correspondingly).

marquiz · 2021-10-21T16:13:50Z

I created #630 to track the enhancements discussed above. Feel free to submit PRs 😉

zwpaper · 2021-10-22T03:03:27Z

thanks for the review! @marquiz and @eliaskoromilas

but I have one more question, do we really need the rbac.serviceAccountName and rbac.serviceAccountAnnotations?

those two seem to be duplicated with serviceAccount, maybe we could unify them and use the serviceAccount.name directly when creating the rbac configurations.

zwpaper · 2021-10-22T04:21:10Z

the build test seems to find some gofmt problems like

--- ./source/cpu/cstate_stub.go.orig	2021-10-22 04:05:28.008781087 +0000
+++ ./source/cpu/cstate_stub.go	2021-10-22 04:05:28.008781087 +0000
@@ -1,3 +1,4 @@
+//go:build !amd64
 // +build !amd64

there are no go changes in this PR, maybe I should raise another PR to fix the gofmt problems?

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

zwpaper · 2021-10-22T08:58:57Z

now this PR should be good to go if there are no more changes requested, or should we need to wait for #608? @marquiz

marquiz · 2021-10-22T09:12:23Z

but I have one more question, do we really need the rbac.serviceAccountName and rbac.serviceAccountAnnotations?

those two seem to be duplicated with serviceAccount, maybe we could unify them and use the serviceAccount.name directly when creating the rbac configurations.

Yeah, let's handle that in a separate PR.

the build test seems to find some gofmt problems like

there are no go changes in this PR, maybe I should raise another PR to fix the gofmt problems?

Ach, that was caused by a recent bump to go v1.17 (and wasn't catched by the CI at the transition). Fixed by #631. We need to merge that and then rebase your PR.

marquiz

Thanks for the update. Two asks (I addition to the few code comments):

Could you add the new parameters to values.yaml?
Could you also add the new parameters to the table in docs/get-started/deployment-and-usage.md?

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

marquiz · 2021-10-22T11:56:58Z

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

+          - "--kubelet-config-file=/host-var/lib/kubelet/config.yaml"
+          - "--podresources-socket=/host-var/lib/kubelet/pod-resources/kubelet.sock"


These two should be parameterized, too (or dropped)

like I said here #623 (comment), those two args are mounted from hostpath, and point to the path inside the container, I have parameterized the hostpath under volume so that users could update those 2 args to the correct hostpath

These are the defaults so no need to specify in the helm chart and they should be dropped (like other cmdline flags that are not changed from default). Unless make them configurable

ok, mounted the files to default location and drop the two args

marquiz

Looks good, now 👍 Just two small comments. After that I think should be ready

marquiz · 2021-10-22T17:26:04Z

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

+          {{- end }}
+        volumeMounts:
+        - name: kubelet-config
+          mountPath: /var/lib/kubelet/config.yaml


Need to mount under /host-var for the container

Suggested change

mountPath: /var/lib/kubelet/config.yaml

mountPath: /host-var/lib/kubelet/config.yaml

marquiz · 2021-10-22T17:26:19Z

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

+        - name: kubelet-config
+          mountPath: /var/lib/kubelet/config.yaml
+        - name: kubelet-podresources-sock
+          mountPath: /var/lib/kubelet/pod-resources/kubelet.sock


Suggested change

mountPath: /var/lib/kubelet/pod-resources/kubelet.sock

mountPath: /host-var/lib/kubelet/pod-resources/kubelet.sock

Ooops, I checked the code and found the default value is set to /var/lib, so I changed that.

but after you point out, and retest, it's /host-var/ 🤦🏻‍♂️, my bad.

changed back.

marquiz

Looks good to me, know. Just one last last wish: could you squash the patchset into one commit? 😊

zwpaper · 2021-10-25T11:20:20Z

hi @marquiz, done squash commits😁

marquiz

Hey, one more nit 🙈 Could you tidy up the commit message (body). We don't need to carry over all the review details there. You can even leave it empty

Signed-off-by: Wei Zhang <kweizh@gmail.com>

zwpaper · 2021-10-26T02:54:42Z

haha, I used to clean up all the commit-msg body, but I saw your word squash so that I used squash🤣, now should be good to go

marquiz

Thanks @zwpaper, looks good now
/lgtm

k8s-ci-robot · 2021-10-26T06:39:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz, zwpaper

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 7, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 7, 2021

k8s-ci-robot requested review from adrianchiris and jjacobelli October 7, 2021 17:21

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 7, 2021

k8s-ci-robot assigned marquiz Oct 9, 2021

marquiz requested changes Oct 21, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 21, 2021

marquiz mentioned this pull request Oct 21, 2021

Helm refactor and cleanup #630

Closed

2 tasks

zwpaper requested a review from marquiz October 22, 2021 04:21

eliaskoromilas reviewed Oct 22, 2021

View reviewed changes

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

marquiz requested changes Oct 22, 2021

View reviewed changes

zwpaper force-pushed the master branch 2 times, most recently from dce38c6 to 1fb927a Compare October 22, 2021 16:00

zwpaper requested a review from marquiz October 22, 2021 16:04

marquiz requested changes Oct 22, 2021

View reviewed changes

zwpaper requested a review from marquiz October 23, 2021 10:41

marquiz reviewed Oct 25, 2021

View reviewed changes

zwpaper force-pushed the master branch from 8f86a37 to aca6a85 Compare October 25, 2021 11:19

zwpaper requested a review from marquiz October 25, 2021 11:20

marquiz reviewed Oct 25, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2021

deployment: add topology updater helm chart

158a559

Signed-off-by: Wei Zhang <kweizh@gmail.com>

zwpaper force-pushed the master branch from aca6a85 to 158a559 Compare October 26, 2021 02:53

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2021

zwpaper requested a review from marquiz October 26, 2021 02:54

marquiz approved these changes Oct 26, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2021

k8s-ci-robot merged commit 661d326 into kubernetes-sigs:master Oct 26, 2021

swatisehgal mentioned this pull request Oct 26, 2021

Exposing Hardware Topology through CRDs in Node feature discovery #333

Closed

marquiz mentioned this pull request Dec 22, 2021

Release v0.10.0 #678

Closed

22 tasks

zwpaper mentioned this pull request Mar 3, 2023

REQUEST: New membership for zwpaper kubernetes/org#4061

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deployment: add topology updater helm chart #623

deployment: add topology updater helm chart #623

zwpaper commented Oct 7, 2021

k8s-ci-robot commented Oct 7, 2021

zwpaper commented Oct 9, 2021 •

edited

Loading

marquiz left a comment

marquiz Oct 21, 2021

zwpaper Oct 22, 2021

eliaskoromilas commented Oct 21, 2021 •

edited

Loading

marquiz commented Oct 21, 2021

eliaskoromilas commented Oct 21, 2021

marquiz commented Oct 21, 2021

marquiz commented Oct 21, 2021

zwpaper commented Oct 22, 2021

zwpaper commented Oct 22, 2021

zwpaper commented Oct 22, 2021

marquiz commented Oct 22, 2021

marquiz left a comment

marquiz Oct 22, 2021

zwpaper Oct 22, 2021

marquiz Oct 22, 2021

zwpaper Oct 22, 2021

marquiz left a comment

marquiz Oct 22, 2021

marquiz Oct 22, 2021

zwpaper Oct 23, 2021

marquiz left a comment

zwpaper commented Oct 25, 2021

marquiz left a comment

zwpaper commented Oct 26, 2021

marquiz left a comment

k8s-ci-robot commented Oct 26, 2021

		- "--kubelet-config-file=/host-var/lib/kubelet/config.yaml"
		- "--podresources-socket=/host-var/lib/kubelet/pod-resources/kubelet.sock"

	mountPath: /var/lib/kubelet/config.yaml
	mountPath: /host-var/lib/kubelet/config.yaml

	mountPath: /var/lib/kubelet/pod-resources/kubelet.sock
	mountPath: /host-var/lib/kubelet/pod-resources/kubelet.sock

deployment: add topology updater helm chart #623

deployment: add topology updater helm chart #623

Conversation

zwpaper commented Oct 7, 2021

k8s-ci-robot commented Oct 7, 2021

zwpaper commented Oct 9, 2021 • edited Loading

marquiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliaskoromilas commented Oct 21, 2021 • edited Loading

marquiz commented Oct 21, 2021

eliaskoromilas commented Oct 21, 2021

marquiz commented Oct 21, 2021

marquiz commented Oct 21, 2021

zwpaper commented Oct 22, 2021

zwpaper commented Oct 22, 2021

zwpaper commented Oct 22, 2021

marquiz commented Oct 22, 2021

marquiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

zwpaper commented Oct 25, 2021

marquiz left a comment

Choose a reason for hiding this comment

zwpaper commented Oct 26, 2021

marquiz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 26, 2021

zwpaper commented Oct 9, 2021 •

edited

Loading

eliaskoromilas commented Oct 21, 2021 •

edited

Loading