chore: k8s pod & node informer actor refactor [DET-9597]

[carolinaecalderon]

Description

As part of the actor refactor project, remove all references to the actor system from the pod & node informers for Kubernetes. To test these changes, informer_intg_test.go is added, in addition to generated mock files.

Test Plan

CircleCI workflows: OSS, EE

Set-up: Follow instructions here to run Determined backed by Kubernetes, but outside of Kubernetes. Start your Kubernetes cluster & start the Determined devcluster.

Case 1a: Suppose an experiment starts/completes successfully, so the pod informers report “Pending”/”Running” until the resources are deleted. The devcluster logs should match the content below:

# To start:
DEBU[...] pod informer is starting                      Informer=default
time="..." level=info msg="created pod <exp-pod-name>" ...
DEBU[...] informer got new pod event for pod: <exp-pod-name> Pending Informer=default
DEBU[...] informer got new pod event for pod: <exp-pod-name> Running Informer=default

# Upon canceling from Determined WebUI:
DEBU[2023-07-10T14:33:42-04:00] informer got new pod event for pod: <exp-pod-name> Failed Informer=default

# Upon successful completion:
# If the informer receives any updates AFTER the experiment is completed, it will receive a warning.
DEBU[...] informer got new pod event for pod: <exp-pod-name> Running  Informer=default
INFO[...] transitioning pod state from RUNNING to TERMINATED  ...
INFO[... pod exited successfully                       ...
INFO[...] requesting to delete kubernetes resources     ...
DEBU[...] resources state changed: ... ResourcesStopped:resources exited successfully  ...
time="..." level=info msg="deleted pod <exp-pod-name>" ...
INFO[...] de-registering pod handler                    ... pod=<exp-pod-name>
DEBU[...] informer got new pod event for pod: <exp-pod-name> Running  Informer=default
WARN[...] received pod status update for un-registered pod  ... pod-name=<exp-pod-name>

Case 2a: Suppose there is a long-running experiment where its pod is killed manually (on the Kubernetes side), so the pod informer fails. The devcluster logs should match the content below, this tells us that Determined “heard” about the pod failure & the informer cannot accept more events.

INFO[...] requesting to delete kubernetes resources ...
DEBU[...] resources state changed: ... ResourcesStopped:resources failed with non-zero exit code: ... 
INFO[...] resources are released for <pod-name> ...
DEBU[...] informer got new pod event for pod: <exp-pod-name> Failed Informer=default
INFO[...] experiment shut down successfully ...

Case 1b: Suppose an experiment starts/completes successfully, so the node informers report “Pending”/”Running” until the resources are deleted. TODO
Upon start:

DEBU[2023-07-13T13:44:22-04:00] informer added node: <node-name>                 component=nodeInformer
DEBU[2023-07-13T13:44:22-04:00] node informer is starting             ...
DEBU[2023-07-13T15:15:45-04:00] informer got new node event(MODIFIED) for node: minikube   component=nodeInformer

Checklist

• Changes have been manually QA'd
• User-facing API changes need the "User-facing API Change" label.
• Release notes should be added as a separate file under docs/release-notes/.
  See Release Note for details.
• Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

DET-9597

[stoksc]

this looks mostly good but there is a flaw that runs throughout, the informer should be long lived and send a stream of events to the pods actor whereas in this code, it is always called synchronously and only returns one event. i think if you change to start the informer in a background routine and fire the event via a callback, all the other required changes should sort of follow from that.

[maxrussell]

Still looking, but submitting for now

[maxrussell]

Looking really good! A request, some suggestions, and a couple nits

[stoksc]

looks great, just a few comments than should be gtg

[stoksc]

lgtm, though probably should get 2 approvals considering the size

[stoksc]

looks good just a few final comments.

[stoksc]

i would return this error and let the caller (pods.go) decide to panic, since panicking unnecessarily in libraries can make them hard to consume correctly (you have to handle an error, you can accidentally not handle a panic). the panic when the retry watcher is slightly different, but mostly because it requires a lot more infra to propagate; if we had more time i would probably say it shouldn't panic, too (and pods should get notified, async, of its error and decide to panic or restart it or something else).

@maxrussell / @erikwilson curious your thoughts here.

[maxrussell]

Totally agreed.

I pretty much only use panic when the system shouldn't try to recover—when I intend to crash the program—so pretty much when I'm sure it's some unrecoverable error. In this case, I don't think this function has enough context to know whether we're in that state, so returning an error so calling code can decide seems like the best option.

[carolinaecalderon]

all good points -- my justification for panicking vs returning an error is that if the startInformer() functions were going to be placed in Initialize, then my bias is that any errors that occur there are higher stakes. Additionally, I don't think I've seen any error handling within any 'initialize' functions from my memory -- although please correct me if I'm wrong & this correlation is not causation.
Perhaps the better solution to preserve the error handling (vs defaulting to panicking) would be to move the start functions out of Initialize -- something that @stoksc has alluded to in later comments

Outdated

[stoksc]

lgtm! great work.