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

Not returning error when unassign called on pod without IP #740

Closed
wants to merge 1 commit into from

Conversation

uruddarraju
Copy link
Contributor

#739

As described in the above issues, the current code for UnassignIP returns an error on ipamD when UnassignIP is called on pods that dont have an IP assigned yet. This fix makes this idempotent and does not return an error when a pod without Ip goes through UnassignIP flow

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

Comment on lines +105 to +107
if err == datastore.ErrUnknownPod {
err = nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you were basing this change on tests done on v1.5.3 or earlier? This kind of change was the main cause of issues in v1.5.4. It looks innocent, but had other effects because of the cleanup that got triggered in the CNI binary. See #641 for some details.

Also, #688 should fix this issue to stop returning an error to Kubelet when we try to delete a pod that failed to get an IP. That change should be in the v1.6.0-rc4 release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mogren. That makes sense.

@uruddarraju uruddarraju closed this Dec 5, 2019
@uruddarraju
Copy link
Contributor Author

uruddarraju commented Dec 10, 2019

@mogren actually, I dont think this has been fixed yet in the cni, I just saw these logs on the cni agent, after a node reboot, leading to all pods on that node going into an Error status:

2019-12-10T20:00:00.241Z [INFO]         Starting CNI Plugin v1.6.0-rc1-44-g067ddf20 ...
2019-12-10T20:00:00.241Z [INFO]         Received CNI del request: ContainerID(93fb009f691cb1b7d448cf276ddf904ceb562ca5f1f000be8a7c444b0d19cfd2) Netns() IfName(eth0) Args(IgnoreUnknown=1;K8S_POD_NAMESPACE=integration-test-nginx-a0dskoz6ap;K8S_POD_NAME=kube-manager;K8S_POD_INFRA_CONTAINER_ID=93fb009f691cb1b7d448cf276ddf904ceb562ca5f1f000be8a7c444b0d19cfd2) Path(/opt/cni/bin/) argsStdinData({"cniVersion":"0.3.1","mtu":"9001","name":"aws-cni","type":"aws-cni","vethPrefix":"eni"})
2019-12-10T20:00:00.245Z [ERROR]        Error received from DelNetwork grpc call for pod kube-manager namespace integration-test-nginx-a0dskoz6ap container 93fb009f691cb1b7d448cf276ddf904ceb562ca5f1f000be8a7c444b0d19cfd2: rpc error: code = Unknown desc = datastore: unknown pod
2019-12-10T20:00:00.245Z [ERROR]        Failed CNI request: rpc error: code = Unknown desc = datastore: unknown pod

@mogren
Copy link
Contributor

mogren commented Dec 10, 2019

@uruddarraju You are right, I just saw it as well. The issue is that gRPC wraps the error, so the check will have to be something like:

if strings.Contains(err.Error(), datastore.ErrUnknownPod.Error()) {

@uruddarraju
Copy link
Contributor Author

uruddarraju commented Dec 10, 2019

@mogren Lol, I just figured that out as well and sent a PR out here: #750

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants