Skip to content

Conversation

@vdice
Copy link
Collaborator

@vdice vdice commented Nov 15, 2024

Describe your changes

This PR was created to address the following issue:

Context

There may be scenarios where both a default containerd file exists (eg at /etc/containerd/config.toml) as well as a K8s-distro specific location (eg at /etc/k0s/containerd.toml for K0s). A common example case is installing a specific K8s distro directly on to a host (VM or otherwise) that already has the system containerd installed.

Issue

In the original distro detection logic, a map is used to see if a containerd config exists at a known/supported location; if it does, the distro is determined accordingly. However, maps in Golang are unordered. This means the default config may be returned prior to recognizing that there is a distro-specific config also on the host (eg K0s, MicroK8s, etc).

Proposed solution

In this PR, I've split out the default location into its own const, leaving the map to contain only distro-specific locations. We perform a lookup in this distro-specific map and if we fail to find a config with these choices, we finally try the default location.

Alternate

I'm open to alternate approaches. Use of a third-party ordered map didn't look as nice (more code/setup). We could also iterate over a slice, which is ordered/deterministic, but again would require a bit more code (at least another data structure).

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests.
  • I tested the changes with the following distributions:
    • Kind
    • MiniKube
    • MicroK8s
    • K3s
    • K0s
    • Rancher RKE2
    • Azure AKS
    • GCP GKE (Ubuntu nodes)
    • AWS EKS (AmazonLinux2 nodes)
    • AWS EKS (Ubuntu nodes)
    • Digital Ocean Kubernetes

@github-actions github-actions bot added the kind/bug Something isn't working label Nov 15, 2024
Copy link
Member

@voigt voigt left a comment

Choose a reason for hiding this comment

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

Your change looks fine for me, only thing left to fix is the s/kwasm/rcm/ naming thing...

@voigt voigt force-pushed the fix/containerd-config-default-last branch from e2abd2a to abac4e8 Compare March 17, 2025 11:58
@voigt voigt merged commit ba8319e into spinframework:main Mar 17, 2025
7 checks passed
@vdice vdice deleted the fix/containerd-config-default-last branch March 17, 2025 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants