Fix for final inventory update #319
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: seans3 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| } | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // unionObjects returns the union of sliceA and sliceB as a slice of unstructured objects. |
There was a problem hiding this comment.
If both sliceA and sliceB contains the unstructured object with the same ObjMeta, the one from sliceB is taken.
| localIds = append(localIds, clusterObj) | ||
| klog.V(4).Infof("skip prune for lifecycle directive %s/%s", pruneObj.Namespace, pruneObj.Name) | ||
| taskContext.EventChannel() <- createPruneEvent(pruneObj, obj, event.PruneSkipped) | ||
| pruneFailures = append(pruneFailures, pruneObj) |
There was a problem hiding this comment.
Maybe add a comment here for why adding it to pruneFailures. It's actually not a failure, we want to use pruneFailures to keep it in the final inventory list.
| continue | ||
| } | ||
| err = namespacedClient.Delete(context.TODO(), pruneObj.Name, metav1.DeleteOptions{}) | ||
| if err != nil { |
There was a problem hiding this comment.
The error handling here is the same as the one after namespacedClient, err := po.namespacedClient(pruneObj). We can combine the getting client and deleting the object into one function, like what you have done for getObject. The new function can be deleteObject.
|
So overall looks good, but I want to understand how we decide whether a resource that we failed to apply should be included in the inventory or not. I want to make sure that a situation where a resource already exists in the cluster and apply (which in this case would be an update/patch) fails, we don't end up orphaning the resource. I don't think this were among the testcases. |
We had an informative discussion about this corner case, and we believe we know a way forward to make the final inventory storage step even more error tolerant. This PR is strictly better than before, so we will merge this current PR. The improvements to the algorithm will be instituted in a future PR. The following is documentation of our discussion to further improve the final inventory calculation. The apply operation currently does a
Implementing this decision tree (in a future PR) would have two prerequisites:
Implementing this decision tree would allow us to remove the final |
|
/lgtm |
Prunefunction to correctly store the final inventory items. These items are:Pruneunit test to validate the final inventory stored is correct.TaskContextinapply_taskto ensure only successfully applied objects are added to context.