Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

return error in case Weave CNI plug-in fails to perform CmdDel #3638

Merged
merged 1 commit into from
May 10, 2019

Conversation

murali-reddy
Copy link
Contributor

@murali-reddy murali-reddy commented May 2, 2019

return error in case Weave CNI plug-in fails to perform CmdDel so that kubelet will retry DEL (preventing IP leak)

Also if there are no IP's associated with container iD just return (so that its idempotent) with out raising error. With out this change, if kubelet fails to perform successful ADD, it would perform DEL (as part of cleanup) with containerid. Weave IPAM will return an error as there are no IP's associated, resulting in kubelet performing the operation unsuccessfully (resulting in retry's forever).

Fixes #3587

@@ -406,7 +406,7 @@ func (alloc *Allocator) Delete(ident string) error {
func (alloc *Allocator) delete(ident string) error {
cidrs := alloc.removeAllOwned(ident)
if len(cidrs) == 0 {
return fmt.Errorf("Delete: no addresses for %s", ident)
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe log this to stderr in case it is of interest to someone debugging?
In the typical case stderr is inherited from kubelet, i.e. the message will show up in the kubelet logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added debug log.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry, I misread what you'd done. If you want to swallow the error for CNI that code should be in the CNI plugin.
Unless I'm missing the reason why you'd want to do it for every caller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this change goes into weaver process. Either called directly or through CNI plugin, either way to me this method need to be idempotent. Hence suppressing the error here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I'm missing why it needs to be idempotent for every caller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have restricted the changes to CNI plugin only by ignoring the error. PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

That is remarkably ugly 😝 I guess we can go with it.

Did wonder about changing ipam to return 204 for the no-addresses case, but I see it currently returns 204 in every case!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is remarkably ugly

Indeed. Could not think more idiomatic way. Thought of adding a custom error type but call crosses process boundaries.

@bboreham bboreham added this to the 2.5.2 milestone May 10, 2019
@murali-reddy murali-reddy changed the base branch from master to 2.5 May 10, 2019 12:02
@murali-reddy
Copy link
Contributor Author

rebased to 2.5

@bboreham bboreham merged commit 6e78a34 into 2.5 May 10, 2019
@bboreham bboreham deleted the pruneowned branch May 10, 2019 14:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants