Implement CustomLogPipes for Spin Logging/IO: The Return

[danbugs]

That's exactly right. If you have your WasiFiles already, you can call ModuleIoRedirects::new directly, and pass that to prepare_component. (At least, you'll be able to if that PR is accepted.)

WAGI does this in a very simple form because it needs different behaviour for stdin, and a slightly different interaction with the buffer for stdout. It's really valuable to know that this is more broadly useful as a public customisation point though!

To provide an update on this from #438 (and maybe close that issue soon), it turns out that I couldn't really make the current architecture use our own pipes — As previously implied, the meat and potatoes of logging is done in execute:
https://github.com/fermyon/spin/blob/c3ff7d90fe115daa3fa85ce3e626c738e7baef76/crates/http/src/spin.rs#L19-L59

It's there that capture_io_to_memory, prepare_component (that receives redirects), and, subsequently, redirect_to_mem_buffer are called.

Considering our builder is set w/ with_engine(), as per:
https://github.com/fermyon/spin/blob/1fbfb23b4e96a580d6d4f4b6d617e916974493f1/crates/engine/src/lib.rs#L103-L119

I tried emulating the work done in execute in the build stage similar to how the store does it:
https://github.com/fermyon/spin/blob/0f51ad0b86b798ae18cb4f211f0c3fcfbbe9a156/crates/engine/src/lib.rs#L304-L316

... but, nothing came of it as execute would eventually run and override/never access my RuntimeContext WASI context data.

All and all, this PR allows users to set CustomLogPipes when creating their ExecutionContextConfiguration — these CustomLogPipes are later accessed by execute and passed in to capture_io_to_memory (which has a side-effect on prepare_component) providing a good and needed override of Spin's default settings.

EDIT: For ease of review, I remade this PR ~ my rebases to fix sign-off issues made the previous one (i.e., #471) kind of messy

[itowlson]

Yes, our previous discussion involved an assumption that you'd be in a position to call the preparation methods directly. If you want to simply host the trigger, then indeed those are not available to you. But I agree, you should be able to host the trigger/executor and decide how to handle the guest component input/output.

The solution in this PR exposes an architectural flaw (or, more kindly, limitation) in my original 'follow' design, and it would be good to correct that now rather than continue down that same path. My error was putting too much policy in the executor, and too much implementation in the engine. I think we should flip that around and take policy as far as possible out of the executor, and place it into the executor host (spin up by default, but a custom host in your case). Implementation could remain in the engine, or go elsewhere - current implementations are very bound to specific policies.

What this might look like will take some more thought, but roughly I'm thinking:

• Engine: defines the abstractions it needs (may be as little as ModuleIoRedirects) for wiring up to the module at execution time
• Something else: provides helpers for constructing ModuleIoRedirects around streams, pipes, files, memory and whatever, and for composing those
• Host: policy: looks at the command line and manifest, and sets up appropriate factories for ModuleIoRedirects using the helpers or its own custom implementation
• Trigger/executor: uses the given factory to instantiate ModuleIoRedirects on a per-request basis (overriding where appropriate e.g. WAGI's stdout handling), calls a flush method afterwards so buffered output can be written to file. It makes no policy choices of its own!

As part of this we could consider whether guest output should be written to log files as it is emitted. The buffering of logs into memory before dumping en bloc to a file may have been influenced by WAGI; I don't know if it's still desirable for it to be that way. (There may be reasons like all the logs for a request being together though.)

These are very vague thoughts, and represent a significant change without a crisp definition. The sketch above moves significant preparation out of the executor, and we may want to review other parts of the preparation pipeline alongside it. What you have here looks fine from a first skim over, and if you just want to get something working then fair enough!

[itowlson]

To be clear, I really appreciate what you've done here! What prompted the above was reflecting on the architecture you are working within, not on any disagreement with what you're doing or how you're doing it. I'm really happy to see this PR!

[danbugs]

Hey, @itowlson ~ Thank you for your review!

I'm sorry for taking a while to reply, I've actually been working on trying out an implementation following your spec. Anyway, how does this (i.e., below) sound?

In terms main types, we'll have the following:

• a RedirectPipes type (previously ModuleIoRedirects) that contains the stdin, stdout, and stderr pipes,
• a RedirectReadHandles type (same as before), and
• a ModuleIoRedirects type that contains our RedirectPipes and RedirectReadHandles — I think this would be good because the Redirect* types are sort of intertwined.

In terms of functions, we'll add the following:

• a new method to RedirectReadHandles that will perform part of redirect_to_mem_buffer's function of creating the locks.
• a new method to ModuleIoRedirects that creates an instance in its' default way (i.e., calling RedirectReadHandles's new method and then following the steps that redirect_to_mem_buffer follows currently).
• a new_from_files method to ModuleIoRedirects that will create an instance from files/pipes/whatever specified by the user.

Next up, we can create an enum called ModuleIoRedirectsType w/ values Default, and FromFiles(CustomLogPipes). This can be added as a field to the ExecutionContextConfiguration struct (or as a new parameter to the host like follow), so the user can specify if they want to override spin's default host implementation w/ their own files.

With this set, the executor can check the engine.config for the ModuleIoRedirectsType and, in a match, call the appropriate new* method, and have its' results be passed to prepare_component, and save_output_to_logs.

[danbugs]

P.S. I think that Arc::try_unwrap() here is not going to work, so this is sort of broken, but... It should give an idea of how I was thinking the code could be structured under your spec, @itowlson!

~let me know what you think!

EDIT: I might have fixed it.**

[danbugs]

I've fixed the issues that I've brought up w/ this latest commit, @itowlson ~ SOOO... You should be good to go for reviewing the implementation of custom log pipes under your spec (e.g., using ModuleIoRedirect factories, and so on).

[itowlson]

Sorry for the slow cycle on the CI. For some reason we still have to approve you as a 'first time contributor' each time. Hopefully this will go away once it's merged. Aiming to take a look tomorrow (so, uh, sorry about the slow cycle on that too).

[danbugs]

@Mossaka ~ I've addressed all the changes you requested in my latest push

[itowlson]

There's a few things here that worry me - I realise that the existing redirection and follow functionality was a bit messy but this looks like it misses significant parts of how that worked - but I could be completely misunderstanding?

One specific thing that concerns me is that this adds a bunch of places where it looks like the code could easily panic. Most of these should be "can't happen" and should be ruled out by strengthening the types; but if there are places where we need the possibility of failure then we should probably be using Results rather than panics. Happy to discuss though!

Thanks for all your hard work and patience on this!

[danbugs]

@itowlson ~ I've made a new push where I addressed all the items I marked as "done ✅". If those look good to you, feel free to "Resolve conversation" them! There are a couple of items we are still discussing, so I'll be waiting your replies there before taking any action ;)

EDIT: I've marked those items as resolved, and also the one you asked me to!

[itowlson]

This looks good except for a couple of places where I am still uncomfortable about unwrapping values: in PipeFile::clone() and in the calls to engine.save_output_to_logs. If we can figure out a way to address those then we are good to go.

I would also really like to have some sort of test to exercise the CLP path through one of the triggers. This is probably not a showstopper but it is a really strong nice to have!

Thanks for your patience with my vagueness and my nitpicking - this is really close now!

[itowlson]

By the way our CI enforces cargo fmt and cargo clippy gates - given that we are still stuck in manual approval to get CI feedback, I thought I'd better flag you to run those so you're not circling around on build failures from small mechanical stuff!

[itowlson]

LGTM - thanks for your patience on this!

[itowlson]

Ugh, I'm sorry, @danbugs - we need you to sign the commits. (And thanks GitHub for not flagging this until after all the other checks had passed.)

[danbugs]

Ugh, I'm sorry, @danbugs - we need you to sign the commits. (And thanks GitHub for not flagging this until after all the other checks had passed.)

Hmmm~ That's odd, I did sign my commits. The DCO bot even confirms it:

What am I missing, @itowlson?

EDIT: It also shows the signature if you hover over my commits:

[itowlson]

They need to be GPG verified commits as well as the DCO signature. See https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification

[danbugs]

@itowlson ~ thank you for your patience, done ✅

[radu-matei]

Also, given that we merge commits, could you please only keep the useful commits?
Thanks!

[danbugs]

@radu-matei ~ done ✅

P.S. from 23 commits down to 4!