-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Add kernel bootstrap file config #373
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
Conversation
5fe77dc to
4906947
Compare
d80cba9 to
2849d89
Compare
|
Wow. Very impressive! One high order bit before I start reading in detail: there's a missing layer of indirection. Adding it will likely entail some additional UX drudgery but hopefully no fundamental challenges (well, no fundamental challenges for you; I'd probably thrash for days 😃 ): the mechanism implemented in this PR is managing "the" configuration, but in principle there is no "the" in the way the UX implemented here conceptualizes it. Prior to this PR there was a default configuration that was put in for testing purposes, but it was present just so there'll be something there when the extension is loaded so you can tell if it's working and go play around with stuff. However, note that the thing was labelled the "default sub-cluster", where we've defined a sub-cluster as a group of related vats with a common configuration and a designated bootstrap vat. An installation of the ocap kernel needs to be able to have any number of sub-clusters defined/installed (and the version we ship might not start with any at all). So the top level of configuration control would be a list of these sub-clusters, which we'd then like to be individually editable via a mechanism like the one implemented here. As a practical matter, for the time being we can deal with just having the single sub-cluster as an interim stage for testing and whatnot (essentially that's what we have prior to this PR anyway), but probably care should be taken to ensure we're not painting ourselves into a corner in a way that makes adding control for multiple sub-clusters unduly difficult later. One way to avoid such difficulty might be to just implement a multiple-sub-cluster UX right now, but I'd defer to your judgement on that. (Probably we'll also want controls for introducing a vat in one sub-cluster to a vat in a different one, but that's a much more advanced thing that can be addressed entirely separately later once we develop a clearer picture of what we actually want.) |
Ok. I think this deserves a new PR though so I added a task and can do it right after |
grypez
left a comment
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.
Looks nice! I defer to @FUDCo's wisdom on the bootstrapping indirection. About the testing setup: I think we will be very happy to have the new @ocap/test-utils kernel mock, and I have a small suggestion to scope it a little tighter. Color me paranoid.
packages/extension/src/kernel-integration/handle-panel-message.test.ts
Outdated
Show resolved
Hide resolved
packages/extension/src/kernel-integration/handle-panel-message.test.ts
Outdated
Show resolved
Hide resolved
FUDCo
left a comment
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 only skimmed the UI stuff, which I don't really feel qualified to have an opinion on (though it looks very nice), so I'll leave to somebody else to pull the trigger on final approval.
The kernel stuff looks fine, modulo the one comment I had about the granularity of control over storage reset; I'd like to see that changed, but wouldn't hold up the train based on that as it is consistent with the status quo.
packages/kernel/src/Kernel.ts
Outdated
| if (config.forceReset) { | ||
| await this.clearStorage(); | ||
| } |
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.
Making the storage reset conditional is a really good idea, but it probably shouldn't be part of the cluster config per se, since the reset will impact all running clusters, not just the one being reconfigured. My intuition is that the reset should be an independent parameter (eventually we should be able to reset only those portions of the data store that pertain to a specific subset of all the vats that are running, but that's probably a slightly hairy task best approached independently).
Probably also worth making it a parameter of the Kernel constructor call as well (or possibly the init method), which currently clears storage unconditionally with a big comment saying "don't keep doing this".
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've created a follow up issue for Implementing Selective Vat Storage Reset and made clearing the storage on init optional
grypez
left a comment
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.
LGTM!
f35a6c9 to
378745d
Compare
Closes #204