Dualstack CNS Changes #1773
Dualstack CNS Changes #1773
Conversation
rjdenney
force-pushed
the
dualstack-cilium-implementation-CNS
branch
19 times, most recently
from
69e8dcc to
86c55e7
Compare
cns/restserver/ipam.go
Outdated
| } else { | ||
| logger.Errorf("[AssignDesiredIPConfigs] Desired IP is already assigned %+v, requested for pod %+v", ipConfig, podInfo) | ||
| //nolint:goerr113 // return error | ||
| return podIPInfo, fmt.Errorf("IP already assigned to another pod") |
There was a problem hiding this comment.
if you return here, what happens to the other desired IPs you may have assigned already in this loop?
is the caller function releasing those back?
There was a problem hiding this comment.
All assigning is done after this loop now
| return podIPInfo, fmt.Errorf("IP already assigned to another pod") | ||
| } | ||
| case types.Available, types.PendingProgramming: | ||
| // This race can happen during restart, where CNS state is lost and thus we have lost the NC programmed version |
There was a problem hiding this comment.
the original function is calling service.assignIPConfig. Why is that not getting called anymore?
There was a problem hiding this comment.
this may indicate that there is no test catching this bug.
There was a problem hiding this comment.
This is getting called outside of this for loop in the next for loop. To avoid assigning IPs and then needing to release them again I just add them to a map here (now changed to a slice) to be looped through and added
cns/restserver/ipam.go
Outdated
| default: | ||
| logger.Errorf("[AssignDesiredIPConfigs] Desired IP is not available %+v", ipConfig) | ||
| //nolint:goerr113 // return error | ||
| return podIPInfo, fmt.Errorf("IO not available") |
There was a problem hiding this comment.
same comment as above -
if you return here, what happens to the other desired IPs you may have assigned already in this loop?
is the caller function releasing those back?
There was a problem hiding this comment.
Nothing will be assigned yet.
cns/restserver/ipam.go
Outdated
| if err := service.populateIPConfigInfoUntransacted(ipState, &podIPInfo); err != nil { | ||
| return cns.PodIpInfo{}, err | ||
| } | ||
| podIPctr := 0 |
There was a problem hiding this comment.
nit: for clarity please use vars such as numIpConfigsToAssign and numIpConfigsAssigned
cns/restserver/ipam.go
Outdated
| } | ||
|
|
||
| // If IPConfig is already assigned to pod, it returns that else it returns one of the available ipconfigs. | ||
| func requestIPConfigHelper(service *HTTPRestService, req cns.IPConfigRequest) (cns.PodIpInfo, error) { | ||
| func requestIPConfigHelper(service *HTTPRestService, req cns.IPConfigsRequest) ([]cns.PodIpInfo, error) { |
There was a problem hiding this comment.
should the func name be updated to reflect it's handling multiple IP configs
cns/restserver/ipam_test.go
Outdated
| t.Fatal("Expected available ips to be zero since we expect the IP to still be assigned") | ||
| } | ||
| } | ||
| // func TestIPAMFailToReleaseOneIPWhenExpectedToHaveTwo(t *testing.T) { |
There was a problem hiding this comment.
This function will need to be updated which is why it is commented out right now
|
|
||
| _, err = requestIPAddressAndGetState(t, req) | ||
| _, err := requestIPAddressAndGetState(t, req) |
There was a problem hiding this comment.
You should add a test where you specify 2 desired IPs - first one is valid while second one is invald.
In addition to validating that we get non-nil error, we should also confirm that we are not partially assigning IPs. This will add coverage to the code where in case of partial success, we release the assigned IP back to the pool
cns/restserver/ipam_test.go
Outdated
| ncIDs := []string{testNCID, testNCIDv6} | ||
| IPs := []string{testIP1, testIP1v6} | ||
| prefixes := []uint8{IPPrefixBitsv4, IPPrefixBitsv6} | ||
| IPAMFailToGetDesiredIPConfigWithAlreadyAssignedSpecfiedIP(t, ncIDs, IPs, prefixes) |
There was a problem hiding this comment.
should validate that we don't allow partial success
| } | ||
| err := service.populateIPConfigInfoUntransacted(ipConfig, &podIpInfo) | ||
| return podIpInfo, err | ||
| case types.Available, types.PendingProgramming: |
There was a problem hiding this comment.
can we add a todo / question on why we would attempt to assign ip which is pendingProgramming?
If the IP state is pendingProgramming for the desired IP, we should fail the request as it may cause the data path issue if we assign it. @rbtr
There was a problem hiding this comment.
I can add something. I thought this was odd myself when I first saw it in the code. I believe the comment below is supposed to explain why and it sounds like we need this for when CNS restarts and marking everything back as assigned that has already been assigned. I would like to know more about this though.
// This race can happen during restart, where CNS state is lost and thus we have lost the NC programmed version
// As part of reconcile, we mark IPs as Assigned which are already assigned to Pods (listed from APIServer)
rjdenney
force-pushed
the
dualstack-cilium-implementation-CNS
branch
from
30a15c2 to
a33291a
Compare
Reason for Change:
This is the CNS side of changes for dualstack in Linux in Windows. Azure-ipam changes will follow once this is merged
Issue Fixed:
Requirements:
Notes: