[cluster autoscaling] initial commit for controller - #42171
Conversation
d12f4e7 to
b9a71c6
Compare
Go Package Import DifferencesBaseline: 13e01c0
|
b9a71c6 to
a741e13
Compare
a741e13 to
eae5682
Compare
1357c21 to
e9e0970
Compare
celenechang
added
the
qa/done
| type store = autoscaling.Store[minNodePool] | ||
|
|
||
| const datadogCreatedLabelKey = "datadoghq.com/datadog-cluster-autoscaler.created" | ||
| const datadogModifiedLabelKey = "datadoghq.com/datadog-cluster-autoscaler.modified" |
There was a problem hiding this comment.
Would you like to track the RC version or observed generation? It could help reduce the number of reconciles.
| } | ||
|
|
||
| func (c *Controller) patchNodePool(ctx context.Context, np *karpenterv1.NodePool, mnp minNodePool) error { | ||
| log.Infof("Patching NodePool: %s", mnp.name) |
There was a problem hiding this comment.
Why did you choose to use Patch instead of a simple Update? It would be much simpler to just have a function that builds a NodePool from the K8S NodePool + NodePoolRecs
There was a problem hiding this comment.
i went with patch because it seems to go with the general flow of keeping the items in the store minimal, and minimally updating the cluster resource (just a couple fields) compared to the potential size of a nodePoolSpec. i'm not sure it would be simpler, but if it's better to go with update i can change that
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| if isCreatedByDatadog(nodePool.GetLabels()) { |
There was a problem hiding this comment.
When processing a workqueue key for a NodePool that isn’t present in the store and is missing in the cluster (e.g., delete events for unmanaged or already-removed NodePools), syncNodePool still dereferences nodePool.GetLabels(). With nodePool == nil this panics and takes down the controller instead of safely ignoring the event. A nil check is needed before using the NodePool object in this branch.
Useful? React with 👍 / 👎.
| sender, err := senderManager.GetSender("cluster_autoscaling") | ||
| sender.DisableDefaultHostname(true) | ||
| if err != nil { |
There was a problem hiding this comment.
StartClusterAutoscaling calls sender.DisableDefaultHostname(true) before verifying the result of senderManager.GetSender. If sender creation fails (for example if the demultiplexer isn’t initialized), GetSender returns an error and nil sender, and this line will panic before returning the error to the caller, preventing the controller from starting cleanly. The error should be handled before dereferencing the sender.
Useful? React with 👍 / 👎.
Co-authored-by: Lénaïc Huard <L3n41c@users.noreply.github.com>
4dac434 to
7844a07
Compare
|
/merge -@ 8am CET |
|
View all feedbacks in Devflow UI.
Pull Request scheduled to be added to the queue on Fri, 28 Nov 2025 07:00:00 UTC
This merge request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
devflow unqueued this merge request: It did not become mergeable within the expected time |
8f8af46 to
6c18314
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
PR can't be merged according to github policy |
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
adel121
left a comment
adel121
left a comment
There was a problem hiding this comment.
LGTM for @DataDog/container-platform owned files. 👍
### What does this PR do? Adds cluster autoscaling controller ### Motivation ### Describe how you validated your changes ### Additional Notes Co-authored-by: vboulineau <vincent.boulineau@datadoghq.com> (cherry picked from commit c585dbd)
### What does this PR do? Remove the `v0.60.0` version pin on `otelgrpc`, thus making `v0.69.0` actual. ### Motivation The TODO named kubernetes/apiserver@b887c9eb ("Drop usages of deprecated otelgrpc methods") as the removal condition. That commit indeed first landed in `k8s.io/apiserver v0.34.0`. It turns out that a number of PRs had put the agent way past that threshold: | PR | `k8s.io/apiserver` | |--------|----------------------------| | #37844 | v0.32.2 -> **v0.34.1** | | #42171 | -> v0.35.0-alpha.0 | | #46808 | -> v0.35.0 | | #48796 | -> v0.35.3 | | #49973 | -> v0.35.4 | | #51049 | -> v0.35.5 | Dropping the stale `replace` directive is therefore safe and sound.
### What does this PR do? Remove the `v0.60.0` version pin on `otelgrpc`, thus making `v0.69.0` actual. ### Motivation kubernetes/apiserver@b887c9e ("Drop usages of deprecated otelgrpc methods") was named as the removal condition: ``` // TODO(songy23): remove this once kubernetes/apiserver@b887c9e is brought to agent ``` That commit indeed first landed in `k8s.io/apiserver v0.34.0`. It turns out that a number of PRs had put the agent way past that threshold: | PR | `k8s.io/apiserver` | |--------|----------------------------| | #37844 | v0.32.2 -> **v0.34.1** | | #42171 | -> v0.35.0-alpha.0 | | #46808 | -> v0.35.0 | | #48796 | -> v0.35.3 | | #49973 | -> v0.35.4 | | #51049 | -> v0.35.5 | Dropping the stale `replace` directive is therefore safe and sound. Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
12 participants
What does this PR do?
Adds cluster autoscaling controller
Motivation
Describe how you validated your changes
Additional Notes