tiltrun: use apiserver data model in exit controller #4367
Conversation
internal/engine/exit/controller.go
Outdated
| c.tiltRun.Spec.ExitCondition = tiltruns.ExitConditionFirstFailure | ||
| } | ||
|
|
||
| if err := c.client.Create(ctx, c.tiltRun); err != nil { |
There was a problem hiding this comment.
i've been wondering if a good rule of thumb is that controllers should be idempotent (so that Create() on a resource that already exists should be OK)
There was a problem hiding this comment.
I agree (and reconciler pattern helps with that a lot, since you can basically just requeue on a lot of things and take care of it the next pass).
In this case, it'd mean extra logic to handle things (e.g. fetch if already created, similar-ish logic on any conflict on update failure) - unless you feel strongly, I think that since this is the only think writing to it at the moment, it'd be better to just let it always assume these operations shouldn't fail.
(The Create is actually a really weird one - I added a TODO because I think it should get created explicitly as part of Tiltfile/configs reloading rather than implicitly here, and then this can behave much closer to a real reconciler and just always read from cached client, at which point it could trivially handle optimistic concurrency failures during update.)
milas
force-pushed
the
milas/exitcontroller-apiserver
branch
from
363adcb
to
3e0dcf8
Compare
This is the first increment towards moving towards a reconciler; the controller uses the API server `TiltRun` data model internally now and updates it, so it can be accessed externally: ``` KUBECONFIG=$(tilt alpha kubeconfig-path) kubectl describe tiltrun Tiltfile ```
milas
force-pushed
the
milas/exitcontroller-apiserver
branch
from
3e0dcf8
to
7ec391c
Compare
|
@maiamcc This should be good for review -- I made various changes based on Nick's initial review and also changes to data model based on our conversation. |
|
rad, looking now! |
There was a problem hiding this comment.
...i apologize for the butt-ton of comments, the vast majority are style nits/requests for comments/naming suggestions that you are very free to ignore! the only thing I'm worried about that might behave badly is the "default to pending" behavior of k8sRuntimeTarget
happy for you to go ahead and merge once you've implemented/rejected all the suggestions, or happy to re-review, either way
fixes tilt-dev#6272 History of this change: - we used to have a very naive definition of "Ready" - callers of the runtime status API had hacks to look at Job pods to determine completeness (tilt-dev#4367) - later, we changed it to have a more sophisticated definition, so that Jobs only count as 'ready' when they're complete. (tilt-dev#5013) - we forgot to remove the old caller hacks :grimace: in any case, i added a bunch of tests, since we need more job integration tests anyway. Signed-off-by: Nick Santos <nick.santos@docker.com>
fixes tilt-dev#6272 History of this change: - we used to have a very naive definition of "Ready" - callers of the runtime status API had hacks to look at Job pods to determine completeness (tilt-dev#4367) - later, we changed it to have a more sophisticated definition, so that Jobs only count as 'ready' when they're complete. (tilt-dev#5013) - we forgot to remove the old caller hacks :grimace: in any case, i added a bunch of tests, since we need more job integration tests anyway. Signed-off-by: Nick Santos <nick.santos@docker.com>
fixes tilt-dev#6272 History of this change: - we used to have a very naive definition of "Ready" - callers of the runtime status API had hacks to look at Job pods to determine completeness (tilt-dev#4367) - later, we changed it to have a more sophisticated definition, so that Jobs only count as 'ready' when they're complete. (tilt-dev#5013) - we forgot to remove the old caller hacks :grimace: in any case, i added a bunch of tests, since we need more job integration tests anyway. Signed-off-by: Nick Santos <nick.santos@docker.com>
) * Handle jobs with deleted pods Signed-off-by: Daniel Nephin <dnephin@gmail.com> * session: ensure 'tilt ci' exits properly when reattaching to jobs fixes #6272 History of this change: - we used to have a very naive definition of "Ready" - callers of the runtime status API had hacks to look at Job pods to determine completeness (#4367) - later, we changed it to have a more sophisticated definition, so that Jobs only count as 'ready' when they're complete. (#5013) - we forgot to remove the old caller hacks :grimace: in any case, i added a bunch of tests, since we need more job integration tests anyway. Signed-off-by: Nick Santos <nick.santos@docker.com> --------- Signed-off-by: Daniel Nephin <dnephin@gmail.com> Signed-off-by: Nick Santos <nick.santos@docker.com> Co-authored-by: Daniel Nephin <dnephin@gmail.com>
This is the first increment towards moving towards a reconciler;
the controller uses the API server
TiltRundata model internallynow and updates it, so it can be accessed externally:
KUBECONFIG=$(tilt alpha kubeconfig-path) kubectl describe tiltrun Tiltfile