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

Set K8s distribution with Helm to simplify choosing container runtime socket #427

Merged
merged 15 commits into from
Dec 14, 2021

Conversation

kate-goldenring
Copy link
Contributor

@kate-goldenring kate-goldenring commented Nov 23, 2021

What this PR does / why we need it:
Now that we have removed the need to specify a crictl path #418, all that needs to be set when installing Akri is the container runtime socket, which is set with --set agent.host.containerRuntimeSock="/some/sock.sock. This varies by distribution. To simplify installation and Kubernetes cluster setup, this proposes providing a kubernetesDistro value. Simply set kubernetesDistro=$DISTRO, where $DISTRO is either k3s, microk8s, or k8s. The template will always preference agent.host.containerRuntimeSocket if set. Chart throws the following error if no distribution nor socket are set:
Please set container runtime socket by either selecting the appropriate K8s distro kubernetesDistro=<k8s|k3s|microk8s>or settingagent.host.containerRuntimeSocket=/container/runtime.sock. See https://docs.akri.sh/user-guide/cluster-setup for more information."

Alternatives:
If adding kuberntesDistro is more confusing than beneficial, just retain the agent.host.containerRuntimeSock value and set it per usual in the docs, just renaming to be associated with container runtime. These docs will need to be modified to describe setting a distro during installation anyways.

Docs to modify: PR project-akri/akri-docs#19

If applicable:

  • this PR has an associated PR with documentation in akri-docs
  • added code adheres to standard Rust formatting (cargo fmt)
  • version has been updated appropriately (./version.sh)
  • all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

# kubernetesDistro describes the Kubernetes distro Akri is running on. It is used to conditionally set
# distribution specific values such as container runtime socket.
kubernetesDistro:
microk8s: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think i'd lean towards using kubernetesDistro==k8s | kubernetesDistro==microk8s | kubernetesDistro==k3s ... rather than kubernetesDistro.microk8s==true, etc.

seems like maybe a cleaner way to make sure one and only one distro is ever specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was worried about spelling but this makes sense. Changed it all!

Copy link
Contributor

@romoh romoh left a comment

Choose a reason for hiding this comment

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

Should we default to some value? like if not set then k8s is picked.

@kate-goldenring
Copy link
Contributor Author

Should we default to some value? like if not set then k8s is picked.

My thought is no. It seems better to error and let them decide their distro. Otherwise slot reconciliation may not work. i know this is different from what we were doing but it would reduce the number of issues folks have with slot reconciliation errors.

@kate-goldenring
Copy link
Contributor Author

Associated documentation: project-akri/akri-docs#19

@jiria
Copy link
Contributor

jiria commented Dec 3, 2021

Please update the PR message with the updated design.

{{- else if eq .Values.kubernetesDistro "k3s" }}
path: "/run/k3s/containerd/containerd.sock"
{{- else if eq .Values.kubernetesDistro "k8s" }}
path: "/var/run/dockershim.sock"
Copy link
Contributor

Choose a reason for hiding this comment

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

What if folks use containerd instead of dockershim?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then they can set agent.host.containerRuntimeSocket to whatever they want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change in documentation should clarify that project-akri/akri-docs#19

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to then use k8s-dockershim to reduce confusion? Otherwise folks with containerd might be surprised why it is not working. Just FYI, AKS has transitioned from dockershim to containerd, the future lies with containerd, not dockershim.

Copy link
Contributor Author

@kate-goldenring kate-goldenring Dec 3, 2021

Choose a reason for hiding this comment

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

in that case i think it would be better to do the alternative i describe in the PR description. Namely, we would provide no kubernetesDistro=$DISTRO_NAME and instead recommend how to set agent.host.containerRuntimeSocket based on the distro. I think that would be better than providing multiple options per distro (k8s-containerd, k8s-dockershim, etc). @bfjelds and @romoh on thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jiria @romoh @bfjelds bump here. Would be nice to get the documentation in one this once we decide on what to do here

Copy link
Collaborator

Choose a reason for hiding this comment

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

i would say have distro=k8s|k3s|etc ... then use whatever the distro default is (dockershim or containerd) ... and document that anything that doesn't follow the defaults can be supported iwth containerRuntimeSocket. though i also have no problem with k8s-dockershim|k8s-containerd|etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going with brian's last comment and sticking with what we have here. If folks complain about wanting our k8s default changed, we can change it. And I'll make sure the documentation points out how to set it to containerd.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

@kate-goldenring
Copy link
Contributor Author

@romoh @bfjelds instead of erroring out, I set k8s' docker as the default. I could not throw a warning with helm, but i commented in the chart manifest that the default is being used: 161cdfd

{{- else if eq .Values.kubernetesDistro "k3s" }}
path: "/run/k3s/containerd/containerd.sock"
{{- else if eq .Values.kubernetesDistro "k8s" }}
path: "/var/run/dockershim.sock"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

@kate-goldenring kate-goldenring merged commit 1d84777 into project-akri:main Dec 14, 2021
@kate-goldenring kate-goldenring deleted the configure-distros branch December 14, 2021 21:20
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.

4 participants