Implement Status Updater Retrying on Failures

[bjee19]

Proposed changes

Problem: NKG will not retry on status update failure, thus there is a chance that some resources will not have up-to-do statuses.

Solution: Add retry logic when status update fails with a small exponential backoff after each retry. Also, added logic to allow for a graceful exit of the status updater when the NKG pod context is cancelled.

Testing: Manually tested that the issue found here: #563 no longer occurs. Added unit tests for retry logic. Updated tests so they correctly test that when the context is canceled, the status updater will no longer update statuses.

Please focus on (optional): Feedback on how many attempts the status updater should retry before returning an error and moving on. Also, what the starting backoff sleep time should be.

Closes #1016

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto main
• I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

[bjee19]

Ran into a tricky bug and got help from Saylor where the fake client we use in tests may not be honoring a canceled context and was still getting and updating statuses even after having its context canceled. This was a problem because when the context is canceled, the blocking select statement in the status updater's Update() will be unblocked, while the goroutine finishes in the background. However, even with a canceled context, the goroutine was still getting and updating statuses. Thus we ran into a race where the tests would expect everything to be finished when updater.Update() returns and it would check statements while statuses were still being changed in the goroutine. To fix this I added a check at the top of updater() so that if the context was canceled it wouldn't continue. The issue may only exist with how our fake client doesn't respect a canceled context.

[bjee19]

Manually tested that the issue found here: #563 still no longer occurs.

[bjee19]

A couple things I'm a bit unsure of:

1. Is there a better name than ConditionWithContextFunc?
2. Is it alright that I called the new interface StatusUpdater and put a nolint on it because it was complaining that the interface started with the same word as the package status
3. How are the tests and is it alright that I split them up like so?

[kate-osborn]

A couple things I'm a bit unsure of:

1. Is there a better name than ConditionWithContextFunc?
2. Is it alright that I called the new interface StatusUpdater and put a nolint on it because it was complaining that the interface started with the same word as the package status
3. How are the tests and is it alright that I split them up like so?

1. Probably. Naming is hard. Maybe NewRetryUpdateFunc?
2. Yeah, it's ok. Again, naming is hard. Too bad that we already have an Updater interface. We could call it K8sUpdater. 🤷‍♀️
3. I think splitting them is fine, but it would be nice if the test file had the prefix updater so it shows up next to the updater_test.go. Maybe updater_retry_test.go.

I wonder if we want to start a new pattern where we name the unit test files unit. Like updater_unit_test.go. Or alternatively, name the gomega test files using bdd.

[bjee19]

I think naming the unit test files unit is a good idea to me!

[kate-osborn]

Looks good, just a few small nits!