Use IP to identify orphaned allocation to be deleted #467
Conversation
Del command should not fail when allocation is not found Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
mlguerrero12
force-pushed
the
fixwrongdeleletionduplicate
branch
from
3d7fef7
to
70dc0ec
Compare
Pull Request Test Coverage Report for Build 9204761188Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
pkg/storage/kubernetes/ipam.go
Outdated
| return newips, err | ||
| updatedreservelist, ipforoverlappingrangeupdate = allocate.DeallocateIP(reservelist, containerID) | ||
| if updatedreservelist == nil { | ||
| // Do not fail if allocation is not present. |
There was a problem hiding this comment.
Cool, I'm following. I like that you logged the condition in allocate.DeallocateIP() -- thanks -- and can silently pass here otherwise. Nice one.
There was a problem hiding this comment.
that's exactly what I don't like about it lol.
I'm ok with it though, since we do not unit test that function.
|
@mlguerrero12 It looks good to me. Just on a side note. Could you also change the function name of |
| if err != nil { | ||
| return nil, nil, err | ||
| // DeallocateIP removes allocation from reserve list. | ||
| func DeallocateIP(reservelist []types.IPReservation, containerID string) ([]types.IPReservation, net.IP) { |
There was a problem hiding this comment.
I really dislike this function's signature - you're at least improving by removing one return attribute from it ...
... but I think it still does make sense to return an error on certain conditions. It is up to the caller to know whether to swallow it or pass it along. CNI usually swallows all errors on DEL though.
Unfortunately, the fact we don't have associated unit tests makes my argument weaker, since there's no where we'd actually check the error against something.
There was a problem hiding this comment.
I see your point but I still don't think an error should be returned. However, I will change the logic a bit and return nil for IP when the allocation is not present. It makes more sense in my opinion. When not found, return the reverlist untouched and nil for IP.
There was a problem hiding this comment.
it's fine the way you currently have. Don't worry. If we had some unit tests at least to define the desired behavior, it could be arguable. Since we don't, let's just use the simplest thing.
I'm fine w/ what you have. Really.
However, I will change the logic a bit and return nil for IP when the allocation is not present.
This does make sense though.
pkg/storage/kubernetes/ipam.go
Outdated
| return newips, err | ||
| updatedreservelist, ipforoverlappingrangeupdate = allocate.DeallocateIP(reservelist, containerID) | ||
| if updatedreservelist == nil { | ||
| // Do not fail if allocation is not present. |
There was a problem hiding this comment.
that's exactly what I don't like about it lol.
I'm ok with it though, since we do not unit test that function.
The IP uniquely identifies the correct orphaned allocation that needs to be deleted during IP reconcilation. Fixes k8snetworkplumbingwg#176 Signed-off-by: Marcelo Guerrero <marguerr@redhat.com>
mlguerrero12
force-pushed
the
fixwrongdeleletionduplicate
branch
from
70dc0ec
to
c61d2fa
Compare
Fixes #176