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

fix(node-installer): corrects the k3s distribution check #43

Merged
merged 1 commit into from
Mar 20, 2024

Conversation

adamreese
Copy link
Member

This commit changes how k3s is detected before restarting containerd. In some cases the /etc/init.d/k3s directory won't exist when running k3s. The ordering is also important because containerd can be run in parallel to k3s.

COPY script/installer.sh /script/installer.sh
COPY ./.tmp/${TARGETPLATFORM} /assets
COPY ./.tmp /assets
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not copy assets for the target platform?

Copy link
Member Author

Choose a reason for hiding this comment

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

When building only a single binary was created and ./.tmp/${TARGETPLATFORM} didn't exist. The binary was ./.tmp/containerd-shiim-spin. There might be a proper fix but I wasn't sure what the expected behavior was.

Copy link
Member Author

@adamreese adamreese Mar 14, 2024

Choose a reason for hiding this comment

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

Did some investigating. It looks like GH actions doesn't use this Makefile to build the node-installer image. I'll remove this and create a proper fix in another PR.
https://github.com/adamreese/containerd-shim-spin/blob/4e9378e241321506bdfc674cf008c35ab87fd2da/.github/workflows/node-installer.yaml#L50-L55

Copy link
Collaborator

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

I am not well versed in the node installer, but these changes did work for me while the previous didn't probably in part to systemctl not being at /bin/systemctl in my system. @adamreese I wonder for more feedback if it would make sense to PR this into https://github.com/KWasm/kwasm-node-installer/blob/main/script/installer.sh. This was copied here for our CI but i think it will likely pull in changes from the kwasm-node-installer

This commit changes how k3s is detected before restarting containerd. In
some cases the `/etc/init.d/k3s` directory won't exist when running k3s.
The ordering is also important because containerd can be run in
parallel to k3s.

Signed-off-by: Adam Reese <[email protected]>
@adamreese adamreese force-pushed the fix/k3s-node-installer branch from 4e9378e to 9f882c0 Compare March 14, 2024 16:17
@kate-goldenring kate-goldenring merged commit c63749f into spinkube:main Mar 20, 2024
9 checks passed
@adamreese adamreese deleted the fix/k3s-node-installer branch March 23, 2024 03:38
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