Refactored expiration/utilization controller into node controller #594
Conversation
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: 6a94249 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/610ad8984b1cdb000711227c |
ellistarn
force-pushed
the
nodecontroller
branch
from
df38956 to
ad7f772
Compare
| } | ||
| return requests | ||
| }), | ||
| ). | ||
| WithOptions(controller.Options{MaxConcurrentReconciles: 10}). |
There was a problem hiding this comment.
I realize this was included in a prior PR, but why did we pick 10 here? Are there any detriments to bumping this number up?
There was a problem hiding this comment.
We could consider setting this to kubeclient qps (default 20). I found that 10 was reasonably high parallelism without exhausting default qps. Perhaps we can evaluate this number as we do more scale testing.
pkg/controllers/node/suite_test.go
Outdated
| node = ExpectNodeExists(env.Client, node.Name) | ||
| Expect(node.DeletionTimestamp.IsZero()).To(BeTrue()) | ||
| }) | ||
| It("should terminate nodes after expiry", func() { |
There was a problem hiding this comment.
Do we have the ability to simulate time going by in these tests? I worry a bit that there might be some code paths not quite exercised here ... compared to having a TTL set (to a non-zero value like 30), prove that (say) 15 seconds in the node is not marked for termination, then (say) 30+ seconds in it actually is so marked?
There was a problem hiding this comment.
These tests are just copied over since it's a refactor, but I'll look into mocking time for these tests.
ellistarn
force-pushed
the
nodecontroller
branch
from
ad7f772 to
641dad0
Compare
ellistarn
changed the title
ellistarn
force-pushed
the
nodecontroller
branch
5 times, most recently
from
1eccfd9 to
bfb2e60
Compare
| }{ | ||
| c.readiness, | ||
| c.liveness, | ||
| c.expiration, | ||
| c.emptiness, |
There was a problem hiding this comment.
If a node doesn't have a finalizer but will be expired for emptiness/expiration, we could choose to delete this node before we add the finalizer since the expiration and emptiness controllers send a delete request before we patch the node.
In addition, since we're running these in parallel, is it possible that the order in which we execute these controllers change?
There was a problem hiding this comment.
Semantically, I want the order of these to not matter. I know that in theory, there are some edge cases (i.e, node comes online, instantly ready, we can't reconcile, and emptyttl is 0), but I think they're contrived enough that I'd much rather define an invariant where the order of these reconcilers doesn't matter.
There was a problem hiding this comment.
I agree that these cases probably won't happen naturally. I also think our logging is in a good enough state that we would be able to debug these cases if they do happen naturally. From then, I'd love to reconsider this approach if necessary.
|
|
||
| const LivenessTimeout = 5 * time.Minute | ||
|
|
||
| // Liveness is a subreconciler that deletes nodes if its determined to be unrecoverable |
There was a problem hiding this comment.
I think this comment might be a little misleading. From reading this, I would assume that a node that has joined, but then has become in an unrecoverable state would then be subject to the workings of this controller.
There was a problem hiding this comment.
I think in theory, that's what this controller should do. I think we do want to implement something like "auto repair" in here.
There was a problem hiding this comment.
I agree. Yet, while we don't have that functionality in place, I think the keeping the comment in line with the actual state of the code makes more sense to me, since we don't know if we want to or will implement "auto-repair"
There was a problem hiding this comment.
That sounds good to me. I'd like to keep the comments roughly the same irrespective of the content.
ellistarn
force-pushed
the
nodecontroller
branch
from
bfb2e60 to
10ae4ef
Compare
ellistarn
force-pushed
the
nodecontroller
branch
from
6a86f63 to
d5336ad
Compare
pkg/controllers/node/emptiness.go
Outdated
| if !empty { | ||
| if _, ok := n.Annotations[v1alpha3.EmptinessTimestampAnnotationKey]; ok { | ||
| delete(n.Annotations, v1alpha3.EmptinessTimestampAnnotationKey) | ||
| logging.FromContext(ctx).Infof("Removed emptiness TTL from node %s", n.Name) | ||
| } | ||
| return reconcile.Result{}, nil | ||
| } | ||
| // 3. Set TTL if not set | ||
| n.Annotations = functional.UnionStringMaps(n.Annotations) | ||
| ttl := time.Duration(ptr.Int64Value(provisioner.Spec.TTLSecondsAfterEmpty)) * time.Second | ||
| emptinessTimestamp, ok := n.Annotations[v1alpha3.EmptinessTimestampAnnotationKey] |
There was a problem hiding this comment.
WDYT?
| if !empty { | |
| if _, ok := n.Annotations[v1alpha3.EmptinessTimestampAnnotationKey]; ok { | |
| delete(n.Annotations, v1alpha3.EmptinessTimestampAnnotationKey) | |
| logging.FromContext(ctx).Infof("Removed emptiness TTL from node %s", n.Name) | |
| } | |
| return reconcile.Result{}, nil | |
| } | |
| // 3. Set TTL if not set | |
| n.Annotations = functional.UnionStringMaps(n.Annotations) | |
| ttl := time.Duration(ptr.Int64Value(provisioner.Spec.TTLSecondsAfterEmpty)) * time.Second | |
| emptinessTimestamp, ok := n.Annotations[v1alpha3.EmptinessTimestampAnnotationKey] | |
| emptinessTimestamp, ok := n.Annotations[v1alpha3.EmptinessTimestampAnnotationKey] | |
| if !empty { | |
| if ok { | |
| delete(n.Annotations, v1alpha3.EmptinessTimestampAnnotationKey) | |
| logging.FromContext(ctx).Infof("Removed emptiness TTL from node %s", n.Name) | |
| } | |
| return reconcile.Result{}, nil | |
| } | |
| // 3. Set TTL if not set | |
| n.Annotations = functional.UnionStringMaps(n.Annotations) | |
| ttl := time.Duration(ptr.Int64Value(provisioner.Spec.TTLSecondsAfterEmpty)) * time.Second |
Issue, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.