Add command 'k0s ctr [command] [flags] [args]'

[jewertow]

Issue
Fixes #856

What this PR Includes
This PR adds command ctr, which is containerd CLI.

[jewertow]

If you want to verify if it works, execute the commands below to see that subcommands, flags and arguments work properly.

k0s ctr images list
k0s ctr image pull docker.io/calico/node:v3.11.2
k0s ctr images label docker.io/calico/node:v3.11.2 app=cni
k0s ctr images label --replace-all docker.io/calico/node:v3.11.2 cni=calico

[jasmingacic]

Can you please run go mod tidy to make CI pass?

[jewertow]

Go build / Smoke test for BYO CRI feature failed but it seems that it is not related to my change. Could you retry this stage?

[jasmingacic]

I'm trying to pass this

# ./k0s ctr -a /run/containerd/containerd.sock i list 
Error: unknown shorthand flag: 'a' in -a

where as ctr is accepting -a --address flag

# ctr  -a /run/containerd/containerd.sock i list
REF TYPE DIGEST SIZE PLATFORMS LABELS 

[jewertow]

I'm sorry. I know what is the problem.

[jasmingacic]

One more thing ctr should have default values for address something like path.Join(k.K0sVars.RunDir, "containerd.sock") and default namespace set to k8s.io so we would be able to do k0s ctr c ls out of the box without the need to do k0s ctr --address /run/k0s/containerd.sock -n k8s.io c ls.
Also the users should be able to override these values by passing their desired values.

Does this make sense?

[jewertow]

Ok, thanks for the feedback. I will fix it soon.

[jasmingacic]

@jewertow also it would be good to have some sort of integration tests for such scenarios which would allow us to check if the tool has been included entirely.

[jewertow]

@jasmingacic does it make sense to set default namespace to k8s.io? The default behaviour of containerd is:

1. Use value of CONTAINERD_NAMESPACE env var if it' set.
2. Use namespace default.

Maybe it would be better to not set namespace and relay on default behaviour of the ctr?

[jasmingacic]

The ctr in this case will be used only for k0s purposes so defaulting to namespace k8s.io where all containers reside makes sense. So we should default to k8s.io basically we already do here https://github.com/k0sproject/k0s/blob/main/pkg/component/worker/ocibundle.go#L46

[jewertow]

@jasmingacic I significantly simplified the implementation. Now it fully relies on the original implementation.

[jewertow]

After changing implementation entirely, do you still see the need to add integration tests?
I think the current implementation is trivial and I don't see corner cases.

[kke]

After changing implementation entirely, do you still see the need to add integration tests?
I think the current implementation is trivial and I don't see corner cases.

I don't see anything worth unit testing in it now, there's hardly any "units" to test. LGTM.

A pretty simple "smoke test" case would be to just add some k0s ctr command to one of the integration tests (such as inttest/install/singlenode_test.go) that would just run k0s ctr images list | grep -q something-its-supposed-to-include and error out if it fails.

[jasmingacic]

Both stand alone ctr --address /run/k0s/containerd.sock -n k8s.io c ls and k0s ctr c ls produce the same output.

Can you please add integration test as @kke proposed here, so we can merge this.

[jewertow]

Yes, I will add a test soon.

[jewertow]

@jasmingacic this failed CI plan looks like random failure, because it's not related to my changes and the test I added works fine. Could you trigger failed tests or run CI plan once again?

[jasmingacic]

@jewertow The CI issue is due to some changes can you sync your fork with the upstream.

[jasmingacic]

@jewertow this is looking good.

@k0sproject/k0s-core can someone else take a look too especially to the GH action changes.

[kke]

If you change this to k0s start we can test the #896 at the same time :)

Suggested change

	_, err = ssh.ExecWithOutput("rc-service k0scontroller start")
	_, err = ssh.ExecWithOutput("k0s start")

[jewertow]

Thanks. You're right, but the command re-service k0scontroller start/stop is also used in other tests, so we could change all of them to k0s start/stop in another pull request, because it's not related to the issue. What do you think?

[jasmingacic]

I'd rather that we don't touch other tests. Let's scope this PR to ctr only.

[jewertow]

Ok, done

[jasmingacic]

Great work @jewertow thank you