containers / virtcontainers

A Go package for building hardware virtualized container runtimes
Apache License 2.0
139 stars 43 forks source link

cni: pass pod ID to CNI plugin's add/delete calls #507

Closed egernst closed 6 years ago

egernst commented 6 years ago

We were passing a virtualEndpoint.NetPair.ID previously.

CNI expects to receive a POD ID when being called. This patch changes the CNI calls to provide this behavior.

This addresses https://github.com/containers/virtcontainers/issues/506

Signed-off-by: Eric Ernst eric.ernst@intel.com

egernst commented 6 years ago

Need to test locally still - WIP

jodh-intel commented 6 years ago

@egernst, @mcastelino - is there any way we could create a minimal cni_test.go for this and start to build up a set of unit tests? Or would that require a lot of infrastructure to allow them to run?

sboeuf commented 6 years ago

LGTM

Approved with PullApprove Approved with PullApprove Approved with PullApprove

egernst commented 6 years ago

@jodh-intel - This would require at least creating a network namespace with a number of endpoints in it. AFAICT, we wouldn't need a POD and this part could just be "faked". I'd need to take a look at what exists today in the network common testing... Can we open an issue for adding this? I think it'll be a bit more involved, and I'd love to fix this bug in the meantime.

sboeuf commented 6 years ago

I agree with @egernst, this can be part of a follow up PR to solve the more global handling of CNI testing. @egernst please open an issue and reference it here. I'll merge this PR in the meantime.

egernst commented 6 years ago

@jodh-intel -- https://github.com/containers/virtcontainers/issues/508

sboeuf commented 6 years ago

@egernst could you remove do-not-merge label ?