-
Notifications
You must be signed in to change notification settings - Fork 339
Playground CLI: Mount /wordpress, /internal, and /tmp dirs within real temporary dir #2446
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
Playground CLI: Mount /wordpress, /internal, and /tmp dirs within real temporary dir #2446
Conversation
With Planning to look at this more tomorrow. |
Currently, the Blueprints v1 worker's |
Yeah, the errno is |
Yeah we might need to rewire some of the /internal initialization logic for a secondary worker when using Blueprints v1 to support this. |
This is a functional experiment now, and Playground CLI multi-worker boot feels nearly instantaneous on my laptop. It requires rebuilding php-wasm/node, which I plan to do after lunch. NOTE: |
@adamziel, the only issue appears to have been creating the initial dirs under |
Some things that need done:
|
This isn't ready to merge, but it is ready for more feedback. |
This is simple and effective, I like it. Thank you @brandonpayton!
This is a good idea. There will be some cases where that auto-cleanup won't run, e.g. the process gets killed without waiting, there's a power outage etc. I wonder what are some things we could do to maximize our chances of actually cleaning up stale directories. Perhaps we could use a naming pattern including PID and then sweep stale directories on startup? There could be some risk with short-lived processes so maybe a |
Poking around more, we'll need to address all these paths: wordpress-playground/packages/playground/wordpress/src/boot.ts Lines 232 to 236 in 89a7f30
Ideally, we can either get rid of Also, we need to take this bit of the PHP class into account (in
|
Noodling on this more, perhaps a useful API would be something like
In any case, I've instrumented the Blueprints v2 worker code and the workers seem to boot fairly quickly. I don't have any numbers to quote but it feels fast! |
@@ -3914,6 +3912,8 @@ export function init(RuntimeName, PHPLoader) { | |||
node = lookup.node; | |||
|
|||
if (FS.isMountpoint(node)) { | |||
console.log({ mountpoint }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I accidentally reformatted the file. My changes here are all about this and the following two console.log
s.
This comment responds to multiple comments, not necessarily in the order they were made.
Nice! Thanks for adding that. Yeah, that's been my experience as well.
@adamziel, what if we invert this? It seems like we want all workers to respond to requests based on the same state. What FS paths do we want to be private/separate per worker?
Ah, good points! It's obvious we'd want to stop proxying already-shared filesystems (and same for copying from old to new PHP instances during hot swap), but somehow I was forgetting that. |
Oh I like it! Off the top of my head, it's just the input/output devices. It seems like we could share everything else 🤔 So these paths would become worker-specific:
We could move them to /internal/isolated (as opposed to /internal/shared to keep the /internal/shared path working for existing API consumers) and share the rest of the filesystem. |
Sweet. I'm looking at doing this. Using /internal/isolated seems a bit tricky to do with Emscripten mounting because it doesn't look like we can easily mount a MEMFS dir inside a NODEFS dir. At least Google suggests we probably cannot easily do this because it would mean mixing contents of a real NODEFS dir with artificial mounts in that dir. Based on my recent looks into emscripten FS implementations, I found this reasoning compelling, but I haven't tested it. If that is truly an issue, we will likely run into trouble if trying to mount a NODEFS dir as a subdir of our default/temp NODEFS dir. Then again, how does wp-now do this today for automounted subdirs of /wordpress ? Will test and follow up here. |
OK, I guess we can forget that. In testing, it seems totally possible. I can mount a real dir as /wordpress and then mount another as /wordpress/wp-content on top of that. |
Ah, that wasn't mounting a MEMFS dir within a NODEFS dir, but I just tested that by editing a php_8_3.js file and mounting a MEMFS dir inside NODEFS like: if (phpWasmInitOptions?.nativeInternalDirPath) {
FS.mount(
FS.filesystems.NODEFS,
{ root: phpWasmInitOptions.nativeInternalDirPath },
'/internal/shared'
);
FS.mkdir('/internal/shared/something');
FS.mount(
FS.filesystems.MEMFS,
{ root: phpWasmInitOptions.nativeInternalDirPath },
'/internal/shared/something'
);
} And there are no errors during boot. So this should all be doable. 👍 |
Random thing, when I was thinking of adjusting the directory trees to workaround possibly mount challenges, I was thinking something like:
@adamziel how does this sound to you? I kinda like it, at least compared to nesting non-shared things in a shared dir. |
I'm out of time today and plan to continue tomorrow. |
Cool. It looks like we already skip any FS node that is not detected as MEMFS: wordpress-playground/packages/php-wasm/universal/src/lib/php.ts Lines 1457 to 1461 in 533d676
|
Sounds good to me! I want to preserve the directory structure inside the |
This is looking really good! I've left some notes, a few of which are blocking, but nothing major. Thank you for figuring this one out @brandonpayton! |
@adamziel! It's good to have your review. I wanted to self-review and get some of the mess cleaned up before you looked, but it sounds like it mostly wasn't too bad. :) Thanks! I plan to follow up on your comments tomorrow. |
🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! I went over the comments, made changes, and left some questions.
A couple of things remain that we could do here or in a follow-up PR:
- Find a way to avoid Playground CLI instances launching conflicting stale-directory cleanup operations. It shouldn't break anything but could cause some confusing error messages to be printed.
- Add logic to remount under
/internal/symlinks
if the mount target changes from a file to a directory or vice versa.
symlinkMountNode.mount.mountpoint !== | ||
symlinkMountPath | ||
) { | ||
phpRuntime.FS.mount( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This area has been tricky for me to think through, but I think it is OK, as long as user PHP doesn't directly manipulate /internal/symlinks
descendants. Here's why:
/internal/symlinks
contains the paths to symlink targets, not the symlinks themselves. If a PHP script changes a symlink (outside of /internal/symlinks
), all that will happen is that our realpath()
override will create another mount under /internal/symlinks/the/absolute/path/of/new/symlink/target
.
Therefore, if a symlink changes its target, it shouldn't have any effect on /internal/symlinks
.
Does that makes sense?
symlinkMountNode.mount.mountpoint !== | ||
symlinkMountPath | ||
) { | ||
phpRuntime.FS.mount( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that does look like a "gotcha" is if someone switches the type of mount from "dir" to "file" or vice versa. Maybe we can handle this in a follow-up PR as it is an existing limitation. Sound OK?
}, | ||
onPHPInstanceCreated: async (php: PHP) => { | ||
await mountResources(php, args['mount-before-install'] || []); | ||
if (this.blueprintTargetResolved) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit off to have to track the pre-install PHP instances like this, but I like how explicit the current code is. This way, we are able to see all waiting instances in one place. In addition, we can remove a PHP instance from the wait list if it exits before the blueprint target is resolved.
AFAICT, if we awaited a promise for blueprintTargetResolved, dead PHP instances would not be able to be GC'd until the blueprint target was resolved, and we'd have to devise another way to avoid trying to apply post-install mounts to each dead PHP instance.
Does that sound reasonable to you?
|
||
// NOTE: This is an async operation, but we do not care to block on it. | ||
// Let's let the cleanup happen as the main thread has time. | ||
cleanupStalePlaygroundTempDirs( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure we're not running multiple simultaneous cleanups in two processes if the developer runs npx @wp-playground/cli twice in two different terminals.
@adamziel, I didn't get to this today but should be able to look at it tomorrow. Or we could address as a follow-up.
…of workers and php instances
I've patched the last few remaining things I was nervous about. This one seems good to go – let's do it! 🤞 🤞 🤞 |
) { | ||
// The /request directory holds per-request state that is isolated to a | ||
// single PHP instance. Let's not copy it. | ||
if (path && path !== '/request') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamziel, great catch! Thank you! We don't want to be duplicating request state.
@adamziel, thanks for your help! It's great to be getting this in. |
## Motivation for the change, related issues #2446 assumes nested mounts are supported by Playground CLI. That wasn't always the case. This PR adds an explicit regression test to ensure they work and will keep working. cc @brandonpayton
Motivation for the change, related issues
There are two reasons to mount a real directory as
/internal/shared
:/internal/shared
to be truly shared between all PHP instances. Issue:Playground CLI: Make /internal/shared truly shared in a multi-worker environment #2301.
/internal
dir and unzipping for secondary workers is making multi-worker CLI startup quite slow. Issue:Playground CLI: Starting additional workers delays readiness of Playground CLI server #2419.
Once we looked at doing the above, we saw that it would be good for workers to share the same real FS by default. It made it easier for users to start a multi-worker Playground server, and those workers would all naturally share the same underlying FS state.
Implementation details
/internal
/wordpress
/home
- We don't use this AFAIK, but since it is assumed to exist in the emscripten-generated php-wasm JS, it seemed good to make sure it is also backed by a native dir./tmp
/internal
dir is passed to php-wasm init where it handles mounting and creation of internal subdirs./wordpress
,/home
, or/tmp
, we mount the temp subdirs in their place.Testing Instructions (or ideally a Blueprint)