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.

Sign up for GitHub

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

testing: Fix VerifyKubernetesImages on Kubernetes versions before v1.24 with docker cruntime #17647

Merged
merged 3 commits into from
Nov 28, 2023
Merged

testing: Fix VerifyKubernetesImages on Kubernetes versions before v1.24 with docker cruntime #17647

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b05b464
handle kubernetes versions before v1.24 with docker as container runtime
prezha Nov 18, 2023
44821c0
use 'minikube image list' instead of 'crictl images'
prezha Nov 21, 2023
251fdc5
todo: switch from 'minikube image list' to 'crictl images'
prezha Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion test/integration/start_stop_delete_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

@spowelljr spowelljr Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ;)


// handle old kubernetes versions (before v1.24) with docker as container runtime
// avoid 'validate service connection: validate CRI v1 image API for endpoint "unix:///var/run/dockershim.sock": rpc error: code = Unimplemented desc = unknown service runtime.v1.ImageService' error
// as newer crictl needs cri-dockerd.sock instead of dockershim.sock
// and if unspecified, crictl will try dockershim.sock first and cri-dockerd.sock last
// ref: https://github.com/shannonxtreme/cri-tools/blob/17484cda811c93b69e61448835db9559c7f3ab9c/cmd/crictl/main_unix.go#L30
if v, _ := util.ParseKubernetesVersion(version); v.LT(semver.Version{Major: 1, Minor: 24}) {
rr, err := Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "kubectl", "--ssh", "--", "get nodes -o wide"))
if err == nil && strings.Contains(rr.Stdout.String(), "docker://") {
cmd = "sudo crictl --runtime-endpoint unix:///var/run/cri-dockerd.sock images -o json"
// try to ensure active "cri-docker.socket", fallthrough on error
_, _ = Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "ssh", "sudo", "systemctl", "restart", "cri-docker.socket"))
}
}

rr, err := Run(t, exec.CommandContext(ctx, Target(), "ssh", "-p", profile, cmd))
if err != nil {
t.Errorf("failed to get images inside minikube. args %q: %v", rr.Command(), err)
}
Expand Down
Loading