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

kata-deploy: add k3s support #823

Merged
merged 2 commits into from Dec 3, 2019
Merged

kata-deploy: add k3s support #823

merged 2 commits into from Dec 3, 2019

Conversation

@wilsonianb
Copy link
Contributor

wilsonianb commented Nov 22, 2019

By default, k3s uses an embedded containerd. Reconfiguring this
containerd requires modifying a template config file and restarting the
k3s systemd service.

Signed-off-by: Brandon Wilson brandon@coil.com

As noted in rancher/k3s#274, you can already configure k3s to instead use an external cri currently supported by kata-deploy.
However, I think supporting the embedded k3s containerd provides a very easy (the easiest?) way to deploy a Kata-enabled cluster:

curl -sfL https://get.k3s.io | sh -
kubectl apply -f kata-rbac.yaml
kubectl apply -f kata-deploy.yaml
systemctl restart k3s-agent && sleep 10
else
# master node
systemctl restart k3s && sleep 10

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Nov 22, 2019

Author Contributor

As is, this could be problematic for single master, multi-node clusters since the server has to restart on the master node, potentially causing a simultaneous kubectl describe/label ... call on a worker node to fail.

This comment has been minimized.

Copy link
@jodh-intel

jodh-intel Nov 22, 2019

Contributor

Thanks for raising @wilsonianb but I'm not comfortable with sleeps like this. Isn't the service "ready" once the systemctl restart returns? If not, I think it would be better to check the service status (or some other method) for the expected state periodically, and timeout/die after a much longer timespan.

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Nov 22, 2019

Author Contributor

Removed the sleeps and now it seems to actually run successfully on two node (master + worker) clusters 🙃

@saiyam1814

This comment has been minimized.

Copy link

saiyam1814 commented Nov 22, 2019

👍

Copy link
Contributor

jodh-intel left a comment

Thanks @wilsonianb and great news! 😄

lgtm but please squash your two commits together into a single commit ("git commit -i master" as explained in https://github.com/kata-containers/community/blob/a6198e2e4cf871783beb9a2c1659fdb3129af9f0/CONTRIBUTING.md#updating-your-pr-based-on-review-comments).

@wilsonianb wilsonianb force-pushed the wilsonianb:k3s branch from a2fd6c3 to be1c1e8 Nov 25, 2019
@wilsonianb

This comment has been minimized.

Copy link
Contributor Author

wilsonianb commented Nov 25, 2019

squashed and rebased

@jodh-intel

This comment has been minimized.

Copy link
Contributor

jodh-intel commented Nov 25, 2019

@jcvenegas

This comment has been minimized.

Copy link
Contributor

jcvenegas commented Nov 25, 2019

/test

@jcvenegas

This comment has been minimized.

Copy link
Contributor

jcvenegas commented Nov 25, 2019

/AzurePipelines run

@azure-pipelines

This comment has been minimized.

Copy link

azure-pipelines bot commented Nov 25, 2019

Azure Pipelines successfully started running 2 pipeline(s).
Copy link
Collaborator

egernst left a comment

The changes are clean, and look fine for enabling K3S.

Stepping back, I begin to get concerned that kata-deploy is going to get bigger, and bigger. ie, I'm looking at similar changes that'll be necessary for minikube (/cc @tstromberg).

If we add support, we should make sure that we have a way to test this in our CI as well.

Copy link
Collaborator

egernst left a comment

Thinking through it more - I'm hoping to simplify things, and perhaps push more of the changes in a k3s specific yaml, rather than making larger changes in bash.

PTAL and let me know what you think?

systemctl restart kubelet
restart_cri_runtime "$1"
if [ "$1" != "containerd-k3s" ]; then
systemctl restart kubelet

This comment has been minimized.

Copy link
@egernst

egernst Nov 26, 2019

Collaborator

Stepping back, I'm not sure if we need to restart kubelet for either runtime's either.

Out of curiosity, what was the issue with restarting kubelet in the K3s case?

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Nov 26, 2019

Author Contributor

Like k3s's embedded containerd, kubelet isn't installed as a systemd service.
Let me know if the restart is safe to remove. It looks like it's been there since the beginning: e642e32#diff-1f1e29e0ac464e6daaef21f619af1d96R26

This comment has been minimized.

Copy link
@egernst

egernst Nov 26, 2019

Collaborator

Thanks for clarifying. Let me do some testing on my end, but I expect just removing is appropriate.

This comment has been minimized.

Copy link
@egernst

egernst Nov 26, 2019

Collaborator

(NOTE: we should have CI so I don't have to test and have everyone in the world trust me!)

@@ -36,7 +38,11 @@ function get_container_runtime() {
if [ "$?" -ne 0 ]; then
die "invalid node name"
fi
echo "$runtime" | awk -F'[:]' '/Container Runtime Version/ {print $2}' | tr -d ' '
if echo "$runtime" | grep -qE 'Container Runtime Version.*containerd.*-k3s'; then
echo "containerd-k3s"

This comment has been minimized.

Copy link
@egernst

egernst Nov 26, 2019

Collaborator

should we rename this to just k3s or k3s-agent, so we can restart the service based on the name, like how it works w/ containerd/crio?

if systemctl is-active --quiet k3s-agent; then
# worker node
systemctl restart k3s-agent
else
# master node
systemctl restart k3s
fi
Comment on lines 56 to 62

This comment has been minimized.

Copy link
@egernst

egernst Nov 26, 2019

Collaborator

debating whether just restarting both k3s and k3s-agent will make life easier. Either way, we can't just do restart $1, so perhaps we shouldn't care...

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Nov 26, 2019

Author Contributor

or we could rename containerd-k3s to k3s || systemctl restart k3s-agent 😬
Assuming that's a terrible idea, I like your suggestion to have get_container_runtime return k3s or k3s-agent

@@ -12,6 +12,8 @@ crio_conf_file="/etc/crio/crio.conf"
crio_conf_file_backup="${crio_conf_file}.bak"
containerd_conf_file="/etc/containerd/config.toml"
containerd_conf_file_backup="${containerd_conf_file}.bak"
containerd_k3s_conf_file="/var/lib/rancher/k3s/agent/etc/containerd/config.toml"

This comment has been minimized.

Copy link
@egernst

egernst Nov 26, 2019

Collaborator

@wilsonianb what if we are just changing the yaml to mount /varl/ib/rancher/k3s/agent/etc/containerd to /etc/containerd, and kept majority of the logic identical?

AFAIU we'd still need to handle the different service name resets, which unfortunately would be k3s and k3s-agent, depending on whether this was master or worker node.

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Nov 26, 2019

Author Contributor
The DaemonSet "kata-deploy" is invalid: spec.template.spec.containers[0].volumeMounts[2].mountPath: Invalid value: "/etc/containerd/": must be unique

I might be possible to mount individual files (including backups) as a workaround, but that seems messier than how the volumes are currently mounted

This comment has been minimized.

Copy link
@egernst

egernst Nov 26, 2019

Collaborator

Well, I was originally thinking there'd be a kata-deploy-in-k3s.yaml which is just about the same, only utilizing the k3s path rather than the standard /etc/containerd/ path.

This would leave the bash part of it generic.

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Nov 27, 2019

Author Contributor

Latest changes are set up to use kustomize as an alternative to maintaining multiple kata-deploy*.yaml files.

@egernst egernst added the wip label Nov 26, 2019
@egernst

This comment has been minimized.

Copy link
Collaborator

egernst commented Nov 26, 2019

Just because that Merge button is big and green, I wanted to add WIP label as we continue the conversation. Thanks @wilsonianb

wilsonianb added 2 commits Nov 27, 2019
Signed-off-by: Brandon Wilson <brandon@coil.com>
By default, k3s uses an embedded containerd. Reconfiguring this
containerd requires modifying a template config file and restarting the
k3s (master node) or k3s-agent (worker node) systemd service.

Signed-off-by: Brandon Wilson <brandon@coil.com>
@wilsonianb wilsonianb force-pushed the wilsonianb:k3s branch from be1c1e8 to 9e716ae Nov 27, 2019
@egernst
egernst approved these changes Dec 2, 2019
Copy link
Collaborator

egernst left a comment

This looks much better now, @wilsonianb -- thanks for introducing the Kustomization.

LGTM, and will start working on adding proper CI for this. Unless you wanted to do this as well :)

@egernst egernst removed the wip label Dec 2, 2019
@egernst

This comment has been minimized.

Copy link
Collaborator

egernst commented Dec 2, 2019

/AzurePipelines run

@egernst

This comment has been minimized.

Copy link
Collaborator

egernst commented Dec 2, 2019

/test

@azure-pipelines

This comment has been minimized.

Copy link

azure-pipelines bot commented Dec 2, 2019

Azure Pipelines successfully started running 2 pipeline(s).
@egernst

This comment has been minimized.

Copy link
Collaborator

egernst commented Dec 3, 2019

Tested the changes to verify existing targets don't fail (using #844). While we still need tests (see #843), happy to have this included.

@egernst egernst merged commit 6318f0a into kata-containers:master Dec 3, 2019
6 checks passed
6 checks passed
jenkins-ci-ubuntu-18-04 Build finished.
Details
jenkins-ci-ubuntu-18-04-snap Build finished.
Details
kata-containers-release #20191202.1 succeeded
Details
kata-containers/SoB-check SoB-check status: success
Details
kata-containers/WIP-check WIP-check status: success
Details
obs-packaging-ci #20191202.1 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.