extracts process management into separate interface

[autodidaddict]

everything should continue to work as it did before. Now, however, the firecracker process pool is in its own struct that conforms to the ProcessManager interface.

Note that the workload manager proxy will fail tests because that proxy relied on magical knowledge of struct internals to do its job. We should find a different way to test these things so that we're only testing against the public interface and not using what should be hidden knowledge.

[autodidaddict]

There is a 1:1 relation between workloads and agents, regardless of sandbox.

…

On Mon, Apr 1, 2024, 12:30 PM kt ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In internal/agent-api/client.go <#155 (comment)>: > func (a *AgentClient) Id() string { return a.agentId } func (a *AgentClient) Start(agentId string) error { + a.log.Info("Agent client starting", slog.String("workloadId", agentId)) This kind of naming is harmful to baby seals and other endangered species. A workload is not an agent, so a workload id should not be set as an agent id or referred to as one. Iterative, I know, and similar to feedback I provided on #154 <#154>. ------------------------------ In internal/agent-api/client.go <#155 (comment)>: > @@ -80,6 +91,31 @@ func (a *AgentClient) Start(agentId string) error { return nil } +func (a *AgentClient) RunTrigger(ctx context.Context, tracer trace.Tracer, subject string, data []byte) (*nats.Msg, error) { I do like AgentClient 👍🏻 ------------------------------ In internal/agent-api/types.go <#155 (comment)>: > @@ -127,7 +127,7 @@ func (r *DeployRequest) Validate() bool { err = errors.Join(err, errors.New("at least one trigger subject is required for this workload type")) } - return err == nil + return err This is more idiomatic to return an error 👍🏻 ------------------------------ In internal/control-api/types.go <#155 (comment)>: > @@ -52,10 +52,11 @@ type InfoResponse struct { } type MachineSummary struct { - Id string `json:"id"` - Healthy bool `json:"healthy"` - Uptime string `json:"uptime"` - Workload WorkloadSummary `json:"workload,omitempty"` + Id string `json:"id"` + Healthy bool `json:"healthy"` + Uptime string `json:"uptime"` Why is uptime a string? ------------------------------ In internal/node/controlapi.go <#155 (comment)>: > @@ -107,41 +107,41 @@ func (api *ApiListener) handleStop(m *nats.Msg) { return } - vm := api.mgr.LookupMachine(request.WorkloadId) - if vm == nil { - api.log.Error("Stop request: no such workload", slog.String("vmid", request.WorkloadId)) + deployRequest, _ := api.mgr.LookupWorkload(request.WorkloadId) 👍🏻 ------------------------------ In internal/node/nodeproxy.go <#155 (comment)>: > @@ -73,11 +73,14 @@ func (m *MachineManagerProxy) Telemetry() *Telemetry { } func (m *MachineManagerProxy) VMs() map[string]*runningFirecracker { - return m.m.allVMs I think we should keep this as much as possible -- even if that means refactoring it for now to ensure tests are not deleted but can instead pass. The tests are all still valid even though the underlying implementation and shape of the structs has changed. ------------------------------ In internal/node/process_mgr.go <#155 (comment)>: > } +// A process subscriber is any struct that wishes too be notified when a process manager has +// finished starting a process +type ProcessSubscriber interface { + // Indicates that the process is now ready, and can be "prepared" for deployment + OnProcessReady(workloadId string) What exactly does it mean for a process to be "ready"? ------------------------------ In internal/node/running_vm.go <#155 (comment)>: > @@ -39,10 +39,6 @@ type runningFirecracker struct { workloadStarted time.Time } -func (vm *runningFirecracker) isEssential() bool { Did this go somewhere else? It should have. — Reply to this email directly, view it on GitHub <#155 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADI4TIN5RMR7Q5SK6VD6EIDY3GDTVAVCNFSM6AAAAABFONBEG2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNRZGM2DENRXGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[autodidaddict]

please address the change requests (I can't request changes on my own PR)

[autodidaddict]

Waiting for the results of manual testing on this before merging.