Skip to content

Commit

Permalink
update use of namespaces in chart (#62)
Browse files Browse the repository at this point in the history
To ensure Helm releases are installed in the desired namespace it is important to always specify the release namespace: helm install -n <namespace>.
This MR fixes the chart so it will follow this approach (which is the standard way since I think Helm 3 at least), instead of using a value to control the namespace.

If you helm install without -n it is the same as using kubectl without -n, it should use the default namespace configured in the current context of your kubeconfig. However the current non-standard behaviour of the chart (which I would argue is a bug) overrides this to end up in kube-system instead of whatever your current default is - IF the user doesn't specify it with -n (but you really should).

Actually the more serious bug is that the current values-based namespace selection could potentially prevent the standard release-based namespace selection mechanism from working at all, so a user doing the right thing (helm install -n mynamespace) would potentially end up with the helm release details going in the right namespace and the k8s objects going in the wrong namespace, I am not sure how that would work.

fix #60

Also:
    There is no such thing as a namespace for a ClusterRoleBinding, k8s ignores it. Remove 'default'
    There is no reason to be granting RBAC privileges to the default service account in the default namespace, removed
  • Loading branch information
rptaylor authored May 9, 2024
1 parent 4607bf0 commit df22825
Show file tree
Hide file tree
Showing 5 changed files with 3 additions and 12 deletions.
1 change: 0 additions & 1 deletion helm/amd-gpu/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ Kubernetes: `>= 1.18.0`
| labeller.enabled | bool | `false` | |
| lbl.image.repository | string | `"docker.io/rocm/k8s-device-plugin"` | |
| lbl.image.tag | string | `"labeller-latest"` | |
| namespace | string | `"kube-system"` | |
| nfd.enabled | bool | `false` | |
| node_selector_enabled | bool | `false` | |
| node_selector."feature.node.kubernetes.io/pci-0300_1002.present" | string | `"true"` | |
Expand Down
4 changes: 2 additions & 2 deletions helm/amd-gpu/templates/NOTES.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{{ .Chart.Name }}-device-plugin-daemonset deployed in namespace '{{ .Values.namespace }}'
{{ .Chart.Name }}-device-plugin-daemonset deployed in namespace '{{ .Release.Namespace }}'
{{- if .Values.labeller.enabled }}
{{ .Chart.Name }}-labeller-daemonset deployed in namespace '{{ .Values.namespace }}'
{{ .Chart.Name }}-labeller-daemonset deployed in namespace '{{ .Release.Namespace }}'
{{- end }}
1 change: 0 additions & 1 deletion helm/amd-gpu/templates/deviceplugin-daemonset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ apiVersion: apps/v1
kind: DaemonSet
metadata:
name: {{ .Chart.Name }}-device-plugin-daemonset
namespace: {{ .Values.namespace }}
spec:
selector:
matchLabels:
Expand Down
7 changes: 1 addition & 6 deletions helm/amd-gpu/templates/labeller.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,19 @@ apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: crb-{{ .Chart.Name }}-labeller
namespace: default
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: cr-{{ .Chart.Name }}-node-labeller
subjects:
- kind: ServiceAccount
name: default
namespace: default
- kind: ServiceAccount
name: default
namespace: {{ .Values.namespace }}
namespace: {{ .Release.Namespace }}
---
apiVersion: apps/v1
kind: DaemonSet
metadata:
name: {{ .Chart.Name }}-labeller-daemonset
namespace: {{ .Values.namespace }}
spec:
selector:
matchLabels:
Expand Down
2 changes: 0 additions & 2 deletions helm/amd-gpu/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ nfd:
labeller:
enabled: false

namespace: kube-system

dp:
image:
repository: docker.io/rocm/k8s-device-plugin
Expand Down

0 comments on commit df22825

Please sign in to comment.