-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update node-feature-discovery deployment #108
Conversation
e0ne
commented
Feb 23, 2021
•
edited
Loading
edited
- Use helm chart updated from NFD repo
- network-operator specifies the latest build of the NFD
- nfd-master '--instance' option is set to 'nvidia.networking' be default
Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>
/retest-nic_operator_helm |
@@ -1,8 +1,15 @@ | |||
apiVersion: v2 | |||
appVersion: v0.6.0 | |||
appVersion: master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we put the image tag v0.6.0-233-g3e00bfb?
deployment/network-operator/charts/node-feature-discovery/templates/clusterrole.yaml
Show resolved
Hide resolved
@@ -46,7 +46,7 @@ spec: | |||
- name: master | |||
securityContext: | |||
{{- toYaml .Values.master.securityContext | nindent 12 }} | |||
image: "{{ .Values.image.repository }}:{{ .Chart.AppVersion }}" | |||
image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this to allow to change the tag? seem my comment above on the Chart.AppVersion which is master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it as exists in NFD but hardcode version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it as exists in NFD but hardcode version
+1
ideally no changes to the chart itself but only override values in network-operator values.yaml file
resources: | ||
{{- toYaml .Values.master.resources | nindent 12 }} | ||
args: | ||
{ { - if .Values.master.instance | empty | not } } | ||
- "--instance={{ .Values.master.instance }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want it to be configurable. you can hardcode the value nvidia.networking or something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's hardcode it in values
@@ -0,0 +1,25 @@ | |||
{{/* | |||
Copyright 2020 NVIDIA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/2020/2021
@@ -24,6 +24,11 @@ sriovNetworkOperator: | |||
|
|||
# Node Feature discovery chart related values | |||
node-feature-discovery: | |||
image: | |||
repository: gcr.io/k8s-staging-nfd/node-feature-discovery | |||
tag: v0.6.0-233-g3e00bfb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do it deafult in the app vesion
repository: gcr.io/k8s-staging-nfd/node-feature-discovery | ||
tag: v0.6.0-233-g3e00bfb | ||
master: | ||
instance: "network-operator" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let remove this to be hard coded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One we'll start using chart from NFD helm repository, we won't be able to hardcode it without network-operator and/or admission controller changes, so I)'m not sure that it's a good idea to do it
4d1e2b8
to
bce0269
Compare
2d7b5cc
to
6bb436a
Compare
logs can be found here. it seems the pod was not labeled with pci-15b3.present or something similar |
/retest-nic_operator_helm |
args: | ||
- "--sleep-interval=60s" | ||
- "--server={{ include "node-feature-discovery.fullname" . }}-master:{{ .Values.master.service.port }}" | ||
{{- with .Values.worker.options }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be restored
6bb436a
to
665a083
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e0ne i expect the nfd chart itself to be identical to what was merged to NFD with the exception of version override in node-feature-discovery/Chart.yaml
is that the case ?
{{- include "node-feature-discovery.labels" . | nindent 4 }} | ||
data: | ||
nfd-worker.conf: | | ||
{{ .Values.worker.config | indent 4 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use the configmap and put our configs here and not under options WDYT ?
i.e override this value from the top level values.yaml in network-operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Let me try it
pullPolicy: IfNotPresent | ||
repository: gcr.io/k8s-staging-nfd/node-feature-discovery | ||
# This should be set to 'IfNotPresent' for released version | ||
pullPolicy: Always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change in top level values.yaml to IfNotPresent ?
This patch also configures '--instance' option for nfd-master to be able to deploy several NFD instances in one cluster. Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>
665a083
to
744e369
Compare
- "0207" | ||
deviceLabelFields: | ||
- vendor | ||
pci: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An indentation looks too big but it works as expected. Any suggestions are welcome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@e0ne please open an issue in github and in our RM for this.
I will merged as it has it doesn't effect functionality
In regards to commit message: |