Fix/refactormgrs #154
Fix/refactormgrs #154
Conversation
|
Now verified that this does indeed work with both elf and v8 binaries |
yeah the whole point of me doing small refactoring PRs was to try and get incremental stuff done. Machine Manager in its current form is going away, in favor a /shrug |
| // } | ||
|
|
||
| func (m *MachineManager) agentLog(agentId string, entry agentapi.LogEntry) { | ||
| vm, ok := m.allVMs[agentId] |
There was a problem hiding this comment.
The map of VMs should still be indexed on vm id. Did we just wholesale rename vm id to agent id?
Instead, we should probably index these based on workload id. Which should come from the WorkloadManager eventually...
| MachineId: vmID, | ||
| Text: entry.Text, | ||
| Level: slog.Level(entry.Level), | ||
| MachineId: agentId, |
There was a problem hiding this comment.
MachineId = agentId? My brain hurts.
| import agentapi "github.com/synadia-io/nex/internal/agent-api" | ||
|
|
||
| // TODO: this will be utilized in a forthcoming PR | ||
| type ProcessManager interface { |
There was a problem hiding this comment.
Is this meant to be what I was calling WorkloadManager?
There was a problem hiding this comment.
I strongly suggest we call this WorkloadManager -- it does indeed seem to be that. I like this interface very much.
| @@ -384,32 +384,6 @@ var _ = Describe("nex node", func() { | |||
| Expect(managerProxy.Telemetry()).To(Equal(nodeProxy.Telemetry())) | |||
| }) | |||
|
|
|||
| Describe("agent internal API subscriptions", func() { | |||
There was a problem hiding this comment.
These should all still be valid tests.
|
|
||
| _, handshakeOk = m.handshakes[vmid] | ||
| time.Sleep(time.Millisecond * agentapi.DefaultRunloopSleepTimeoutMillis) | ||
| func (m *MachineManager) agentHandshakeTimedOut(agentId string) { |
There was a problem hiding this comment.
Does this get called anywhere else other than awaitHandshake? If not, it makes it harder to read as a separate function.
What will the use-cases be to allow AgentClient instances to support this provided timeout callback?
I actually think Edit: that isn't to say that the branch isn't working after these changes 🙏🏻 |
|
That's pretty nearly what I plan. Review still says you're waiting for changes |
When testing this locally, the v8 runner is rejecting the echofunction.js example with a reason of
[]. I don't know why this is happening and don't know if it's related to this PR or something else.Elf binaries appear to work just fine.