fix(offloading): Correctly deleted offloaded data. Fixes #2206 #2207
Conversation
| @@ -182,6 +186,12 @@ func (wdc *nodeOffloadRepo) ListOldOffloads(namespace string) ([]UUIDVersion, er | |||
| } | |||
|
|
|||
| func (wdc *nodeOffloadRepo) Delete(uid, version string) error { | |||
| if uid == "" { | |||
There was a problem hiding this comment.
checks to make sure that can't happen again
| @@ -145,7 +147,8 @@ func (wdc *nodeOffloadRepo) List(namespace string) (map[UUIDVersion]wfv1.Nodes, | |||
| log.WithFields(log.Fields{"namespace": namespace}).Debug("Listing offloaded nodes") | |||
| var records []nodesRecord | |||
| err := wdc.session. | |||
| SelectFrom(wdc.tableName). | |||
There was a problem hiding this comment.
we should never use SelectFrom it always does SELECT *
workflow/controller/controller.go
Outdated
| @@ -268,6 +268,7 @@ func (wfc *WorkflowController) periodicWorkflowGarbageCollector(stopCh <-chan st | |||
| oldRecords, err := wfc.offloadNodeStatusRepo.ListOldOffloads(wfc.GetManagedNamespace()) | |||
| if err != nil { | |||
| log.WithField("err", err).Error("Failed to list old offloaded nodes") | |||
| return | |||
| @@ -14,7 +14,8 @@ import ( | |||
| ) | |||
|
|
|||
| type UUIDVersion struct { | |||
| UID, Version string | |||
| UID string `db:"uid"` | |||
There was a problem hiding this comment.
these were all empty strings
Codecov Report
@@ Coverage Diff @@
## master #2207 +/- ##
==========================================
- Coverage 11.70% 11.70% -0.01%
==========================================
Files 52 52
Lines 26313 26322 +9
==========================================
Hits 3081 3081
- Misses 22837 22846 +9
Partials 395 395 Continue to review full report at Codecov.
|
workflow/controller/controller.go
Outdated
| @@ -268,6 +268,7 @@ func (wfc *WorkflowController) periodicWorkflowGarbageCollector(stopCh <-chan st | |||
| oldRecords, err := wfc.offloadNodeStatusRepo.ListOldOffloads(wfc.GetManagedNamespace()) | |||
| if err != nil { | |||
| log.WithField("err", err).Error("Failed to list old offloaded nodes") | |||
| return | |||
There was a problem hiding this comment.
This will exit the loop. if there is any intermediate network issue or DB issue, return will exit the loop with an error message and stop the node status GC.
Do you think the following behavior will fit for this scenario?
- Implement ExponentialBackoff for the Query inside the loop instead of return
- Restart the controller so it will check the DB connectivity while start.
There was a problem hiding this comment.
I'm not sure about back off etc. Adds a massive level of testing complexity for little benefit.
There was a problem hiding this comment.
how will the application behave if wfc.offloadNodeStatusRepo.ListOldOffloads return the err?
There was a problem hiding this comment.
it will exit, I think this is incorrect, should continue (same applies for other lines) - I will fix
Checklist:
"fix(controller): Updates such and such. Fixes #1234".Fixes #2206