-
Notifications
You must be signed in to change notification settings - Fork 31
feat: add support for k0s #124
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for the PR! Looks good on a first skim, I'll validate this later and do a full review. Eventually it would be very nice if we had some way to validate the installer in e2e tests for all the different distros. But no need for you to add anything for now, I'll give this a try at some point later. |
|
Yes, also I was thinking also maybe we should have different file like distros/generic distros/k0s etc... so we can add more with time. I wanted to add for https://github.com/portainer/kubesolo, but i want to see for this PR first. |
|
@ctrox can you allow builds for this so I can test? |
I approved the checks and also pushed the branch myself as PRs can't trigger image pushes (would be a bit problematic if they could). |
|
I just tested this, when the manager started up it crashed out failing to find PEM data in the certificate input. UPDATE: scratch that, I'd deployed it in the wrong namespace and it always expects zeropod-system and I'd deployed it in zeropod |
ctrox
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the installer on a fresh k0s cluster, nice work! Just a few comments regarding the path handling which could be simplified.
| func containerdSocketPath(runtime containerRuntime) string { | ||
| switch runtime { | ||
| case runtimeK0S: | ||
| return k0sContainerdSock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of changing the socket path, wouldn't it be simpler to only change the hostPath and mount the socket into /run/containerd inside the container? We also do this for k3s where the containerd socket is not in the standard path on the host. Same for the etc containerd dir.
| initContainers: | ||
| - name: installer | ||
| volumeMounts: | ||
| - name: containerd-etc | ||
| mountPath: /etc/k0s | ||
| - name: containerd-run | ||
| mountPath: /run/k0s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would not be needed if the installer would just look for the socket in /run/containerd as for the other distros.
| containers: | ||
| - name: manager | ||
| volumeMounts: | ||
| - name: containerd-etc | ||
| mountPath: /etc/k0s | ||
| - name: containerd-run | ||
| mountPath: /run/k0s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed, the manager does not need to access the containerd config/socket.
| containerdv1AlreadyConfigured = fullContainerdConfigV2 + runtimeConfig + ` | ||
| ) | ||
|
|
||
| var containerdv1AlreadyConfigured = fullContainerdConfigV2 + fmt.Sprintf(runtimeConfig, strings.TrimSuffix(defaultOptPath, "/"), true) + ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason this can't be in the existing var block?
|
Will address the changes :) |
Oh would be nice if that would work out of the box. Created #127 to address this. |
fixes #122