fix: get container ID from kube rather than docker #371

rudoi · 2019-04-01T15:36:02Z

Issue #, if available:

Description of changes:

Gets container ID from Kubernetes rather than Docker. This makes the CNI runtime-agnostic.

When using containerd instead of Docker, the CNI essentially skips allocating addresses for all pods because it can't find containers to associate them with. The current retry logic doesn't fail hard when the CNI is unable to connect to a Docker sock. New pods appear to successfully obtain IP addresses, but when the CNI pod restarts, these IPs will all be freed because the CNI will be unable to connect to Docker and therefore will skip allocating IPs for all pods on the host.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

mogren

Ran conformance tests and everything looks good. Thanks, this was a nice cleanup, just a small logging comment.

I'll update our dependencies in another PR, guess we don't need the docker client import any more.

pkg/k8sapi/discovery.go

liwenwu-amazon · 2019-04-08T23:16:59Z

This change needs to be carefully tested out to make sure no same IP can be allocated to different Pods.
ipamD does NOT preserve IP allocations pool state through its restart
During ipamD restart, it first builds IP warm Pool info from instance metadata service, then it talks to Docker to find out the all running Pod's IPs and take running Pod IPs out of IP warm pool. After this, it then responds CNI-ADD request.

To get same info from kube, you need to make sure ipamD have learned all running Pod IPs before start accepting CNI-ADD.

mogren · 2019-04-08T23:52:02Z

Thanks @liwenwu-amazon, that is good to know. We won't merge this without some more testing.

rudoi · 2019-04-09T20:31:34Z

@mogren / @liwenwu-amazon

I dumped the list of ENIs on a host running 5 pods, 3 of which were hostNetwork=true, and saw that 2 were allocated as expected. Then, I kill'd the aws-k8s-agent process, causing the pod to restart. After the CNI plugin came back online, I captured the ENI list again and there was no diff between the two ENI dumps.

FWIW, I've been running a cluster off of this branch with several nodes and several dozen pods for about 10 days. A few of the CNI daemonset pods have restarts for various reasons, but I haven't seen any issues so far with duplicate IP allocation.

If there's anything more specific I should run some tests for, please let me know. I'm curious to understand how getting the container ID from kube API vs Docker has an impact on how IPs are allocated. It doesn't seem like the container ID is ever read again after the initial fetch.

liwenwu-amazon · 2019-04-10T04:53:21Z

@rudoi @mogren I think you should try to restart ipamD while Pods are being scaled up and down rapidly. And make sure there is no race condition when using kube API. Here is one example:

Assumes IP warm pool have 200 IPs, and have allocated 150 IPs to Pods, e.g. IP1 ... IP150
then ipamD restart
ipamD rebuild its IP warm pool to have 200 IPs
ipamD starts watching PODs from K8S API Server
it start learning pods that were using IP1 to IP 150
what if K8S server schedule a new Pod before ipamD finish learning pods (IP1 to IP150), then it might give one of those IP (IP1 to IP150) to this new Pod.

With Docker API, the ipamD make sure it finds out all running Pod before start accepting CNI-add request.

rudoi · 2019-04-10T14:31:46Z

Understood, @liwenwu-amazon. Thanks for clarifying. I'll try my hardest to cause that race. 😄

mogren · 2019-05-10T04:31:19Z

@rudoi Hi again!

I like this change, and I think we have some great integration tests to test this. I still bumped this change out to the v1.6 Milestone since I'm trying to get v1.5 out soon. Would you care to rebase this change against the latest master?

rudoi · 2019-05-13T16:22:27Z

@rudoi Hi again!

I like this change, and I think we have some great integration tests to test this. I still bumped this change out to the v1.6 Milestone since I'm trying to get v1.5 out soon. Would you care to rebase this change against the latest master?

Hi @mogren! Rebased, made sure tests are passing, etc. Looking forward to seeing the integration tests! I did some stress testing last week but I wasn't totally satisfied with my scenarios.

dadux · 2019-05-14T23:36:02Z

@rudoi - thanks for this PR.

We are about to deploy some of our clusters with this fix - so hopefully we'll get some real world use case before it gets merged into 1.6.

rudoi · 2019-05-14T23:38:24Z

@dadux - awesome, looking forward to seeing the results!

We've been running this for about 3 weeks in real clusters and have not run into anything yet. -fingers crossed-

Fix tests

dadux · 2019-07-23T06:47:54Z

Hi @mogren - what is preventing this to get merged ?

Happy to provide some help if needed !

rudoi · 2019-07-23T14:20:33Z

Happy to help out as well!

mogren · 2019-07-23T20:02:39Z

Hey @dadux and @rudoi! I just wanted to run a few more tests to try and catch any potential edge cases. Everything looks good on my end, so thanks a lot. Sorry it took so long to verify the change.

* fix: get container ID from kube rather than docker * chore: add log statement when containerID is found * fix: log containerID after assignment * fix: update unit tests (cherry picked from commit 14de538)

* fix: get container ID from kube rather than docker * chore: add log statement when containerID is found * fix: log containerID after assignment * fix: update unit tests (cherry picked from commit 14de538) (cherry picked from commit ef06ed8)

* fix: get container ID from kube rather than docker * chore: add log statement when containerID is found * fix: log containerID after assignment * fix: update unit tests (cherry picked from commit 14de538) (cherry picked from commit ef06ed8) (cherry picked from commit d644610)

This reverts commit 14de538.

* Revert "fix: get container ID from kube rather than docker (#371)" This reverts commit 14de538. * go mod tidy * Add initial proof-of-concept implementation of CRI support. * Update CRI socket path to /var/run/cri.sock * Filter ready sandboxes and abort if there's a pod UID conflict. * Revert "fix: get container ID from kube rather than docker (#371)" This reverts commit 14de538. * Address review comments, refactor to use pod sandbox nomenclature consistently. * Bail if we can't retrieve local pod sandboxes on startup.

* Revert "fix: get container ID from kube rather than docker (aws#371)" This reverts commit 14de538. * go mod tidy * Add initial proof-of-concept implementation of CRI support. * Update CRI socket path to /var/run/cri.sock * Filter ready sandboxes and abort if there's a pod UID conflict. * Revert "fix: get container ID from kube rather than docker (aws#371)" This reverts commit 14de538. * Address review comments, refactor to use pod sandbox nomenclature consistently. * Bail if we can't retrieve local pod sandboxes on startup.

) * Revert "fix: get container ID from kube rather than docker (#371)" This reverts commit 14de538. * go mod tidy * Add initial proof-of-concept implementation of CRI support. * Update CRI socket path to /var/run/cri.sock * Filter ready sandboxes and abort if there's a pod UID conflict. * Revert "fix: get container ID from kube rather than docker (#371)" This reverts commit 14de538. * Address review comments, refactor to use pod sandbox nomenclature consistently. * Bail if we can't retrieve local pod sandboxes on startup.

rudoi mentioned this pull request Apr 1, 2019

Does this CNI support containerd? #365

Closed

mogren added the enhancement label Apr 1, 2019

mogren added this to the v1.5 milestone Apr 3, 2019

rudoi force-pushed the fix/get-container-from-kube branch from a6ee2f9 to 827328e Compare April 4, 2019 18:52

mogren self-requested a review April 6, 2019 00:17

mogren suggested changes Apr 8, 2019

View reviewed changes

pkg/k8sapi/discovery.go Show resolved Hide resolved

mogren approved these changes Apr 8, 2019

View reviewed changes

mogren requested a review from nckturner April 8, 2019 22:03

rudoi force-pushed the fix/get-container-from-kube branch from c387c66 to 574334a Compare April 15, 2019 20:15

mogren modified the milestones: v1.5, v1.6 May 9, 2019

rudoi force-pushed the fix/get-container-from-kube branch from 574334a to 821467c Compare May 13, 2019 14:49

Andrew Rudoi added 4 commits May 13, 2019 07:52

fix: get container ID from kube rather than docker

593a720

chore: add log statement when containerID is found

a487c99

fix: log containerID after assignment

4b50536

test: fix ipamd tests

4a30273

rudoi force-pushed the fix/get-container-from-kube branch from f4e4ee2 to 4a30273 Compare May 13, 2019 15:57

Claes Mogren and others added 2 commits June 13, 2019 10:42

Merge branch 'master' into fix/get-container-from-kube

c267d97

Fix unit tests

1eaee71

mogren removed the request for review from nckturner June 19, 2019 20:32

Merge branch 'master' into fix/get-container-from-kube

0583f8d

Update rpc_handler_test.go

7235b95

Fix tests

mogren mentioned this pull request Jul 23, 2019

Detach ENI before deleting #538

Merged

mogren merged commit 14de538 into aws:master Jul 23, 2019

drakedevel mentioned this pull request Nov 9, 2019

Restarting aws-node leaks all pod IPs after #371 due to use of incorrect container ID #712

Closed

drakedevel added a commit to frontapp/amazon-vpc-cni-k8s that referenced this pull request Nov 12, 2019

Revert "fix: get container ID from kube rather than docker (aws#371)"

de8e10f

This reverts commit 14de538.

drakedevel mentioned this pull request Nov 12, 2019

Use CRI to obtain pod sandbox IDs instead of Kubernetes API #714

Merged

drakedevel added a commit to frontapp/amazon-vpc-cni-k8s that referenced this pull request Nov 14, 2019

Revert "fix: get container ID from kube rather than docker (aws#371)"

d558753

This reverts commit 14de538.

drakedevel added a commit to frontapp/amazon-vpc-cni-k8s that referenced this pull request Nov 16, 2019

Revert "fix: get container ID from kube rather than docker (aws#371)"

bb17215

This reverts commit 14de538.

drakedevel added a commit to frontapp/amazon-vpc-cni-k8s that referenced this pull request Nov 25, 2019

Revert "fix: get container ID from kube rather than docker (aws#371)"

e243d10

This reverts commit 14de538.

mogren mentioned this pull request Dec 4, 2019

Use CRI to obtain pod sandbox IDs instead of Kubernetes API (#714) #741

Merged

moensch mentioned this pull request May 11, 2020

Is dockersock mount still needed? #971

Closed

hakman mentioned this pull request Dec 27, 2020

Use containerd.sock for AmazonVPC CNI with containerd kubernetes/kops#10502

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: get container ID from kube rather than docker #371

fix: get container ID from kube rather than docker #371

rudoi commented Apr 1, 2019

mogren left a comment

liwenwu-amazon commented Apr 8, 2019

mogren commented Apr 8, 2019

rudoi commented Apr 9, 2019

liwenwu-amazon commented Apr 10, 2019

rudoi commented Apr 10, 2019

mogren commented May 10, 2019

rudoi commented May 13, 2019

dadux commented May 14, 2019

rudoi commented May 14, 2019

dadux commented Jul 23, 2019

rudoi commented Jul 23, 2019

mogren commented Jul 23, 2019

fix: get container ID from kube rather than docker #371

fix: get container ID from kube rather than docker #371

Conversation

rudoi commented Apr 1, 2019

mogren left a comment

Choose a reason for hiding this comment

liwenwu-amazon commented Apr 8, 2019

mogren commented Apr 8, 2019

rudoi commented Apr 9, 2019

liwenwu-amazon commented Apr 10, 2019

rudoi commented Apr 10, 2019

mogren commented May 10, 2019

rudoi commented May 13, 2019

dadux commented May 14, 2019

rudoi commented May 14, 2019

dadux commented Jul 23, 2019

rudoi commented Jul 23, 2019

mogren commented Jul 23, 2019