AUTOSCALE-558: Expose KubeletConfig on OpenShiftEC2Nodeclass as structured fields + preserveunknown/overflow#8192
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/valid-reference
|
@jkyros: This pull request references AUTOSCALE-558 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces structured kubelet configuration: a new KubeletConfiguration API type with custom JSON marshal/unmarshal, IsZero semantics, and an optional Sequence Diagram(s)sequenceDiagram
actor User
participant OSE2NC as OpenshiftEC2NodeClass
participant KarpIgnition as KarpenterIgnitionController
participant KubeletCM as Kubelet ConfigMap\n(Management Cluster)
participant EC2NC as EC2NodeClass\n(Karpenter)
participant NP as NodePool\n(Karpenter)
participant Node as Provisioned Node
User->>OSE2NC: Create/Update with spec.kubelet
OSE2NC->>KarpIgnition: Notify controller of change
alt spec.kubelet is set
KarpIgnition->>KarpIgnition: Add kubeletConfigFinalizer
KarpIgnition->>KarpIgnition: Merge user kubelet config + base taints
KarpIgnition->>KubeletCM: Create/Update per-NodeClass ConfigMap (data["config"]=KubeletConfig YAML)
else spec.kubelet is unset
KarpIgnition->>KubeletCM: Delete per-NodeClass ConfigMap
KarpIgnition->>KarpIgnition: Remove kubeletConfigFinalizer if present
end
User->>NP: Create NodePool referencing OpenshiftEC2NodeClass
NP->>EC2NC: Populate EC2NodeClass.Kubelet via KarpenterKubeletConfiguration()
NP->>Node: Provision node referencing Kubelet ConfigMap
Node->>KubeletCM: Read and apply kubelet config
sequenceDiagram
actor Operator
participant KarController as KarpenterController
participant MgrCM as Management ConfigMap\n(karpenter taint)
participant Support as support/karpenter helpers
Operator->>KarController: Reconcile loop
KarController->>Support: KarpenterTaintConfigManifest()
Support-->>KarController: YAML manifest
KarController->>MgrCM: Create/Update global taint ConfigMap (data["config"]=manifest)
MgrCM-->>KarController: Created/Updated or Error
🚥 Pre-merge checks | ✅ 7 | ❌ 5❌ Failed checks (5 warnings)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jkyros The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
area/api
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8192 +/- ##
==========================================
+ Coverage 37.53% 37.63% +0.09%
==========================================
Files 751 751
Lines 92026 92194 +168
==========================================
+ Hits 34544 34695 +151
- Misses 54841 54845 +4
- Partials 2641 2654 +13
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/test e2e-aws-autonode |
|
Heyyy that is super cool, I don't have to have my claude watch and root cause test failures anymore |
|
KubeletConfig passed, teardown failure. One more time |
| // +kubebuilder:object:generate=false | ||
| // +kubebuilder:pruning:PreserveUnknownFields | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.imageGCHighThresholdPercent) || !has(self.imageGCLowThresholdPercent) || self.imageGCHighThresholdPercent > self.imageGCLowThresholdPercent",message="imageGCHighThresholdPercent must be greater than imageGCLowThresholdPercent" | ||
| type KubeletConfiguration struct { |
There was a problem hiding this comment.
I can either come back afterwards and add it, or we can wait for that to merge and then do this one, and I can add it here. I'll set a test up based on your branch in the mean time.
There was a problem hiding this comment.
Added envtest test coverage for KubeletConfiguration
|
/test e2e-aws-autonode |
|
tests are just taking too long i think 😂 We can see TestKarpenter hitting the 2 hour mark which is when it stops. |
openshift-ci
Bot
added
the
needs-rebase
jkyros
force-pushed
the
autoscale-558-kubeletconfig-overflow
branch
from
c11822a to
4d6300e
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
@jkyros: This pull request references AUTOSCALE-558 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
needs-rebase
|
@JoelSpeed I'm generally not a fan of the approach I've taken here, I was trying to unblock this by giving you the closest thing to "validate the fields we care about inside the rawExtension" behavior you desired from our Karpenter sync that I could. We need this for Karpenter GA, but if we're going to have to "OpenShift-ify" all these upstream fields every time it's really going to hurt.
This whole thing is going to be a torture device if we're deviating from upstream in breaking ways. Going forward we'd probably need to not change the fields names/types like we're trying to do in review here. (Types in a breaking way for sure at least, Names...if we leave it so "overflow wins the merge" that's weirdly probably okay because the users existing config will continue to work) 😛
My plan was:
Users are going to be sticking values in spec.Kubelet, and we're going to reconcile all of that to the correct places (ignition, karpenter ec2nodeclass) from there. We have full control.
It does, but "overflow" currently wins. I can just re-order that block so the structured fields marshal in afterwards, but someone would have to intentionally abuse that for it to matter. EDIT: actually I think overflow winning is good, that means that if we change the name, their existing config will work without breaking, and we could reconcile that under the hood (e.g. they specify |
jkyros
force-pushed
the
autoscale-558-kubeletconfig-overflow
branch
2 times, most recently
from
7adb331 to
26ae4c2
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/test e2e-aws |
jkyros
force-pushed
the
autoscale-558-kubeletconfig-overflow
branch
from
26ae4c2 to
b4b1fa2
Compare
openshift-ci
Bot
added
the
area/ci-tooling
|
jkyros
force-pushed
the
autoscale-558-kubeletconfig-overflow
branch
from
ed024e1 to
844d836
Compare
|
Quay are you done being broken? |
|
quay, you said you were fixed, but that doesn't seem fixed: |
|
Let's see if quay is sane again, and I don't know how codecov can flake, but it is. |
|
I expect that image stream being globally broken is going to prevent tests from working, but let's try: |
|
@jkyros: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll four failures on PR #8192 are unrelated to the PR's code changes. The two Prow jobs ( Root CauseThe root cause is a missing container image in the OCP 5.0 CI release stream. Specifically:
Recommendations
Evidence
|
| @@ -0,0 +1,147 @@ | |||
| apiVersion: apiextensions.k8s.io/v1 | |||
There was a problem hiding this comment.
nit: it be nice to populate Expected: for these cases
There was a problem hiding this comment.
Thanks, populated expected for these
|
|
||
| // Overflow holds additional kubelet configuration fields not explicitly defined above. | ||
| // These fields are preserved during serialization and deserialization, allowing arbitrary | ||
| // kubelet configuration to pass through to the node's ignition/MachineConfig. |
There was a problem hiding this comment.
nit: may be mention overflow fields bypass all CRD validation, invalid values will manifest as node bootstrap failures (kubelet crash loop), not admission errors.
There was a problem hiding this comment.
Updated language to mention the kubelet crash loop
|
|
||
| When graduating a field from overflow to a typed struct field: | ||
|
|
||
| - **Match upstream Karpenter's field name and JSON tag exactly.** Our structured fields must use the |
There was a problem hiding this comment.
what happens with overflow fields set in existing configs, if upstream Karpenter introduce support for it, and we want to promote it but karpenter upstream choose to differ from upstream kubelet on how the semantic is exposed?
There was a problem hiding this comment.
I think we actually want to match kubelet. Users today will set kubelet fields which end up in our overflow, and we want to maintain compatibility with those. If Karpenter decide to change the field, that would break our upgrades if we made the field structured and kept them aligned to karpenter, so we would have to align to kubelet, then translate to karpenter (so that it can then translate back to kubelet)
There was a problem hiding this comment.
I agree. Match kubelet. I changed that one weird EvictionSoftGracePeriod Karpenter time.Duration back to a string (so we're matching upstream Kube not Karpenter, it will serialize the same) and updated the AGENTS, etc language to match this.
| // +kubebuilder:validation:MinProperties=1 | ||
| // +optional | ||
| KubeReserved map[string]string `json:"kubeReserved,omitempty"` | ||
| // evictionHard is a map of signal names to quantities that defines hard eviction thresholds. |
There was a problem hiding this comment.
do we need to validate that evictionHard and EvictionSoft doesn't clash any values?
There was a problem hiding this comment.
Added as much CEL as we can to validate this.
They can be either quantities or percentages (yay!) and that makes it difficult to compare since CEL doesn't know node capacity. But we can can compare percentages to percentages and values to values so I added what ended up being kind of a gnarly expression that compares them if it can and fails open if they are mix/match quantity/percentage.
EDIT: Nope. Too gnarly. quantity() and compareTo() are too expensive. And apparently...I can't put max lengths on the map key strings to help without type aliasing them so I have somewhere to attach. Yuck. (unless we want to hack on the CRD like adjust-cel.sh does or something? I don't think we do )
I'm going to turn into Joel here with the "ugh, this API sucks" 😛
Yeah I think (because contents can be either type) we either have to hack on the CRD or type alias it to pass budget and still validate.
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
On each of the maps, could we add a maximum limit too? Seems like the realistic user case is ~4/5 entries in each map, what if we limited to 32 to give plenty of buffer but bring estimated CEL costs down?
| // When graduating new fields from overflow to typed fields, match upstream Karpenter's | ||
| // field names and types exactly. See api/AGENTS.md "KubeletConfiguration Field Graduation" |
There was a problem hiding this comment.
Is it karpenter, or kubelet that we must match?
There was a problem hiding this comment.
Matching Kubelet, adjusted accordingly.
…eClass Add KubeletConfiguration type with custom JSON marshal/unmarshal to support both explicitly typed fields (maxPods, systemReserved, eviction thresholds, etc.) and arbitrary overflow fields that pass through to the node's kubelet config via MachineConfig. Typed fields get CEL validation at admission time (range checks, cross-field rules like imageGCHigh > imageGCLow and evictionSoft >= evictionHard, key-set validation on maps). Overflow fields bypass CRD validation entirely — invalid values surface as kubelet crash loops at node bootstrap, not admission errors. The overflow mechanism uses a runtime.RawExtension with json:"-" and custom MarshalJSON/UnmarshalJSON to split known fields into the struct and unknown fields into overflow. On marshal, structured fields win over overflow on conflict. Field types match upstream kubelet (k8s.io/kubelet/config/v1beta1) as the primary compatibility target, with upstream Karpenter as a secondary reference. The one deliberate deviation is evictionMaxPodGracePeriod (*int32 vs kubelet's int32) required by the API linter since 0 is a valid value with no Minimum constraint. Signed-off-by: John Kyros <jkyros@redhat.com>
…d mapping Wire the KubeletConfiguration from OpenshiftEC2NodeClass through the karpenter-operator's ignition controller to inject kubelet settings into node ignition via MachineConfig. Add type mapping function (karpenterKubeletConfigurationFromNodeClassSpec) that converts our kubelet types to upstream Karpenter types, including a string-to-Duration conversion for evictionSoftGracePeriod where our API matches kubelet's map[string]string but Karpenter uses map[string]metav1.Duration. Move taint ConfigMap creation from the hypershift-operator to the karpenter-operator for centralized management. Signed-off-by: John Kyros <jkyros@redhat.com>
jkyros
force-pushed
the
autoscale-558-kubeletconfig-overflow
branch
from
a2887fe to
31ee3c0
Compare
|
envtest...cancelled itself? hmmm |
Add envtest suites for CEL validation rules including threshold ordering, field promotion ratcheting, and generated ratcheting tests. Add e2e test infrastructure for verifying kubelet config on nodes. Signed-off-by: John Kyros <jkyros@redhat.com>
jkyros
force-pushed
the
autoscale-558-kubeletconfig-overflow
branch
from
31ee3c0 to
0a5d587
Compare
|
/test e2e-aws-autonode |
5 participants
What this PR does / why we need it:
OpenShiftEC2NodeClassas a set of structured fields (the ones Karpenter needs for scheduling/bin packing) + preserves unknownec2nodeclassso it can use themWhich issue(s) this PR fixes:
Fixes
AUTOSCALE-558
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
Tests