Skip to content
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

Added Helm charts for deployment. #35

Open
wants to merge 7 commits into
base: developing
Choose a base branch
from

Conversation

anvithks
Copy link

Added Helm3 support for deploying Huawei CSI driver for Kubernetes.

  • Created the basic Chart with driver information.
  • Created common values.yaml
  • Created the common ConfigMap

registrar: quay.io/k8scsi/csi-node-driver-registrar:v2.0.1
huaweiCsiControllerService: huawei-csi:test
huaweiCsiNodeService: huawei-csi:test
imagePullPolicy: "IfNotPresent"
Copy link

@AmitRoushan AmitRoushan Aug 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comments on each attribute for easy of use

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

name: huawei-csi-configmap
- name: secret
secret:
secretName: huawei-csi-secret

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, Add line feed at end of file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

- name: registration-dir
mountPath: /registration
- name: huawei-csi-driver
image: {{ required "Must provide the CSI controller service node image." .Values.images.huaweiCsiNodeService }}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO message is for node service.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this.

name: huawei-csi-configmap
- name: secret
secret:
secretName: huawei-csi-secret

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, Add line feed at end of file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

capabilities:
add: ["SYS_ADMIN"]
allowPrivilegeEscalation: true
imagePullPolicy: "IfNotPresent"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add image pull policy template here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

valueFrom:
fieldRef:
apiVersion: v1
fieldPath: spec.nodeName

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, Currently the "CSI_ESDK_NODENAME" is not used. Please remove it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I leave the env: section empty or remove the section altogether?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the env variable CSI_ESDK_NODENAME for now and retained the env: section.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove env section as well

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

namespace: kube-system

---
kind: ClusterRole
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Helm best practices each resource definition should be defined in a separate template.

metadata:
name: huawei-csi-controller
namespace: kube-system
---
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Helm best practices each resource definition should be defined in a separate template.

apiVersion: apps/v1
metadata:
name: huawei-csi-controller
namespace: kube-system
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoding a namespace is not recommended in Helm charts. We should discuss how to best approach the issue with the kube-system namespace being hardcoded in secretGenerate and secretUpdate.

template:
metadata:
labels:
app: huawei-csi-controller
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a the default template function for labels created by helm create ... would allow you to be more consistent regarding your labels and simplify the configuration of labels. See ... for how we're using the .labels and .selectorLabels from _helpers.tpl in our Helm chart: https://github.com/adfinis-sygroup/helm-charts/blob/c23bb7d8380df256e454cc143510de60511ae2e2/charts/huawei-csi-plugin/templates/daemonset-node.yaml#L7

data:
csi.json: |
{{ $length := len .Values.backends }} {{ if gt $length 0 }} { {{ end }}
"backends": {{ .Values.backends | toPrettyJson | nindent 8 }}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our chart we decided to just dump the YAML structure from values.yaml directly into csi.json: https://github.com/adfinis-sygroup/helm-charts/blob/c23bb7d8380df256e454cc143510de60511ae2e2/charts/huawei-csi-plugin/templates/configmap.yaml#L9

Might be easier then constructing a JSON and more flexible if you would decide to add more then just the backends section in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants