testing: Fix VerifyKubernetesImages on Kubernetes versions before v1.24 with docker cruntime #17647
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: prezha The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The cri-tools and cni-plugins versions need to be modified, to match the kubernetes version. This was a change made in 1.28 (to fork the packages per release), but also applies to older... |
spowelljr
changed the title
| @@ -352,7 +352,23 @@ func testPulledImages(ctx context.Context, t *testing.T, profile, version string | |||
| t.Helper() | |||
| defer PostMortemLogs(t, profile) | |||
|
|
|||
| rr, err := Run(t, exec.CommandContext(ctx, Target(), "ssh", "-p", profile, "sudo crictl images -o json")) | |||
| cmd := "sudo crictl images -o json" | |||
There was a problem hiding this comment.
What do you think about using minikube image list --format=json instead? We're kind of hacking behind the scenes when this functionality is already built into minikube. The command will use the cruntime to get the image list and will prevent a lot of this complication.
There was a problem hiding this comment.
@spowelljr thanks for the review and comment!
my thinking was:
i wanted to make a minimal change in the hope that we'll deprecate dockershim at some point soon and then just delete this "workaround" block (as part of a bigger refactor/pruning of related code)
i'd also want to avoid testing a minikube functionality by using another minikube functionality (the "image list" subcommand in this case) as if a test fails, it would not be so obvious what failed specifically and potentially make debugging a bit harder
then i thought about using docker images (being the cruntime in this case) directly, but that would require output processing similar to what we do in the minikube image list, effectively duplicating that, which made little sense
so, initially, i opted to just keep the crictl images that we use in this test...
but, at the end of the day, these are integration tests, so it wouldn't harm much i guess (as long as we keep in mind the above), so i amended the pr using minikube image list instead :)
There was a problem hiding this comment.
local test results:
$ make integration -e TEST_ARGS="-minikube-start-args='--driver=kvm2 --container-runtime=docker --alsologtostderr -v=7' -test.run TestStartStop/group/old-k8s-version --cleanup=false"
...
--- PASS: TestStartStop (638.84s)
--- PASS: TestStartStop/group (0.00s)
--- PASS: TestStartStop/group/old-k8s-version (638.84s)
--- PASS: TestStartStop/group/old-k8s-version/serial (638.84s)
--- PASS: TestStartStop/group/old-k8s-version/serial/FirstStart (160.85s)
--- PASS: TestStartStop/group/old-k8s-version/serial/DeployApp (9.26s)
--- PASS: TestStartStop/group/old-k8s-version/serial/EnableAddonWhileActive (0.78s)
--- PASS: TestStartStop/group/old-k8s-version/serial/Stop (12.21s)
--- PASS: TestStartStop/group/old-k8s-version/serial/EnableAddonAfterStop (0.21s)
--- PASS: TestStartStop/group/old-k8s-version/serial/SecondStart (442.66s)
--- PASS: TestStartStop/group/old-k8s-version/serial/UserAppExistsAfterStop (5.02s)
--- PASS: TestStartStop/group/old-k8s-version/serial/AddonExistsAfterStop (5.08s)
--- PASS: TestStartStop/group/old-k8s-version/serial/VerifyKubernetesImages (0.25s)
--- PASS: TestStartStop/group/old-k8s-version/serial/Pause (2.53s)
PASS
Tests completed in 10m38.840530637s (result code 0)
ok k8s.io/minikube/test/integration 638.871s
There was a problem hiding this comment.
Not relying on another minikube command is valid, I'm fine with either solution. Once we bump the minimum minikube version I'm fine reverting this PR as well. But if you're happy with it in its current state I'm happy to merge it
There was a problem hiding this comment.
sure - both would work
i think we can stick with the minikube image list for now and switch back to the crictl images once we bump the minimum supported k8s version of 1.24+
i've added a TODO comment there, so we don't forget - i think we're good to go with this one ;)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
kvm2 driver with docker runtime Times for minikube start: 50.9s 48.2s 51.0s 51.4s 49.9s Times for minikube ingress: 23.1s 27.1s 24.6s 26.7s 28.2s docker driver with docker runtime Times for minikube start: 24.6s 22.5s 22.3s 25.3s 21.1s Times for minikube ingress: 20.8s 20.8s 18.8s 20.8s 20.8s docker driver with containerd runtime Times for minikube start: 24.1s 23.7s 23.8s 20.5s 22.6s Times for minikube ingress: 31.3s 31.3s 31.3s 31.3s 31.3s |
|
These are the flake rates of all failed tests.
To see the flake rates of all tests by environment, click here. |
|
thanks @prezha |
fixes #17646
TestStartStop/old-k8s-version/VerifyKubernetesImages uses constants.OldestKubernetesVersion, that is currently v1.16.0, so if docker is used as the container runtime, it would imply using dockershim, in which case, as described in the issue, crictl fails to list images:
why?
we're currently using crictl version v1.28.0 and cri-tools switched to cri v1 api in v1.24.0
on a side note, based on the compatibility matrix, we can expect issues with versions mismatch:
this pr should fix the tests for (really) old k8s versions, but we should also consider bumping the oldest version from the current v1.16 (i think we aim to support the last six versions?) or perhaps try to hold on until v1.29 and then start removing everything docker-related by supporting only k8s v1.24+
after
local test run: