POC - Refactor Status Updater to be resource agnostic #1787
Conversation
Problem: Currently, the status updater is aware of what resource it is updating the status of. This makes it difficult to extend because for each new resource we add support for, we have to modify the status updater. Solution: Replace old status updater with two new ones (1) Regular Updater, to update statuses of multiple resources via UpdateRequest type (2) CachingGroupUpdater to update statuses of groups of resources and cache results until the updater is enabled (when pod becomes leader). It uses Regular Updater. Using groups allow replacing statuses of subset of resources, without the need to recompute all cached statuses. The new status2 package (which will replace status package) is resource agnostic. This is acomplished by representing status update as a function (Setter). The manager packages were updated: - provisioner mode, to use regular Updater - static mode, to use CachingGroupUpdater status Setters were updated and moved to the static package. CLOSES -- nginxinc#1071
|
I'd like to get the feedback on the approach. Please note that low level implementation details (like bodies of new functions or updated) will change. The new code is implemented just to make it compile and work (I successfully ran cafe example) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1787 +/- ##
============================================
+ Coverage 86.16% 100.00% +13.83%
============================================
Files 87 1 -86
Lines 5515 209 -5306
Branches 52 52
============================================
- Hits 4752 209 -4543
+ Misses 717 0 -717
+ Partials 46 0 -46 ☔ View full report in Codecov by Sentry. |
| statuses = append(statuses, buildBackendTLSPolicyStatuses(graph.BackendTLSPolicies)...) | ||
|
|
||
| groupReq := status2.GroupUpdateRequest{ | ||
| Name: "all-graphs-expect-gateway", |
There was a problem hiding this comment.
| Name: "all-graphs-expect-gateway", | |
| Name: "all-graphs-except-gateway", |
| } | ||
|
|
||
| type GroupUpdateRequest struct { | ||
| Name string |
There was a problem hiding this comment.
What are we expecting for name? Does each status update require a unique name? update-{idx}? Or is this field used to differentiate between types of status updates (gateway API resources v/s ngf resources).
There was a problem hiding this comment.
each group requires a unique name.
Or is this field used to differentiate between types of status updates (gateway API resources v/s ngf resources).
yep
There was a problem hiding this comment.
Why not use a typed string or an enum? I think a string is too ambiguous
There was a problem hiding this comment.
it is up to the client to decide how many groups to use. For this perspective, string looks reasonable to me
There was a problem hiding this comment.
Okay, I suggest adding the groups as consts on the client side so that they are documented somewhere. I also recommend describing the purpose of the group name as a comment in the final code. It's not clear what the group name is for.
| Request []UpdateRequest | ||
| } | ||
|
|
||
| type CachingGroupUpdater struct { |
There was a problem hiding this comment.
Not sure this is the best name. Yes, the updater is caching, but it is not clear from the name why you would want to use this updater. We are caching so that we can replay the statuses when a leader changes.
There was a problem hiding this comment.
How about DelayedGroupUpdater
All updates are delayed until the Updater is disabled.
Note that once Enabled, it cannot be disabled.
There was a problem hiding this comment.
All updates are delayed until the Updater is disabled.
Is that true? Updates are made immediately if enabled...
There was a problem hiding this comment.
Isn't it a leader election updater? I also don't know if we need group in the name. The Updater can update statuses for multiple resources too. If I was writing a new controller, how would I decide which Updater to use?
There was a problem hiding this comment.
Isn't it a leader election updater?
yep. but leader election is a signal to enable it. so that's why there is Enable method. and from that perspective, since the type isn't really coupled to leader election otherwise, I didn't put Leader in its name.
The Updater can update statuses for multiple resources too. If I was writing a new controller, how would I decide which Updater to use?
If the developer wants to update statuses without leader election - Updater (like in provisioner mode)
If the developer uses leader election - GroupUpdater
Note that we introduce Groups because the static mode handler updates statuses separately for:
- Gateways
- All Graph resources expect Gateways
- NginxGateway resource
( Could be more groups later)
With having groups for each of those, we can replace the statuses of each group after each update by the handler. This is only needed until the Updater is enabled. We don't really need to store updates after the updater is Enabled - we can update right way. I can update the code to reflect that.
There was a problem hiding this comment.
This makes sense, but I still think the name should reflect the purpose of the interface. CachingGroupUpdater doesn't mean anything in the context of our product. It doesn't indicate that it can be enabled or that it should be used for leader election. I understand not wanting to couple the updater to leader election, but when the purpose of the Updater is to only write status when it's leader, I'm not sure it can be avoided. Even if we leave the name the same, we will have to add comments to the code to indicate that it is safe for leader election.
I also feel that the term group is ambiguous, but I can't think of a better name.
There was a problem hiding this comment.
Do you feel the same way about DelayedGroupUpdater?
What about DeferredGroupUpdater?
There was a problem hiding this comment.
I think DeferredGroupUpdater is a better description of what the updater does, but I still don't think it conveys that it is safe for leader election. I think you'll still end up with a comment.
Naming is hard, and I'm definitely not going to block the PR over this, but I don't see the harm in calling it what it is. I'd feel differently if there were another purpose for this Updater, but it is purpose-built for leader election. Why call it anything else? Coupling isn't always bad when two things are actually related.
There was a problem hiding this comment.
what about LeaderAwareGroupUpdater?
|
closing this. will create a new PR for the proper implementation. thanks for the feedback |
Proposed changes
Problem:
Currently, the status updater is aware of what resource it is updating the status of. This makes it difficult to extend because for each new resource we add support for, we have to modify the status updater.
Solution:
Replace old status updater with two new ones
(1) Regular Updater, to update statuses of multiple resources via UpdateRequest type
(2) CachingGroupUpdater to update statuses of groups of resources and cache results until the updater is enabled (when pod becomes leader). It uses Regular Updater.
Using groups allow replacing statuses of subset of resources, without the need to recompute all cached statuses.
The new status2 package (which will replace status package) is resource agnostic. This is acomplished by representing status update as a function (Setter).
The manager packages were updated:
status Setters were updated and moved to the static package.
CLOSES -- #1071
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.