Add PendingRelease to CNS on start #672
Conversation
cns/restserver/internalapi.go
Outdated
| } | ||
| } | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
need a cancellable context, but this parent is short lived, potentially pass in context to initCNS
There was a problem hiding this comment.
Curious, why is this being moved from main? It seems like in this scope (ReconcileNCState) the monitor would get "started" multiple times rather than just once.
Codecov Report
@@ Coverage Diff @@
## master #672 +/- ##
==========================================
- Coverage 42.63% 41.72% -0.92%
==========================================
Files 72 72
Lines 10392 9488 -904
==========================================
- Hits 4431 3959 -472
+ Misses 5482 5058 -424
+ Partials 479 471 -8 |
ramiro-gamarra
left a comment
ramiro-gamarra
left a comment
There was a problem hiding this comment.
Couple of questions
cns/restserver/internalapi.go
Outdated
| } | ||
| } | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
Curious, why is this being moved from main? It seems like in this scope (ReconcileNCState) the monitor would get "started" multiple times rather than just once.
cns/restserver/ipam.go
Outdated
| ipconfig.State = cns.PendingRelease | ||
| service.PodIPConfigState[id] = ipconfig | ||
| } else { | ||
| return fmt.Errorf("Inconsistent state, ipconfig with ID [%v] marked as pending release, but does not exist in state", id) |
There was a problem hiding this comment.
Curious, would it be better to try to fix the spec at this point (i.e. garbage collect the ip id that is not referenced anywhere else)? Maybe that's done somewhere else but I'm not aware.
There was a problem hiding this comment.
This can be an expected state. say during reconcile, spec had some stale ToBeDeleted ips list which were already cleanup in status. Thus, PodIpConfigState wont have that ip. We can log this but this is not an error.
cns/restserver/internalapi.go
Outdated
| } | ||
| } | ||
|
|
||
| ctx := context.Background() |
There was a problem hiding this comment.
+1 on Ramiro's feedback. This should be called from main only after Reconcile is completed. It could be possible that reconcile api is skipped if CRD is not available but IPAM pool monitor should still start.
cns/restserver/internalapi.go
Outdated
| logger.Errorf(returnMessage) | ||
| } | ||
|
|
||
| err = service.MarkExistingIPsAsPending(spec.IPsNotInUse) |
There was a problem hiding this comment.
We dont need to call this during all createOrUpdateNC calls. This only needs to be marked one time during Reconcile. Can we move this to the caller API (ReconcileNCState)
cns/restserver/ipam.go
Outdated
| ipconfig.State = cns.PendingRelease | ||
| service.PodIPConfigState[id] = ipconfig | ||
| } else { | ||
| return fmt.Errorf("Inconsistent state, ipconfig with ID [%v] marked as pending release, but does not exist in state", id) |
There was a problem hiding this comment.
This can be an expected state. say during reconcile, spec had some stale ToBeDeleted ips list which were already cleanup in status. Thus, PodIpConfigState wont have that ip. We can log this but this is not an error.
| for _, id := range pendingIPIDs { | ||
| if ipconfig, exists := service.PodIPConfigState[id]; exists { | ||
| if ipconfig.State == cns.Allocated { | ||
| return fmt.Errorf("Failed to mark IP [%v] as pending, currently allocated", id) |
There was a problem hiding this comment.
I am not super familiar with this but should we still try to set PendingRelease for subsequent IPs when one of them has allocated state?
* add pending release on start
4 participants
Reason for Change:
Issue Fixed:
Adds the pending release from the CRD to CNS on start
Requirements:
Notes: