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.
84b20c9 to
75e251b
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 and Do Not Disrupt annotations to each node when it is cordoned for an update and we remove it after the update is completed. Signed-off-by: Zack Zlotnik <zzlotnik@redhat.com> Assisted-by: AI
75e251b to
30e06a1
Compare
This branch has not been deployed
5 participants
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.