Repository navigation
MCO-2599: makes bootc-operator aware of karpenter - #204
cheesesashimi wants to merge 1 commit into
Conversation
cedee9d to
902f247
Compare
| log.Info("Cordoning node", "node", node.Name) | ||
| modifiedNode := node.DeepCopy() | ||
| modifiedNode.Spec.Unschedulable = true | ||
| metav1.SetMetaDataAnnotation( |
There was a problem hiding this comment.
is there an annotation to understand if the nodes have been provisioned by karpeneter and eventually adding this annotation conditionally? Otherwise, it is always there. Not that it does any harm but just wondering if we could keep the node clean if there is no karpenter at all involved
There was a problem hiding this comment.
There are two ways to do this conditionally: Using the Kubernetes Discovery API and looking for CustomResourceDefinitions (CRD). I've outlined their benefits and drawbacks here:
Discovery API
An issue we could run into with the Discovery API is that it does not have informers, watchers, etc., which means that this API is not event-driven. To avoid unnecessary load on the API server, client-go provides a caching Discovery API client (link). However, one must still define the cache invalidation policy, meaning that we could end up with a race where Karpenter could reap a node unexpectedly. Here's how that could happen:
- A cluster admin starts a bootc-operator update.
- While the bootc-operator update is in progress, Karpenter is installed.
- The Discovery API cache has not been invalidated and bootc-operator selects the next node for an update without applying the do-not-repair annotation.
- The node update process takes longer than expected and Karpenter reaps the node because the do-not-repair annotation was not present.
While the concerns around this are likely overblown, it nevertheless is an edge-case that we should be able to gracefully handle.
Watching for CustomResourceDefinitions
We could add a watcher for CustomResourceDefinitions (CRD) which would allow us to be notified immediately whenever a given CRD is installed or deleted. In this case, we can check for the presence of Karpenter CRDs and adjust the rollout accordingly. This approach would not completely eliminate the possibility of a race, but it would reduce it substantially. It would require some additional permissions for the controller, but it is doable.
Both of those solutions increase complexity and in this particular situation, I'm not sure that it is necessary since adding / removing this annotation unconditionally will do nothing if Karpenter is not installed.
There was a problem hiding this comment.
is there an annotation to understand if the nodes have been provisioned by karpeneter and eventually adding this annotation conditionally?
I revisited this question today and determined that the best way to determine whether the nodes' lifecycle is managed by Karpenter is to check if an OwnerReference exists which points to a Karpenter NodeClaim object which would be present in that case. So we could conditionally set the annotation in that case.
| if metav1.HasAnnotation(node.ObjectMeta, karpenterDoNotRepairAnnotationKey) { | ||
| delete(modified.Annotations, karpenterDoNotRepairAnnotationKey) | ||
| } |
There was a problem hiding this comment.
curious, why do we need to delete?
There was a problem hiding this comment.
The only time we want Karpenter to ignore a node is when we're performing an update. At any other time, if the node enters a failure mode that Karpenter can remediate (e.g., by deleting / replacing it), we want Karpenter to be able to handle it.
|
This requires a rebase to include the testing for k8s 1.37 |
902f247 to
84b20c9
Compare
| bootc-operator can run alongside [Karpenter]. During a node update, the | ||
| operator drains and reboots one or more nodes. To prevent Karpenter from | ||
| repairing or replacing a node while that operation is in progress, the | ||
| operator uses the `karpenter.sh/do-not-repair` annotation. |
There was a problem hiding this comment.
What about do-not-disrupt feels like this should also be applied during update?
There was a problem hiding this comment.
The do-not-disrupt annotation is used for identifying workloads that Karpenter should not disrupt whereas the do-not-repair annotation is used to identify nodes that Karpenter should ignore once that node meets Karpenters criteria for repair / replacement.
There was a problem hiding this comment.
Maybe I don't understand this correctly, I haven't launched a cluster to try. But, I think if a node as the do-not-repair annotation then it is not protected from consolidation and drift event from Karpenter?
So with my limited understanding, if the bootc-operator drains a node with the annotation do-not-repair. Karpenter would see that node being empty (ie no pods running) and mark it for Consolidation (deletion). I guess there might be some race involved here but that could happen.
There was a problem hiding this comment.
I was incorrect about the do-not-disrupt annotation. After doing a bit more research, it looks like we'll need both annotations for this to work correctly. I'll update this PR to include both of them. Thanks for challenging my assumptions here!
There was a problem hiding this comment.
No worries, trying to wrap my head around this, so thanks for being my sounding board 😊
There was a problem hiding this comment.
After doing a bit more research, even with both of those annotations set, it is still possible that a node could be reaped while a bootc update is in progress. However, in that case, it is due to expiration and forceful termination policies and not because of the bootc update. I'll be sure to put this in the documentation that I'm including with this PR.
| complexity than benefit, and clusters without Karpenter simply do not consume | ||
| these annotations. | ||
|
|
||
| ## Annotation lifecycle |
There was a problem hiding this comment.
How do both operator works together, IIUC Karpenter will replace the whole fleet when one updates the AMI refs.
When using the bootc-operator should an Admin freeze the AMI refs in the Karpenter objects?
Then when Karpenter scale-out new nodes it will start from an old AMI which would trigger the bootc-operator to update and reboot? This is not great and we have just recreated our problem with old boot-images :-(
I am not sure if we have a good solution for this 🤔
There was a problem hiding this comment.
Karpenter will replace the whole fleet when one updates the AMI refs
I think it depends on how many NodeClasses and NodePools one has. If all of the cluster nodes are in a single NodeClass and its AMI is updated, then yes, Karpetner would replace the whole fleet.
When using the bootc-operator should an Admin freeze the AMI refs in the Karpenter objects?
As I understand it, these are frozen by default in Karpenter, although one can use a more dynamic AMI selection if they wanted to.
Then when Karpenter scale-out new nodes it will start from an old AMI which would trigger the bootc-operator to update and reboot? This is not great and we have just recreated our problem with old boot-images :-(
It is the boot-image problem again. But I think that the solution here could be easier, provided that one has multiple NodeClasses and BootcNodePools since one could then use a blue/green rollout pattern. We'd need to come up with some guidance around that.
There was a problem hiding this comment.
Yes I think the emphasis/technology of Karpenter clashes with bootc-operator.
I would say there's something a bit more general going on here in that actually most uses of Kubernetes can be ~stateless by default (which is not what bootc defaults to, but can be opted-into), where you don't even support in-place updates in general.
But that approach gets hard in some cases where one has good reasons for machines with a more persistent identity and state (sometimes e.g. databases and the like), and that's where a more bootc-like approach can shine.
It is the boot-image problem again.
Hmm but that's only in the case where there is "AMI vs container" skew - in most cases one can update to the latest AMI/disk-image by default for new nodes right?
But I think that the solution here could be easier, provided that one has multiple NodeClasses and BootcNodePools since one could then use a blue/green rollout pattern. We'd need to come up with some guidance around that.
I am not fully following this, would this be "blue" is in-place updates and "green" is newly provisioned? But yes blue/green for pure bootc (or Karpenter) make sense.
Offhand, it feels to me like use of bootc-operator with Karpenter would be a thing where one would want to opt-in for specific systems?
There was a problem hiding this comment.
Yes I think the emphasis/technology of Karpenter clashes with bootc-operator.
I agree. Although the two technologies clash, we should ensure that they clash in a known and graceful manner, which is what this PR is for.
Hmm but that's only in the case where there is "AMI vs container" skew - in most cases one can update to the latest AMI/disk-image by default for new nodes right?
Yes they can.
I am not fully following this, would this be "blue" is in-place updates and "green" is newly provisioned? But yes blue/green for pure bootc (or Karpenter) make sense.
Yes. In the example I'm thinking about, blue are nodes that were provisioned from an older AMI and have been in-place updated to the point where the AMI / container image skew is too big. And green are nodes that were provisioned from the latest AMI. And over time, the blue nodes are replaced with green nodes. To my understanding, this is possible today with Karpenter. If someone is using bootc-operator + Karpenter, then they'd have to include BootcNodePools as well.
Offhand, it feels to me like use of bootc-operator with Karpenter would be a thing where one would want to opt-in for specific systems?
I agree. Though I'd also argue that by installing both operators at the same time that one is opting into this. If that was done right now, they must be careful to configure both operators so they won't act on the same nodes. But what happens if they either don't do that or they add configs (intentionally or otherwise) which allow both operators to act on the same nodes?
In the best-case scenario, bootc-operator might complete the update before Karpenter can reap the node. In the worst-case scenario, it could lead to infinite node churn, resembling an infinite loop:
- bootc-operator cordons and drains the node to perform the in-place update.
- Karpenter thinks the node is "unhealthy" or "empty", so it removes the node and provisions a replacement.
- bootc-operator eventually selects the replacement node for an update because the bootc image does not match the desired state.
- GOTO 1.
In this situation, we should prevent the node churn from happening by teaching bootc-operator to tell Karpenter not to attempt to reap the nodes it is currently updating for reasons relating to the node being empty or unhealthy. And that's what this PR does.
There was a problem hiding this comment.
Though I'd also argue that by installing both operators at the same time that one is opting into this.
Agreed!
In this situation, we should prevent the node churn from happening by teaching bootc-operator to tell Karpenter not to attempt to reap the nodes it is currently updating for reasons relating to the node being empty or unhealthy. And that's what this PR does.
SGTM!
75e251b to
30e06a1
Compare
| // Patch the K8s Node: cordon it and, when we own the Karpenter | ||
| // annotations, set them. All are idempotent; we combine them into a | ||
| // single patch to minimise API calls. | ||
| if !node.Spec.Unschedulable || shouldOwnDoNotRepair || shouldOwnDoNotDisrupt { |
There was a problem hiding this comment.
do we really need this if if then we are evaluating each conditions?
There was a problem hiding this comment.
You could use a early return and avoid the nested if, like:
| if !node.Spec.Unschedulable || shouldOwnDoNotRepair || shouldOwnDoNotDisrupt { | |
| if node.Spec.Unschedulable && !shouldOwnDoNotRepair && !shouldOwnDoNotDisrupt { | |
| return nil | |
| } |
There was a problem hiding this comment.
Thanks for the suggestion! I'll get that fixed up.
30e06a1 to
0c093f5
Compare
|
FTR this came up in the community meeting today, which I know is an unfriendly time for most people here except @cheesesashimi |
|
Triage of cgwalters's comment: claimed; tracked in cgwalters-forge/tracker#701. Generated-by: https://github.com/cgwalters/#llms |
|
@cheesesashimi are you confident that this works or should we test the change with karpenter first? |
|
@Mergifyio backport release-2.0 |
🟠 Waiting for conditions to matchDetails
|
8801b7a to
473cf94
Compare
473cf94 to
e2c1dc7
Compare
|
@alicefr I did some testing with this and Karpenter today and overall, I'm pleased with the results. bootc-operator set the appropriate annotations which prevented Karpenter from draining the nodes while bootc-operator was actively working on them. However, I encountered an interesting behavior that we should be aware of and document, which I included in this PR. Although bootc-operator takes precautions to ensure that Karpenter does not replace a node while an update is in progress, it is possible that Karpenter may replace the node(s) after the update is complete. Here's how:
In this scenario, Karpenter did not replace the node while the bootc update process was active. Instead, Karpenter replaced it after the bootc update was complete and because the node was empty. There are ways to configure Karpenter so that it does not do this, but setting up and managing those configs is not something that bootc-operator should do. |
e2c1dc7 to
9ee7cca
Compare
Nice findings and thanks for the heads up. I agree this is something we need to make karpenter aware of or properly configure it. |
| The controller represents NodeClaims with Kubernetes `Unstructured` objects | ||
| instead of importing Karpenter's Go API types. This keeps Karpenter an optional | ||
| integration and avoids adding a Karpenter API dependency or scheme registration | ||
| to bootc-operator. It also lets clusters without the NodeClaim CRD continue to | ||
| run normally. The tradeoff is that the small set of fields used here, such as | ||
| `status.nodeName` and `status.conditions`, is checked at runtime rather than | ||
| being enforced by Go types. |
There was a problem hiding this comment.
Isn't this an implementation detail?
There was a problem hiding this comment.
It is. I did want the rationale behind the Unstructured choice documented someplace. If there's a better place to put it, then I'm open to that.
| if apimeta.IsNoMatchError(err) { | ||
| // CRD not installed — Karpenter is not present on this cluster. | ||
| return nil, karpenterClaimCRDMissing, nil | ||
| } | ||
| if apierrors.IsNotFound(err) { | ||
| // Owner ref points to a NodeClaim that no longer exists. | ||
| // This is unsafe: Karpenter may be replacing or terminating | ||
| // the node right now. | ||
| return nil, karpenterClaimAbsent, nil | ||
| } | ||
| return nil, karpenterClaimError, err |
There was a problem hiding this comment.
nit:maybe it we could encapsulate the if in a switch case
There was a problem hiding this comment.
Agreed. A switch case is easier to reason about here.
| func (r *BootcNodePoolReconciler) isKarpenterDisruptionActive( | ||
| ctx context.Context, | ||
| node *corev1.Node, | ||
| ) (active bool, claimMissing bool, claim *unstructured.Unstructured, err error) { |
There was a problem hiding this comment.
nit: can we return a struct and and error instead of 4 variables?
There was a problem hiding this comment.
Agreed. I'll make this a bit better.
| continue | ||
| } | ||
| switch condition["type"] { | ||
| case "Rebooting", "DisruptionReason", "InstanceTerminating": |
There was a problem hiding this comment.
are there already any constants existing for those conditions we could reuse?
There was a problem hiding this comment.
I'll add some.
|
|
||
| // Build desired BootcNode state. | ||
| modifiedBN := bn.DeepCopy() | ||
| if node.Annotations[karpenterDoNotRepairAnnotationKey] != "true" { |
There was a problem hiding this comment.
what if Annotation in nil?
There was a problem hiding this comment.
It is possible that the map is nil, though it is unlikely here. In Golang, checking for key membership in a nil map is safe and will not cause a panic. However, writing to a nil map will cause a panic. Using metav1.SetMetaDataAnnotation() performs a nil check before attempting to write to the map and will initialize the map before writing if it is nil.
| } | ||
| if claimAvailable { | ||
| claimAnnotations := claim.GetAnnotations() | ||
| if claimAnnotations[karpenterDoNotRepairAnnotationKey] != "true" { |
There was a problem hiding this comment.
Same here, can the annotations be nil?
| if claimAvailable { | ||
| claimAnnotations := claim.GetAnnotations() | ||
| modifiedClaimAnnotations := map[string]string{} | ||
| maps.Copy(modifiedClaimAnnotations, claimAnnotations) | ||
| if claimAnnotations[karpenterDoNotRepairAnnotationKey] != "true" { | ||
| modifiedClaimAnnotations[karpenterDoNotRepairAnnotationKey] = "true" | ||
| } | ||
| if claimAnnotations[karpenterDoNotDisruptAnnotationKey] != "true" { | ||
| modifiedClaimAnnotations[karpenterDoNotDisruptAnnotationKey] = "true" | ||
| } | ||
| if !annotationsEqual(claimAnnotations, modifiedClaimAnnotations) { | ||
| modifiedClaim := claim.DeepCopy() | ||
| modifiedClaim.SetAnnotations(modifiedClaimAnnotations) | ||
| if err := r.Patch(ctx, modifiedClaim, client.MergeFrom(claim)); err != nil { | ||
| return fmt.Errorf("patching NodeClaim for staging protection: %w", err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| if claimAvailable { | |
| claimAnnotations := claim.GetAnnotations() | |
| modifiedClaimAnnotations := map[string]string{} | |
| maps.Copy(modifiedClaimAnnotations, claimAnnotations) | |
| if claimAnnotations[karpenterDoNotRepairAnnotationKey] != "true" { | |
| modifiedClaimAnnotations[karpenterDoNotRepairAnnotationKey] = "true" | |
| } | |
| if claimAnnotations[karpenterDoNotDisruptAnnotationKey] != "true" { | |
| modifiedClaimAnnotations[karpenterDoNotDisruptAnnotationKey] = "true" | |
| } | |
| if !annotationsEqual(claimAnnotations, modifiedClaimAnnotations) { | |
| modifiedClaim := claim.DeepCopy() | |
| modifiedClaim.SetAnnotations(modifiedClaimAnnotations) | |
| if err := r.Patch(ctx, modifiedClaim, client.MergeFrom(claim)); err != nil { | |
| return fmt.Errorf("patching NodeClaim for staging protection: %w", err) | |
| } | |
| } | |
| } | |
| if claimAvailable { | |
| ann := claim.GetAnnotations() | |
| needsRepair := ann[karpenterDoNotRepairAnnotationKey] != "true" | |
| needsDisrupt := ann[karpenterDoNotDisruptAnnotationKey] != "true" | |
| if needsRepair || needsDisrupt { | |
| modifiedClaim := claim.DeepCopy() | |
| modifiedAnn := modifiedClaim.GetAnnotations() | |
| if modifiedAnn == nil { | |
| modifiedAnn = make(map[string]string) | |
| } | |
| modifiedAnn[karpenterDoNotRepairAnnotationKey] = "true" | |
| modifiedAnn[karpenterDoNotDisruptAnnotationKey] = "true" | |
| modifiedClaim.SetAnnotations(modifiedAnn) | |
| if err := r.Patch(ctx, modifiedClaim, client.MergeFrom(claim)); err != nil { | |
| return fmt.Errorf("patching NodeClaim for staging protection: %w", err) | |
| } | |
| } | |
| } |
| } | ||
|
|
||
| func TestKarpenterRebootActive(t *testing.T) { | ||
| claim := &unstructured.Unstructured{Object: map[string]interface{}{ |
There was a problem hiding this comment.
why using unstructured object? Cannot we import the karpenter crd definitions?
There was a problem hiding this comment.
By using the unstructured API, we can enforce the idea that Karpenter integration is completely optional. While this is still possible with importing the Karpenter API objects, it seems overkill for the type of integration that we want to have. That said, I don't have a hard opinion about this since regardless of whether we import the API objects or not, we still have runtime checks.
|
This PR requires a rebase |
c9702c1 to
8e5f5b8
Compare
| When a Karpenter-owned node enters the `Staging` state (the daemon is pulling | ||
| the new image), bootc-operator immediately sets `do-not-repair` and | ||
| `do-not-disrupt` on the Node and, where available, the associated NodeClaim. | ||
| This protection persists through slot assignment, drain, reboot, and post-boot | ||
| health verification. |
There was a problem hiding this comment.
I'm wondering if Staging is the right phase and not too early. Right now, the phases are one after the other, but what about when we introduce the maintenance windows where the reboot is postponed. Isn't better to set those annotation when the node occupies a reboot slot? I think that maybe a node might not be longer needed but if it is still in staging/staged but not rebooted immediatly cannot be removed by Karpenter
|
@cheesesashimi this single commit is quite huga, would be possible to split into multiple ones? |
Protect Karpenter-managed nodes from repair and voluntary disruption throughout image staging, reboot, and health verification, including their NodeClaims when available. Detect active Karpenter disruptions and reboots while deferring node drains and/or reboots until completion, preserving externally managed annotations and cordon state. This also handles the case where there is drift between the boot image for a given node and the desired bootc node image, provided that the node has the well-known taint, bootc.dev/os-not-ready:NoSchedule Assisted-by: AI Signed-off-by: Zack Zlotnik <zzlotnik@redhat.com>
8e5f5b8 to
81f2384
Compare
If a cluster contains both bootc-operator and Karpenter, it is possible that Karpenter could reap cluster nodes while a bootc-operator driven update is in progress. To prevent that, we detect whether the Node object is owned by a Karpenter NodeClaim object. And in that situation, we apply both the Karpenter Do Not Repair (
karpenter.sh/do-not-repair) and Do Not Disrupt (karpenter.sh/do-not-disrupt) annotations to each node when it is cordoned for an update and we remove it after the update is completed.