crio: filter out systemd related components

[rphillips]

In Openshift we are seeing systemd components get added to housekeeping within the Kubelet using crio. This patch filters the system-systemd namespace for containers.

cc @harche

[k8s-ci-robot]

Hi @rphillips. Thanks for your PR.

I'm waiting for a google 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.

[bobbypage]

/ok-to-test

[harche]

@rphillips Thanks for this PR. I presume this PR is trying to fix the cadvisor related issue reported in https://bugzilla.redhat.com/show_bug.cgi?id=1978528

This fix looks good, but not sure if I got it entirely correctly or I am missing something. This fix seem to add an awareness in the crio factory that it cannot handle and accept paths with system-systemd. But in the logs of that bug the error seems to be coming from the raw factory and not crio factory.

If you look at the way suitable factory is selected, it just iterates over all existing implementations of that interface. So would it be possible that even after having this fix in place, which would make crio factory return with false for canHandle but then raw factory to still handle it and error out anyway?

Since the code path going all the way into raw factory, which seem to be handling just about anything, I am slightly concerned that the problem might persist or I didn't get that flow correctly.

If possible, it would be great to have a unit test in container/factory_test.go to verify that this fix indeed addresses the reported issue.

/hold

[rphillips]

Good point. It does look like the raw handler allows just about anything. Going to close this PR.

[kolyshkin]

Why is this public?

[bobbypage]

ping, no need for this to be public right?

[rphillips]

There is not a need, but I kept the precedent from the CrioNamespace variable.

[harche]

@bobbypage While this updated PR can potentially fix the issue by returning true for canHandle but false for canAccept, as I mentioned in #2957 (comment), the way appropriate factory is selected right now is by iterating over the keys of a hasmap. This would not guarantee the order in which the factories are evaluated to see if they can handle (and can accept) the given path.

Considering the raw factory can handle pretty much anything, IMO we should make sure that it's always considered last before other factories. To be precise, we need to iterate over an ordered list (where raw is last) instead of the current hasmap here, https://github.com/google/cadvisor/blob/master/container/factory.go#L235

Let me know what do you think about this, if you are fine by it I can send a PR to fix that and then we can consider accepting this PR.

[harche]

#2999

[bobbypage]

#2999 is merged. Is there anything needed to update in this PR or is it ready to review/merge?

I would like to cut release on cAdvisor soon so we can vendor it back into k/k in time for k8s1.23 code freeze.

[rphillips]

This is ready for review and merge.

[bobbypage]

thanks, small nit on #2957 (comment) and this is ready to go

[rphillips]

Commented here #2957 (comment) ... I think we should preserve the precedent.

[bobbypage]

LGTM