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

Adding flags to support overriding container runtime endpoint. #1443

Merged
merged 2 commits into from
May 11, 2021

Conversation

haouc
Copy link
Contributor

@haouc haouc commented Apr 24, 2021

What type of PR is this?
We want to add a flag to let user able to override dockershim.sock with alternative runtime endpoint mounted at cri.sock.

feature
Which issue does this PR fix:
AWS VPC CNI Helm Chart doesn't have a flag to set container runtime mounting path.

What does this PR do / Why do we need it:

  • adding a new flag to enable the overriding
  • adding a new flag to input the path of new runtime socket
  • updating README accordlingly
  • bump chart version to 1.1.5

If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:

Testing done on this change:

  • helm install AWS VPC CNI with new flags enabled
amazon-vpc-cni-k8s % helm install aws-vpc-cni --namespace kube-system charts/aws-vpc-cni --set cri.enabled=true --set cri.hostPath=/var/run/containerd/containerd.sock
NAME: aws-vpc-cni
LAST DEPLOYED: Sat Apr 24 16:28:50 2021
NAMESPACE: kube-system
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
aws-vpc-cni has been installed or updated. To check the status of pods, run:

kubectl get pods --namespace kube-system -l "app.kubernetes.io/name=aws-node,app.kubernetes.io/instance=aws-vpc-cni"

amazon-vpc-cni-k8s % kubectl get pods --namespace kube-system -l "app.kubernetes.io/name=aws-node,app.kubernetes.io/instance=aws-vpc-cni"
NAME             READY   STATUS    RESTARTS   AGE
aws-node-gwmcr   1/1     Running   0          9s
aws-node-m9csq   1/1     Running   0          9s

  • verified mountPath and hostPath
- mountPath: /var/run/cri.sock
        name: cri
- hostPath:
    path: /var/run/containerd/containerd.sock
    type: ""
  name: cri
  • verified IPAMD logging endpoint as cri.sock and able to pull Pod Sandbox info from containerd socket
  • helm uninstalled the AWS VPC CNI
  • helm install without the new flags
amazon-vpc-cni-k8s % helm install aws-vpc-cni --namespace kube-system charts/aws-vpc-cni
NAME: aws-vpc-cni
LAST DEPLOYED: Sat Apr 24 16:20:46 2021
NAMESPACE: kube-system
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
aws-vpc-cni has been installed or updated. To check the status of pods, run:

kubectl get pods --namespace kube-system -l "app.kubernetes.io/name=aws-node,app.kubernetes.io/instance=aws-vpc-cni"
amazon-vpc-cni-k8s % kubectl get pods --namespace kube-system -l "app.kubernetes.io/name=aws-node,app.kubernetes.io/instance=aws-vpc-cni"
NAME             READY   STATUS    RESTARTS   AGE
aws-node-dbqr8   1/1     Running   0          12s
aws-node-rbsrm   1/1     Running   0          12s
  • verified dockershim.sock was mounted as expected
  • verified aws-node up and running and IPAMD logs endpoint as dockershim.sock and able to pull Pod Sandbox info.

Automation added to e2e:

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
no

Does this change require updates to the CNI daemonset config files to work?:

no
Does this PR introduce any user-facing change?:


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Comment on lines 124 to 128
{{- if .Values.cri.enabled }}
- name: cri
hostPath:
path: {{- toYaml .Values.cri.hostPath | nindent 18 }}
{{- else }}

Choose a reason for hiding this comment

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

This is more like socket path override than enable. I'd say expose something like CRI override host path and if that's set, you override in the chart. You can even avoid having an explicit flag (i.e. enabled: false) to check this

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.

None yet

2 participants