pkg/kube: extract Syncer from lifecycle client
#1045
Conversation
chrisseto
force-pushed
the
chris/p/extract-syncer
branch
3 times, most recently
from
260f3d6 to
3b4ea8a
Compare
Extract the simple syncing logic from `lifecycle.ResourceClient` into a generic and dedicated `kube.Syncer` struct. Future work (Console CR) will utilize this new struct to implement the bulk of its logic. Additionally fix a subtle bug in the existing sync algorithm. As `client.InNamespace` was never provided to the `ResourceClient`'s `List` calls, it would default to `"default"`. As the tests used the default namespace, this issue wasn't visible. The primary trouble is that `Sync` would not correct delete resources due to `List` not returning them. It's likely NodePools were affected in some way but how is unclear.
chrisseto
force-pushed
the
chris/p/extract-syncer
branch
from
3b4ea8a to
a8369ef
Compare
| "github.com/redpanda-data/redpanda-operator/pkg/otelutil/log" | ||
| ) | ||
|
|
||
| const fieldOwner client.FieldOwner = "redpanda-operator" |
There was a problem hiding this comment.
I also discovered, via an upgrade test, that this value is different from that in the lifecycle client.
There was a problem hiding this comment.
Sounds good to me to make them the same. Shouldn't cause any issues if they're different since this controller is only relevant for CRDs that are reffing the cluster, but the consistency is a 🎉 to me.
andrewstucki
left a comment
andrewstucki
left a comment
There was a problem hiding this comment.
Ovrall these changes LGTM. There's one issue that may or may not be a thing. Don't recall how more recent versions of Go deal with variable scoping on ranges. I overall like this extraction as it makes it a bit easier to reason about components rather than having as fat of a lifecycle client.
|
|
||
| existing = append(existing, &poolWithOrdinals{ | ||
| set: statefulSet, | ||
| set: &statefulSet, |
There was a problem hiding this comment.
| set: &statefulSet, | |
| set: ptr.To(statefulSet), |
it's a bit hard to read in the diff, but pretty sure this is a loop-initialized variable you're taking the address of. Though I vaguely recall newer versions of Go may have fixed some of the lifetime scoping of range variables, so this might not be problematic... just raises my spidey senses a bit.
There was a problem hiding this comment.
I'm 90% certain that this is fine as of go... 1.22/1.23. If it wasn't fine ptr.To would produce the same error :P
I'll double check before merging. As I wrote this all I was thinking was "rustc would scream blood murder about this. I love garbage collection".
There was a problem hiding this comment.
👍 - yeah, looks like 1.22 introduced a change in range-based scope lifecycles. Though, if it hadn't, ptr.To would actually fix it due to the assignment to an intermediate variable (i.e. the function parameter).
| "github.com/redpanda-data/redpanda-operator/pkg/otelutil/log" | ||
| ) | ||
|
|
||
| const fieldOwner client.FieldOwner = "redpanda-operator" |
There was a problem hiding this comment.
Sounds good to me to make them the same. Shouldn't cause any issues if they're different since this controller is only relevant for CRDs that are reffing the cluster, but the consistency is a 🎉 to me.
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
Extract the simple syncing logic from
lifecycle.ResourceClientinto ageneric and dedicated
kube.Syncerstruct.Future work (Console CR) will utilize this new struct to implement the bulk of
its logic.
Additionally fix a subtle bug in the existing sync algorithm. As
client.InNamespacewas never provided to theResourceClient'sListcalls,it would default to
"default". As the tests used the default namespace, thisissue wasn't visible. The primary trouble is that
Syncwould not correctdelete resources due to
Listnot returning them. It's likely NodePools wereaffected in some way but how is unclear.