Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WorkerManager is being reported to be created **after** the agent is already started and reported its status #399

Closed
CMCDragonkai opened this issue Jul 4, 2022 · 3 comments · Fixed by #446
Assignees
Labels
bug Something isn't working r&d:polykey:supporting activity Supporting core activity

Comments

@CMCDragonkai
Copy link
Member

Describe the bug

In the deployed testnet, I can see that WorkerManager: Created WorkerManager is logged after the STDOUT of the agent. I'm not sure if this is a bug or if its just due to cloud watch reporting the 2 messages at the same time.

2022-07-04T07:13:37.048000+00:00 ecs/polykey-testnet/e6c450d3cf2443f785eaa0d98d122671 INFO:WorkerManager:Creating WorkerManager
2022-07-04T07:13:37.137000+00:00 ecs/polykey-testnet/e6c450d3cf2443f785eaa0d98d122671 {"pid":1,"nodeId":"v7osqo4g9uca06qb48ra7pbpergp59qtdh9584a0j0a4ud80qgaig","clientHost":"0.0.0.0","clientPort":1315,"agentHost":"127.0.0.1","agentPort":36631,"proxyHost":"0.0.0.0","proxyPort":1314,"forwardHost":"127.0.0.1","forwardPort":42899,"recoveryCode":"adjust broom select dish major portion arm horn kit thrive onion coil margin brother arctic skin equip soup mango club thought like guard sister"}
2022-07-04T07:13:37.137000+00:00 ecs/polykey-testnet/e6c450d3cf2443f785eaa0d98d122671 INFO:WorkerManager:Created WorkerManager

Do note that the 2 message timestamps are exactly the same, even the milliseconds 2022-07-04T07:13:37.137000+00:00. It could just due to the fact that the task is set to mode: non-blocking logging which causes the message to arrive out of order.

Part of the problem is that STDOUT and STDERR are interleaved in cloudwatch, so maybe that's the issue too.

Check the code too as we should definitely wait for worker manager to be fully created before we print out the agent status.

To Reproduce

  1. Run pk agent start
  2. Observe WorkerManager: Created WorkerManager

Expected behavior

  1. Message should arrive and be printed out before the final STDOUT.

Additional context

Notify maintainers

@CMCDragonkai

@CMCDragonkai CMCDragonkai added the bug Something isn't working label Jul 4, 2022
@tegefaulkes
Copy link
Contributor

tegefaulkes commented Jul 8, 2022

Reviewing /src/bin/agent/CommandStart.ts I can see that things happen in this order;

  1. createPolykeyAgent
  2. createWorkerManager
  3. pkAgent.setWorkerManager
  4. process.stdout.write information.

This is the expected order. Setting the workerManager is synchronous and creating it is properly awaited. So I don't see any async weirdness happening here.

So like you said, it's likely cloudwatch just giving is the order slightly wrong due to the outputs being on stdout and stderr at exactly the same time. so I don't think it's an issue.

One thing stands out though. The worker manager is only getting started after createPolykeyAgent has finished. dont we want to provide the workerManager to the agent so that the KeyManager can use it to generate the root keypair? We would need to add workerManager as a parameter to createPolykeyAgent so that it can be set between creating and starting the agent.

tegefaulkes added a commit that referenced this issue Jul 8, 2022
This allows the relevant domains to access the worker pool for startup operations. Notably `KeyManager` uses this to do key generation.

Related #399
tegefaulkes added a commit that referenced this issue Jul 12, 2022
This allows the relevant domains to access the worker pool for startup operations. Notably `KeyManager` uses this to do key generation.

Related #399
@CMCDragonkai
Copy link
Member Author

Ok so the WorkerManager is designed to be set and unset dynamically which is done for the other domains.

There is a usecase to have it available during createPolykeyAgent.

So I want to examine this later, as it is not a high priority.

Currently the node-forge doesn't actually use the WorkerManager to generating the root keys anyway. So this would only affect log order.

@CMCDragonkai
Copy link
Member Author

This is already trivially fixed by #446, because the KeyManager has been replaced with KeyRing and CertManager which both can take the worker manager at creation time to help with bootstrapping. However it can also be set dynamically afterwards too. During testing, it's also fast enough to not bother with a worker manager at all, so it's still optional.

@CMCDragonkai CMCDragonkai added the r&d:polykey:supporting activity Supporting core activity label Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working r&d:polykey:supporting activity Supporting core activity
3 participants