-
Notifications
You must be signed in to change notification settings - Fork 430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ASOAPI: propagate machinepool values to ManagedClustersAgentPools #4798
Conversation
/hold for #4794 |
/milestone v1.15 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4798 +/- ##
==========================================
+ Coverage 62.66% 62.73% +0.06%
==========================================
Files 198 199 +1
Lines 16420 16507 +87
==========================================
+ Hits 10290 10356 +66
- Misses 5369 5381 +12
- Partials 761 770 +9 ☔ View full report in Codecov by Sentry. |
} else if replicaManager != infrav1exp.ReplicasManagedByAKS { | ||
return fmt.Errorf("failed to enable autoscaling, replicas are already being managed by %s according to MachinePool %s's %s annotation", replicaManager, machinePool.Name, clusterv1.ReplicasManagedByAnnotation) | ||
} | ||
} else if !autoscaling && replicaManager == infrav1exp.ReplicasManagedByAKS { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment here to indicate that this implements the required reconciliation logic to enable mutating from "node pool is autoscaler-enabled" to "node pool is not autoscaler-enabled"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I exactly get what you mean, but I took a stab here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya this works
this lgtm w/ some minor comments |
/hold only for squash now that #4794 has merged. |
It wasn't automatic, but merging main seemed to work so those old changes don't appear in the diff anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
LGTM label has been added. Git tree hash: 1599f67ed609ef80db50dbf0f9e1a8ad53b3baca
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
squashed! (to the original two commits) |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR uses the ASOAPI mutator framework to set the Kubernetes version and replica count for ASO agent pools based on its owning MachinePool. It also includes handling for autoscaling AKS node pools.
The first commit in this PR is the same as what's queued up to merge in #4794. It should disappear from this PR automatically once that other PR merges. I'm electing to create this PR now to open up these new changes for feedback in the meantime.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #4713
Special notes for your reviewer:
TODOs:
Release note: