Record installation defaults in the status instead of the spec - #5203
Open
caseydavenport wants to merge 11 commits into
Open
Record installation defaults in the status instead of the spec#5203caseydavenport wants to merge 11 commits into
caseydavenport wants to merge 11 commits into
Conversation
caseydavenport
added
kind/enhancement
caseydavenport
marked this pull request as ready for review
August 18, 2026 12:34
caseydavenport
commented
Aug 18, 2026
Defaulting seeds from the recorded values, so a user-set field takes over and drops out of the recorded set.
Downstream controllers read the computed spec and wait until the core controller publishes it.
The pool list merges whole, so a declared pool that omits the name dropped the recorded default for it. Per-pool fields now come from the recorded default with the same CIDR, and the IP pool controller diffs its defaults against the raw spec.
Each controller declares the spec paths it defaults, and downstream controllers read the computed spec published on the status.
Keeps the object we write back exactly as the user declared it, so the patch and the status writes don't need to restore the spec afterwards.
Records IP pool defaults per declared pool instead of all-or-nothing, so a user's own pools stay theirs.
Stops the API server injecting field defaults into the status, which caused a rewrite every reconcile.
caseydavenport
force-pushed
the
casey-installation-defaults
branch
from
August 20, 2026 13:12
41e8c6d to
0e3a5d8
Compare
5 tasks
tmjd
reviewed
Aug 21, 2026
tmjd
left a comment
tmjd
left a comment
Member
There was a problem hiding this comment.
All the code for having the IPPools managed by the ippool controller is quite complicated. I'm not a big fan of that.
| // We rely on the core controller for defaulting, so wait until it has done so before continuing | ||
| if reflect.DeepEqual(instanceStatus, operatorv1.InstallationStatus{}) { | ||
| err := fmt.Errorf("InstallationStatus is empty") | ||
| r.status.SetDegraded(operatorv1.ResourceNotReady, "InstallationStatus is empty", err, reqLogger) |
Member
There was a problem hiding this comment.
We use to degrade in this case, should we do the same now when status.Computed is not set?
| return nil | ||
| } | ||
|
|
||
| // Write a copy, so the response can't replace the effective spec we're working from. |
Member
There was a problem hiding this comment.
That's interesting that this is needed. I wouldn't have thought that Status().Update would not have updated the main spec in what was passed to it.
| } | ||
|
|
||
| // Write default IP pool configuration back to the Installation object using patch. | ||
| preDefaultPatchFrom := client.MergeFrom(installation.DeepCopy()) |
Member
There was a problem hiding this comment.
Any idea why we patched before? I'm wondering if we should still be patching for updating the defaults
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The operator writes its computed defaults back into the Installation spec, which makes the operator the field manager for anything it defaulted. Helm and Argo then conflict on those fields, and a server-side apply from the chart gets rejected. See #5102.
This changes the defaulting to leave the spec alone:
Also fixes a bug this uncovered: an empty ipPools list marshalled the same as an unset one, so asking for no IP pools got the default pool instead. The documented behavior now holds.
IP pool semantics, spelled out in the CRD docs as part of this:
ipPools: []means no pools. The operator deletes the pools it owns and creates none.ipPoolsleaves the choice to the operator. On a cluster where it has already recorded pool defaults, that keeps the pools in use, where before it deleted them.First-install pool defaulting takes two status round-trips: the IP pool controller records the pool defaults, then the core controller republishes the effective config with them included. A few FV timeouts went up to cover that window.
Existing clusters where the operator already owns spec fields need one
helm upgrade --force-conflictsto hand ownership back to the chart.Constants that could be plain CRD schema defaults are left alone here - that's a follow-up.
Related: CORE-13376