Draft: vertical scaling proof of concept #416
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/p2p/Self.ts
Outdated
| try { | ||
| // eslint-disable-next-line security/detect-non-literal-fs-filename | ||
| await fs.writeFile(filePath, resp.id) | ||
| process.exit(1) // exiting with status 1 causes our modified PM2 to not restart the process |
There was a problem hiding this comment.
This should be ok... but we will have to test to make sure the docker will not restart us.
If we have any issues with stuff restarting even when we use exit(1) the fix would be to add a 'config' that will let us select a sleep instead of an exit. Basically we would sleep the process for say 10 minutes because we know the cord will get pulled soon anyhow.
Actually the best thing may be to have a setting that lets us just choose between exit(1) and sleep 10min
this way once testing starts we have the ability to pivot if we need to.
There was a problem hiding this comment.
@PudgyPug I believe the code sleeps for 10 seconds, while the suggestion was to sleep for 10 minutes
While 10 min is a long time, 10 seconds might be to few for the migration to a new VPS to happen, there's a lot of infra config going on in the background so I can't guarantee there's an external kill signal inside of 10 sec
Can you add an process.env.VERTICALSCALING_STRATEGY (or _shutdown_mode as you like) to the local config & enable setting it in process.env, it would be nice to be able to use either in our POC without having to recompile
The default I'd like to use for that right now is shutdown over sleep, but as long as the env config can be set this isnt as important
gcp-config.patch
Outdated
| - enabled: false, | ||
| - path: '.' | ||
| + enabled: true, | ||
| + path: '/home/node/config' |
There was a problem hiding this comment.
This is my bad in the task description but I think we can make these be server configs because that is something related to the entire network state and we will sync to match the network which will have these values off.
Also nested server configs can give us some issues.
I think the change needed here is to just make settings in code where these values are off and use env vars to adjust them.
i.e. VERTICALSCALING_ENABLED. etc.
Core is actually lacking the equivalent to shadeum flags (a solution for individual node settings) but for now we can stay low tech.
I think a patch that would also override and set value in these new settings would still be a good idea in case we decide that building a custom docker is better than relying on ENV vars.
so to recap for thes settings we will just use consts for each setting like:
const verticalScalingMode_enabled = false || process.env.VERTICALSCALING_ENABLED
then we also have a patch that will set the above const as well for the compiled option of setting things.
these consts could live in a new file in the config folder called localConfig.ts they can be exported and used where needed. It is ok to group in object/struct if that works out more cleanly.
There was a problem hiding this comment.
added a new localConfig file and addressed the comment. lmk if this is as intended also @chrischabot is a patch file still needed at all now or can we opt for using the VERTICALSCALING_ENABLED and VERTICALSCALING_PATH env variables now?
There was a problem hiding this comment.
@PudgyPug this is perfect, much prefer the env configs. As per comment above please also add one for the shutdown_mode too to make this perfect
| // write a file to FS | ||
| try { | ||
| // eslint-disable-next-line security/detect-non-literal-fs-filename | ||
| await fs.writeFile(filePath, resp.id) |
Check failure
Code scanning / CodeQL
Potential file system race condition
…progress on the GCP project for multiple days. This is a temporary workaround so it can be looked at without blocking
No description provided.