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

crictl testing #983

Closed
chuckha opened this issue Jul 10, 2018 · 24 comments
Closed

crictl testing #983

chuckha opened this issue Jul 10, 2018 · 24 comments
Assignees
Labels
area/test kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Milestone

Comments

@chuckha
Copy link

chuckha commented Jul 10, 2018

crictl is not part of the test suite for kubeadm. The test suite only uses docker (?) and therefore the crictl code paths in kubeadm are not exercised.

We should hold off integrating crictl further until we have a better test framework to support the changes.

Edit:

This ticket is regarding a gap in the upstream blocking tests. We need to have automated tests in place that exercise kubeadm using docker and crictl.

As stated by others below, we cannot rely on only crictl because we cannot depend on the dockershim being available. The shim is provided by the kubelet and the kubelet is not always available.

@chuckha chuckha added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. area/test labels Jul 10, 2018
@chuckha chuckha added this to the v1.12 milestone Jul 10, 2018
@timothysc timothysc added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Jul 10, 2018
@timothysc timothysc removed this from the v1.12 milestone Jul 10, 2018
@timothysc timothysc added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 10, 2018
@yujuhong
Copy link

I'd like to propose an alternative. We can always use cri socket +crictl to interact with the runtime (when needed), and deprecate the special docker code path.
This simplifies the testing flow significantly, since Docker will no longer be a special case. The one downside is crictl will be a prerequisite for using kubeadm, but the benefits still outweigh the cost to me. Of course, the documentation, etc., will also need to be updated.

I have an example PR here: kubernetes/kubernetes#66415 If folks think this is reasonable, I will work on getting the PR merged.

@timothysc @luxas
/cc @Random-Liu

@bart0sh
Copy link

bart0sh commented Jul 20, 2018

@yujuhong did you try if it works with docker this way? Which socket to use for that? Which versions of docker support this?

@bart0sh
Copy link

bart0sh commented Jul 20, 2018

/cc @kad

@yujuhong
Copy link

@bart0sh the CRI piece for docker is in kubelet (pkg/kubelet/dockershim). If it's an okay assumption that kubelet will be running when talking docker, this should work. I'll need to look into kubeadm code to see if the assumption holds true.

@bart0sh
Copy link

bart0sh commented Jul 23, 2018

@yujuhong As far as I know you can't assume that dockershim socket exists and kubelet is running for all kubeadm modes.

btw, current code already supports working with the docker through dockershim socket. You don't need to remove docker support to achieve that. Just use --cri-socket unix:///var/run/dockershim.sock in the kubeadm command line.

@yujuhong
Copy link

@yujuhong As far as I know you can't assume that dockershim socket exists and kubelet is running for all kubeadm modes.

Hmm....can you point me to the relevant bits where kubeadm needs to talk to docker while kubelet is absent? Thanks!

btw, current code already supports working with the docker through dockershim socket. You don't need to remove docker support to achieve that. Just use --cri-socket unix:///var/run/dockershim.sock in the kubeadm command line.

Yes, but supporting both modes for docker would likely lead to insufficient testing for one of them. The main point of the proposal is to force docker testing goes through the same code path as other runtimes.

@bart0sh
Copy link

bart0sh commented Jul 24, 2018

Hmm....can you point me to the relevant bits where kubeadm needs to talk to docker while kubelet is absent? Thanks!

Sure. Here are 2 use cases:

  1. resetting master:
$ sudo ./kubeadm reset --cri-socket unix:///var/run/dockershim.sock
[reset] WARNING: changes made to this host by 'kubeadm init' or 'kubeadm join' will be reverted.
[reset] are you sure you want to proceed? [y/N]: y
[preflight] running pre-flight checks
[reset] stopping the kubelet service
[reset] unmounting mounted directories in "/var/lib/kubelet"
E0724 12:08:54.205050   16783 reset.go:153] [reset] failed to remove containers: output: 2018/07/24 12:08:54 grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: dial unix /var/run/dockershim.sock: connect: no such file or directory"; Reconnecting to {/var/run/dockershim.sock <nil>}
time="2018-07-24T12:08:54+03:00" level=fatal msg="listing pod sandboxes failed: rpc error: code = Unavailable desc = grpc: the connection is unavailable" 
, error: exit status 
  1. pulling images:
$ sudo ./kubeadm config images pull --cri-socket-path unix:///var/run/dockershim.sock
failed to pull image "k8s.gcr.io/kube-apiserver-amd64:v1.11.1": output: 2018/07/24 12:10:13 grpc: addrConn.resetTransport failed to create client transport: connection error: desc = "transport: dial unix /var/run/dockershim.sock: connect: no such file or directory"; Reconnecting to {/var/run/dockershim.sock <nil>}
time="2018-07-24T12:10:13+03:00" level=fatal msg="pulling image failed: rpc error: code = Unavailable desc = grpc: the connection is unavailable" 
, error: exit status 1

Both cases work with docker just fine.

@luxas
Copy link
Member

luxas commented Jul 24, 2018

Yeah it's the dependency of requiring the kubelet to be running in order for dockershim to be run that is problematic. If that wouldn't be the case we could directly switch over, but I don't think so with the current architecture...

@yujuhong
Copy link

@bart0sh @luxas thanks for the reply! I'd really like to see dockershim being tested the same as other CRI runtimes, but looks like this is not possible today. Will keep an eye one this and see if we can improve it in the future.

@bart0sh
Copy link

bart0sh commented Jul 31, 2018

@yujuhong Well, it's not possible for all kubeadm modes, but we can start with what's possible.

Another thought: It would be nice to have most frequently used CRI implementations (CRI-O and containerd) tested. Correct me if I'm wrong but it seems to me that dockershim is a temporary solution, i.e. it will not be needed anymore when all runtimes implement CRI.

@timothysc
Copy link
Member

/assign @bart0sh @rosti

@timothysc timothysc removed the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Oct 11, 2018
@timothysc timothysc added this to the v1.13 milestone Oct 11, 2018
@bart0sh
Copy link

bart0sh commented Oct 12, 2018

Can anybody point me to the testing code where crictl tests should be implemented?

@timothysc timothysc modified the milestones: v1.13, v1.14 Nov 14, 2018
@bart0sh
Copy link

bart0sh commented Feb 14, 2019

second attempt: Can anybody point me to the testing code where crictl tests should be implemented, please?

@neolit123
Copy link
Member

reading the OP by @chuckha again.

i'm going to assign you to this as well but this is going to be very tricky @bart0sh
/assign @bart0sh

i don't have the bandwidth for this, this cycle and also currently i don't have all the details on how we should approach this for crictl and non-docker.

@neolit123 neolit123 modified the milestones: v1.14, v1.15 Feb 14, 2019
@bart0sh
Copy link

bart0sh commented Feb 14, 2019

@neolit123 I was hoping to use Kind for this. Is it reasonable hope?

I still don't have a clue where this should be implemented. My current guess is test/e2e_kubeadm, but I'm still not sure.

@neolit123
Copy link
Member

neolit123 commented Feb 14, 2019

kind currently only supports Docker as the CR on the nodes it creates.
i think it's out of scope for kind 1.0 (to be released with k8s 1.14) to support anything else.

if this is urgent for your company you can try patching kind to support crictl?
in terms of sig-cluster-lifecycle and kubeadm this is tricky...

we might bootleg a solution next cycle that runs crictl on a single control plane / kubeadm setup inside a prow pod (without kind). i cannot help much here at this point (for now).

@bart0sh
Copy link

bart0sh commented Feb 14, 2019

if this is urgent for your company you can try patching kind to support crictl?

Nope, not urgent. I'm just trying to understand how to test kubeadm with CRI-O runtime as I'm assigned to this issue.

@bart0sh
Copy link

bart0sh commented Feb 20, 2019

Looks like at least some work related to this is ongoing in Kind.
I'm going to monitor these 2 issues:
kubernetes-sigs/kind#153
kubernetes-sigs/kind#154

@BenTheElder
Copy link
Member

I'd like to make it possible to use another CRI within the nodes in some form (alternate images possibly) long term for the sole reason that we should be able to change it out as needed / we should probably get on the CRI paths ourselves eventually.

CRIs are generally probably best tested on VMs or bare metal, but just for integration testing kubeadm + the CRI path, kind will probably reasonable in the not terribly distant future.

@bart0sh
Copy link

bart0sh commented Feb 25, 2019

@BenTheElder That sounds great. Which CRI are you going to start from? I'd recommend CRI-O if possible. I can help with testing.

@BenTheElder
Copy link
Member

I actually intended to do containerd first as that aligns with our existing experience (we know a lot about running docker in containers, and by extension the core), and with recent Docker we can hopefully do a gradual migration from dockershim -> cri-containerd without even changing the node images significantly since Docker now ships with containerd underneath.

A few users already depend on docker existing and I'm not sure how quickly we can break that 😬

I've spoken with @Random-Liu and a few others about this a bit.. There's even some tentative discussion from @dims about docker -> using the underlying containerd being a path to not need the built-in dockershim going forward.

Most all of Kubernetes's CI runs on dockershim still at the moment, for better or worse (probably worse? 😛).

CRI-O is another very obvious choice to look at, but to my knowledge there's not much out there (roughly zero?) about CRI-O in containers so we'll have to figure out what the quirks are ourselves and how to manage shipping it in the image, so I see that as a next step. 🙏

Similarly we plan to support podman for the node containers on the host, but there's a lot less out there about doing things like this and I expect quirks that will need debugging so we're landing that soonish after we flesh things out using Docker. 😅

@rosti
Copy link

rosti commented Feb 25, 2019

@BenTheElder @bart0sh As I outlined in the aforementioned email, it is possible with Docker 18.09 and a tiny change to /etc/containerd/config.toml. We need to comment out the following line:

disabled_plugins = ["cri"]

and then restart containerd.

However, as dockerd and the CRI plugin of containerd work in different containerd namespaces, there won't be any resource sharing between those two (pre-pulled/loaded images, pre-started containers, etc. won't be seen on the CRI side if they were initiated via dockerd).

@neolit123
Copy link
Member

with kind moving to containerd, we are now going to lack dockershim tests on the kubeadm nodes.
then again, we are testing k8s + kubeadm boostrap and the CRI testing itself should be on the sig-node side.

IMO, this ticket can be soon closed.

@neolit123
Copy link
Member

upstream kind (used in the kubeadm-kind, not kinder jobs) is already using crictl.
logged issue for docker e2e jobs: #1592

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test kind/feature Categorizes issue or PR as related to a new feature. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

No branches or pull requests

8 participants