Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I haven't done go.work + main.go testing in my local kind. I'll want to do that before merging this -- will work on that a bit later. |
| cells := shard.Spec.MultiOrch.Cells | ||
| if len(cells) == 0 { | ||
| return ctrl.Result{}, fmt.Errorf( | ||
| "MultiOrch has no cells specified - cannot deploy without cell information", |
There was a problem hiding this comment.
This is a deviation from the design document:
"The Operator will deploy one instance of this Deployment into EVERY Cell listed in 'cells'. If 'cells' is empty, it defaults to all cells where pools are defined."
There was a problem hiding this comment.
Hmm, I see. Can we consider explicitly assigning the list of cells from top level resource (MultigresCluster) to the Shard resources instead? Because the templates resolution results in a very explicit spec for child resources, this "defaulting" logic for child resources feels a bit mismatched.
For now, I changed the logic to follow the documented spec with 105fac6
|
In The Problem: // pkg/resource-handler/controller/shard/shard_controller.go on MAIN branch
// ...
for poolName := range shard.Spec.Pools {
stsName := buildPoolName(shard.Name, poolName) // <--- Returns "shard-pool-primary"
sts := &appsv1.StatefulSet{}
err := r.Get(ctx, ..., Name: stsName, sts)
// ...
}However, your reconciliation logic (
Why Tests Passed: Fix: for poolName, pool := range shard.Spec.Pools {
for _, cell := range pool.Cells {
stsName := buildPoolNameWithCell(shard.Name, poolName, string(cell))
// ... Get and aggregate ...
}
} |
|
You don't need to address this comment now but I thought I would mention it so we can be prepare soon. To follow up with the resolver module I created in preparation for the webhook (to avoid where Here are the specific areas we will need to refactor to align with that design: 1. Centralizing defaults (Single Source of Truth)
2. The "Read-Only Child" & Explicit Specs
3. Webhook Enablement/Disablement // Inside MultigresCluster Reconcile loop
resolvedCluster := resolver.Resolve(cluster)
// Now create Child CRs using resolvedCluster.Spec...This works regardless of whether a webhook ran previously. If the webhook ran, cluster comes in fully populated. If not, Resolve populates it in memory. This means the |
This comment has been minimized.
This comment has been minimized.
🔬 Go Test Coverage ReportSummary
Status✅ PASS DetailShow New Coverage |
|
Re:
I understand where you are coming from, but with our current multi-Go module setup, it's possible one controller could be using one version, whereas other controllers using something else. This was a deliberate design, allowing full control of how operator can handle any cases, including multiple version support (e.g. two minor releases supported at all time, and one patch needs to adjust only part of the logic). I do think such cases would be rather rare, but if we want to make it fully controlled in a single place, we must merge multiple modules into one (namely
I like this approach more, and just not use any explicit version information in the resource-handler code. We should return an error if the image version is not found. Should we try to merge this with the current logic, and create an issue to fix up later? Or do you want me to update the logic in this PR already? |
Yeah let's proceed with the PR as it is and correct later. At this point we should aim for functionality for next week, not perfection, we can circle back afterwards. |
2 participants
This ensures all the API changes are incorporated into resource-handler. Also, some commandline arguments have been corrected.