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

Add helm NOTES file #10641

Merged
merged 1 commit into from
Apr 1, 2020
Merged

Conversation

soumynathan
Copy link
Contributor

This patch adds helm NOTES file to the respective template folders.

Fixes: #10070
Signed-off-by: Swaminathan Vasudevan svasudevan@suse.com

@soumynathan soumynathan requested a review from a team March 19, 2020 19:37
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

Could you show a full example of how the helm output looks when you use install with these notes files in place? In particular I'm wondering how the multiple copies of the notes file interact with the user as they won't be manually installing different subcharts from each of these directories.

Some more minor notes below.

install/kubernetes/cilium/charts/agent/templates/NOTES.txt Outdated Show resolved Hide resolved
@@ -0,0 +1,6 @@
You have successfully installed Cilium {{ .Chart.name }}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need all of the different copies of this? Surely when a user runs helm install cilium ... they only need to see this message once, and that's pulled from a single location...? (I'm not sure, but would be good to know).

If so, can we symlink them to one common copy to simplify making changes in future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought was to add the notes for each and every template directory and calling these directory depends on the global configuration.
We can even restrict to the high level chart instead of the sub charts level. What do you think.

Copy link
Member

Choose a reason for hiding this comment

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

At a high level from user perspective, I assume the user installs via helm install ... and we print this nice message telling them what just happened and how to find help. Achieving that is the main goal of the issue #10070.

From a maintainer perspective, I don't like adding the same content to the repo multiple times. If we ever need to change it, we have to manually go through and make the change to all these different files, I'd like to avoid this. If we avoid it by having a single copy in a more appropriate location, great. If we avoid it by creating one copy of the file and symlinking, that's fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. For now when I try to install through HELM.
This is what I see, even though the NOTE.txt is there in place.

NAME: cilium
LAST DEPLOYED: Thu Mar 19 22:18:52 2020
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None

For some reason it is not picking up the contents from the NOTES.txt file. Let me add it to the cilium folder and see if it reflects.

Copy link
Member

Choose a reason for hiding this comment

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

@soumynathan Where are we at with this?

  • Can we use something like symlinks to ensure we have exactly one copy of the notice that's easy to update?
  • Does the output work now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running individual charts, pulls in the notes.txt file, as shown below.

master-node:~/go/src/github.com/cilium/install/kubernetes$ helm install --dry-run --namespace kube-system -f ./cilium/values.yaml cilium ./cilium/charts/agent
NAME: cilium
LAST DEPLOYED: Wed Apr  8 18:21:48 2020
NAMESPACE: kube-system
STATUS: pending-install
REVISION: 1
TEST SUITE: None
HOOKS:
MANIFEST:
---
# Source: agent/templates/serviceaccount.yaml
apiVersion: v1
kind: ServiceAccount
metadata:
 name: cilium
 namespace: kube-system
---
# Source: agent/templates/clusterrole.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
 name: cilium
rules:
- apiGroups:
 - networking.k8s.io
 resources:
 - networkpolicies
 verbs:
 - get
 - list
 - watch
- apiGroups:
 - discovery.k8s.io
 resources:
 - endpointslices
 verbs:
 - get
 - list
 - watch
- apiGroups:
 - ""
 resources:
 - namespaces
 - services
 - nodes
 - endpoints
 verbs:
 - get
 - list
 - watch
- apiGroups:
 - ""
 resources:
 - pods
 - nodes
 verbs:
 - get
 - list
 - watch
 - update
- apiGroups:
 - ""
 resources:
 - nodes
 - nodes/status
 verbs:
 - patch
- apiGroups:
 - apiextensions.k8s.io
 resources:
 - customresourcedefinitions
 verbs:
 - create
 - get
 - list
 - watch
 - update
- apiGroups:
 - cilium.io
 resources:
 - ciliumnetworkpolicies
 - ciliumnetworkpolicies/status
 - ciliumclusterwidenetworkpolicies
 - ciliumclusterwidenetworkpolicies/status
 - ciliumendpoints
 - ciliumendpoints/status
 - ciliumnodes
 - ciliumnodes/status
 - ciliumidentities
 - ciliumidentities/status
 verbs:
 - '*'
---
# Source: agent/templates/clusterrolebinding.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
 name: cilium
roleRef:
 apiGroup: rbac.authorization.k8s.io
 kind: ClusterRole
 name: cilium
subjects:
- kind: ServiceAccount
 name: cilium
 namespace: kube-system
---
# Source: agent/templates/daemonset.yaml
apiVersion: apps/v1
kind: DaemonSet
metadata:
 labels:
   k8s-app: cilium
 name: cilium
 namespace: kube-system
spec:
 selector:
   matchLabels:
     k8s-app: cilium
 template:
   metadata:
     annotations:
       # This annotation plus the CriticalAddonsOnly toleration makes
       # cilium to be a critical pod in the cluster, which ensures cilium
       # gets priority scheduling.
       # https://kubernetes.io/docs/tasks/administer-cluster/guaranteed-scheduling-critical-addon-pods/
       scheduler.alpha.kubernetes.io/critical-pod: ""
     labels:
       k8s-app: cilium
   spec:
     containers:
     - args:
       - --config-dir=/tmp/cilium/config-map
       command:
       - cilium-agent
       livenessProbe:
         exec:
           command:
           - cilium
           - status
           - --brief
         failureThreshold: 10
         # The initial delay for the liveness probe is intentionally large to
         # avoid an endless kill & restart cycle if in the event that the initial
         # bootstrapping takes longer than expected.
         initialDelaySeconds: 120
         periodSeconds: 30
         successThreshold: 1
         timeoutSeconds: 5
       readinessProbe:
         exec:
           command:
           - cilium
           - status
           - --brief
         failureThreshold: 3
         initialDelaySeconds: 5
         periodSeconds: 30
         successThreshold: 1
         timeoutSeconds: 5
       env:
       - name: K8S_NODE_NAME
         valueFrom:
           fieldRef:
             apiVersion: v1
             fieldPath: spec.nodeName
       - name: CILIUM_K8S_NAMESPACE
         valueFrom:
           fieldRef:
             apiVersion: v1
             fieldPath: metadata.namespace
       - name: CILIUM_FLANNEL_MASTER_DEVICE
         valueFrom:
           configMapKeyRef:
             key: flannel-master-device
             name: cilium-config
             optional: true
       - name: CILIUM_FLANNEL_UNINSTALL_ON_EXIT
         valueFrom:
           configMapKeyRef:
             key: flannel-uninstall-on-exit
             name: cilium-config
             optional: true
       - name: CILIUM_CLUSTERMESH_CONFIG
         value: /var/lib/cilium/clustermesh/
       - name: CILIUM_CNI_CHAINING_MODE
         valueFrom:
           configMapKeyRef:
             key: cni-chaining-mode
             name: cilium-config
             optional: true
       - name: CILIUM_CUSTOM_CNI_CONF
         valueFrom:
           configMapKeyRef:
             key: custom-cni-conf
             name: cilium-config
             optional: true
       image: "docker.io/cilium/cilium:latest"
       imagePullPolicy: Always
       lifecycle:
         postStart:
           exec:
             command:
             - "/cni-install.sh"
             - "--enable-debug=false"
         preStop:
           exec:
             command:
             - /cni-uninstall.sh
       name: cilium-agent
       securityContext:
         capabilities:
           add:
           - NET_ADMIN
           - SYS_MODULE
         privileged: true
       volumeMounts:
       - mountPath: /sys/fs/bpf
         name: bpf-maps
       - mountPath: /var/run/cilium
         name: cilium-run
       - mountPath: /host/opt/cni/bin
         name: cni-path
       - mountPath: /host/etc/cni/net.d
         name: etc-cni-netd
       - mountPath: /var/lib/cilium/clustermesh
         name: clustermesh-secrets
         readOnly: true
       - mountPath: /tmp/cilium/config-map
         name: cilium-config-path
         readOnly: true
         # Needed to be able to load kernel modules
       - mountPath: /lib/modules
         name: lib-modules
         readOnly: true
       - mountPath: /run/xtables.lock
         name: xtables-lock
     hostNetwork: true
     initContainers:
     - command:
       - /init-container.sh
       env:
       - name: CILIUM_ALL_STATE
         valueFrom:
           configMapKeyRef:
             key: clean-cilium-state
             name: cilium-config
             optional: true
       - name: CILIUM_BPF_STATE
         valueFrom:
           configMapKeyRef:
             key: clean-cilium-bpf-state
             name: cilium-config
             optional: true
       - name: CILIUM_WAIT_BPF_MOUNT
         valueFrom:
           configMapKeyRef:
             key: wait-bpf-mount
             name: cilium-config
             optional: true
       image: "docker.io/cilium/cilium:latest"
       imagePullPolicy: Always
       name: clean-cilium-state
       securityContext:
         capabilities:
           add:
           - NET_ADMIN
         privileged: true
       volumeMounts:
       - mountPath: /sys/fs/bpf
         name: bpf-maps
         mountPropagation: HostToContainer
       - mountPath: /var/run/cilium
         name: cilium-run
       resources:
         requests:
           cpu: 100m
           memory: 100Mi
     restartPolicy: Always
     priorityClassName: system-node-critical
     serviceAccount: cilium
     serviceAccountName: cilium
     terminationGracePeriodSeconds: 1
     tolerations:
     - operator: Exists
     volumes:
       # To keep state between restarts / upgrades
     - hostPath:
         path: /var/run/cilium
         type: DirectoryOrCreate
       name: cilium-run
       # To keep state between restarts / upgrades for bpf maps
     - hostPath:
         path: /sys/fs/bpf
         type: DirectoryOrCreate
       name: bpf-maps
     # To install cilium cni plugin in the host
     - hostPath:
         path:  /opt/cni/bin
         type: DirectoryOrCreate
       name: cni-path
       # To install cilium cni configuration in the host
     - hostPath:
         path: /etc/cni/net.d
         type: DirectoryOrCreate
       name: etc-cni-netd
       # To be able to load kernel modules
     - hostPath:
         path: /lib/modules
       name: lib-modules
       # To access iptables concurrently with other processes (e.g. kube-proxy)
     - hostPath:
         path: /run/xtables.lock
         type: FileOrCreate
       name: xtables-lock
       # To read the clustermesh configuration
     - name: clustermesh-secrets
       secret:
         defaultMode: 420
         optional: true
         secretName: cilium-clustermesh
       # To read the configuration from the config map
     - configMap:
         name: cilium-config
       name: cilium-config-path
 updateStrategy:
   rollingUpdate:
     maxUnavailable: 2
   type: RollingUpdate

NOTES:
You have successfully installed Cilium agent

Your release version is 1.7.90

For any further help, visit https://docs.cilium.io/en/v1.7/gettinghelp

@@ -0,0 +1,6 @@
You have successfully installed Cilium {{ .Chart.name }}
Copy link
Member

Choose a reason for hiding this comment

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

For preflight, I think the notes page might need to be a bit different if there's an option, because preflight is only pulling the cilium images to all the nodes, it's not installing them yet.

@coveralls
Copy link

coveralls commented Mar 19, 2020

Coverage Status

Coverage decreased (-0.01%) to 45.514% when pulling 5f80e5f on soumynathan:add-helm-notes-for-cilium into 3486b3f on cilium:master.

@soumynathan
Copy link
Contributor Author

Thanks for working on this.

Could you show a full example of how the helm output looks when you use install with these notes files in place? In particular I'm wondering how the multiple copies of the notes file interact with the user as they won't be manually installing different subcharts from each of these directories.

Some more minor notes below.

Sure I can post the output.

@aanm aanm added pending-review release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Mar 20, 2020
This patch adds helm NOTES.txt file to the respective template folders.

Fixes: cilium#10070
Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>
@aanm
Copy link
Member

aanm commented Mar 30, 2020

test-me-please

Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

@joestringer should we backport this or leave it for 1.8?

@joestringer
Copy link
Member

Backporting to v1.7 seems low-risk and easy enough.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.2 Mar 30, 2020
@aanm aanm merged commit 94dcaa7 into cilium:master Apr 1, 2020
1.8.0 automation moved this from In progress to Merged Apr 1, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.2 Apr 1, 2020
@joestringer joestringer moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.2 Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.7.2
Backport done to v1.7
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

Add helm NOTES file
4 participants