Make the containerd factory configurable

[0902horn]

1. Change the default value of command line flag --containerd to '/run/containerd/containerd.sock'.
2. Add a command line flag --containerd-namespace defaulting to 'k8s.io'.
3. Create containerd client with values from --containerd and --containerd-namespace.

[k8s-ci-robot]

Hi @0902horn. Thanks for your PR.

I'm waiting for a google or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[0902horn]

This PR fixes #2082 .

[dashpole]

/ok-to-test

cc @Random-Liu

[dashpole]

is it possible to get containers from all namespaces? Just curious

[0902horn]

is it possible to get containers from all namespaces? Just curious

According to comments of containerd namespaces package, I think getting containers across namespaces could confuse cAdvisor or other applications that rely on metrics provided by cAdvisor, since identifiers in different containerd namespaces might be the same and these namespaces are not reflected by metrics.

[dashpole]

@0902horn that makes sense. I was just curious if there was a way (e.g. empty string) to get everything if I wanted to. Since cAdvisor is a cgroup monitor, it will end up collecting metrics for those containers anyways. It just wouldn't attach metadata (e.g. container name) for those. I am fine with the change without that, but I wouldn't be surprised if we get a feature request for being able to do that eventually.

[0902horn]

@dashpole Thanks for your replies.
It seems reasonable to get such a feature. So I did a little further work on this. The metric tag named 'id' is a cgroup path actually, so I guess cAdvisor would be fine if it gets containers from all namespaces. However, I found that 'ctr' requires a namespace (default to 'default').

#ctr --namespace= container ls
ctr: namespace is required: failed precondition

I confirmed that this is by design by reading the doc.
When I specified '--containerd-namespace=' on cAdvisor built from this PR, I got this error message:

I0326 06:35:34.698090       1 factory.go:109] Error trying to work out if we can handle /kubepods.slice/kubepods-besteffort.slice/kubepods-besteffort-pod149154bb_4c7b_11e9_beec_00163e041f4b.slice/a9d2267854302a842e9dd52b70f658c5ebcbab1e338f8e88e22d7c0ff462a89f: failed to load container: namespace is required: failed precondition

So a possible way is listing all namespaces and creating multiple clients with each to query containers.
If the feature is requested, I would like to contribute another PR.

[dashpole]

Thanks a bunch for the investigation. In that case, just supporting a single namespace seems like the correct behavior. I'll do a review in the next few days

[dashpole]

lgtm

[dashpole]

FYI @Random-Liu

[dashpole]

wait, does this still work if it isn't a unix socket?

[0902horn]

It has to be a unix socket because the original implementation of 'func Client' has following code (client.go#L58):

tryConn, err := net.DialTimeout("unix", address, connectionTimeout)

I think it is by design for security and simplicity. See this containerd issue containerd/containerd#1324.

[dashpole]

I meant to ask if this will still connect to the unix socket if you remove the unix:// prefix

[0902horn]

Oh...I thought you were asking TCP or something.
ArgContainerdEndpoint was not used before, so don't worry about changing its value's format. Now it is used and requires such format without unix:// prefix.

[0902horn]

I'm happily seeing this PR is merged. Thanks @dashpole