numaproj / numaplane

Control Plane for Numaproj
Apache License 2.0
12 stars 1 forks source link

feat: support consecutive Updates to our dynamic objects; also remove use of dynamic client for Rollouts (not needed) #249

Closed juliev0 closed 2 months ago

juliev0 commented 2 months ago

Modifications

  1. the Update() function in the controller-runtime framework always returns the new object with the correct resourceVersion in it (as mentioned in this article). I wanted to do the same thing for our dynamic client Update functions. This way if we call it consecutively, we are unlikely to have any resource version conflicts that cause us to return an error. Verified this by: writing a unit test for it to confirm the updated resourceVersion gets returned from an UpdateCR() call and that we can make a subsequent call to UpdateCR().
  2. I noticed that we were using the dynamic client to update the Rollout CRs, which wasn't necessary, so I changed it to use the standard controller-runtime framework, thereby making the code simpler. Verified the calls to update status are working by running e2e and unit tests, and also updating the e2e test to check for Rollout.Status.Phase which it previously wasn't.
  3. Removed the requirement from the unit tests that the "sync failure" metrics be 0. Normal resource version conflicts can cause this to increment. Note that the change above does cause there to be some errors issued some of the time where there weren't before (the previous code just updated spec and status and not any metadata such as resourceVersion so there was never a conflict), but actually conflicts are a good thing because it means we won't ignore more recent updates that have been made on the object.