Aquarium and pH #1120
Aquarium and pH #1120
Conversation
|
@philipcmonk, how long do some of these basic tests task? It'd be nice to wire something up for CI, as an example for future testing efforts if nothing else. |
|
Haven't timed it, but something like 5-10 minutes on my machine. It's significantly faster if you don't start collections, acme, dns, and hall. If it's easy to wire up to CI, I'd say we should do it. CI is changing in cc-release, so we also could just hook it up to CI there. |
| :_ this(subscribed =(command %subscribe)) | ||
| (aqua-vane-control-handler our ost subscribed command) | ||
| :: | ||
| ++ diff-aqua-effects |
There was a problem hiding this comment.
A little comment explaining that this ignores effects other than %restore or %send might be helpful.
| ^+ ..abet-pe | ||
| =. this | ||
| %- emit-aqua-events | ||
| [%event who [//behn/0v1n.2m9vh %born ~]]~ |
There was a problem hiding this comment.
Does this need to be the same /0v1n.2m9vh as in the ames handler? Maybe that constant should be split off into sur/aquarium/hoon.
There was a problem hiding this comment.
That value is u3A->sen, the "instance number" in vere. It's set to (scot %uv (mug now)) on boot/restart. I think the idea is to ensure there's a new duct corresponding to the new process, but it's not used for effect routing (or for injecting %wake events). I don't think it's needed here. It might not be needed at all, but that's a larger conversation.
There was a problem hiding this comment.
I just cargo-culted that. The right answer is probably for aqua to keep track of that, change it on restart, and insert it into each of these wire as necessary. For now, I've just copied the same string everywhere.
| pil=pill | ||
| assembled=* | ||
| tym=@da | ||
| fleet-snaps=(map term (map ship pier)) |
There was a problem hiding this comment.
Do :fleet-snaps and :piers refer to the same ships? If so, it might be cleaner to have :fleet-snaps be just a (map term (set ship)) so there's less risk of the two getting out of sync.
There was a problem hiding this comment.
fleet-snaps is a set of snapshots of the state of the ships. It's intended to freeze a fleet so you can restore back to that point later on. This is how pH can cache e.g. "start ~bud" so that you only have to do it once rather than once per test.
sur/aquarium.hoon
Outdated
| @@ -0,0 +1,96 @@ | |||
| :: | |||
| :: Traditionally, ovo refers an event or card, and ova refers to a list | |||
There was a problem hiding this comment.
You're both wrong: ovo is the conventional name for an +ovum, which is (pair wire card). An "event" is (pair @da ovum).
| :: ~? !=(20 suy) [%ap-fill-add [[our dap] q.q.pry ost] +(suy)] | ||
| [%& +(qel.ged (~(put by qel.ged) ost +(suy)))] | ||
| =/ subscriber-ship p:(~(got by sup.ged) ost) | ||
| ?: &(=(20 suy) !=(our subscriber-ship)) |
There was a problem hiding this comment.
Huh, looks like you turned off the culling if our own ship is the one subscribed. Does that still work for web stuff?
There was a problem hiding this comment.
I tried a bunch of stuff to reduce the number of subscription updates in the queues between aqua/ph and aqua-*, and I couldn't come up with anything that would work reliably.
I'm not sure how web stuff uses this? Is that the only way we clean up subscriptions created by web pages?
|
@philipcmonk, i think it's worth adding to CI. It's not particularly complicated to add now, and then re-add once arvo/next converges with vere/cc-release. Duplicating the |
|
Added it to CI. Now CI takes ~25 minutes. Looks like the old builds took between 8 and 17 minutes (not sure what causes them to be longer; maybe changes to /sys?). |
|
Changes to sys/ definitely *should* take longer, as they build a new pill;
are there many that are not close to 8 or close to 17 minutes?
…On Fri, Mar 29, 2019 at 4:59 PM Philip Monk ***@***.***> wrote:
Added it to CI. Now CI takes ~25 minutes. Looks like the old builds took
between 8 and 17 minutes (not sure what causes them to be longer; maybe
changes to /sys?).
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1120 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABxXhqZsUifobA8vJ5fGvKYRqv_X7f_Iks5vbqjggaJpZM4cEoU2>
.
|
|
Also, would it be possible to add custom folds for individual test phases showing more granular timings? Even 8 minutes is rather a lot. (5 of those is the actual test script, 8 in the pill case; it looks like we're maybe building the pill twice??) |
|
@ohAitch, we have to build a second pill to correctly capture userspace changes (which are now stored in the pill). I think the right approach is to a) split the different test types across different CI jobs and b) cache and promote build artifacts more efficiently. The right place will be on top of cc-release, which now uses nix and make as the build system, and urb.py as the test runner. The right time will be when somebody is free to work on it. |
|
Hm, I don't see it being built at all in the 8 minute cases though, is this
a master vs cc-release difference? (And even if you have to add userspace,
recompiling everything from scratch seems superfluous given you could
instead just mutate the "userspace files" event at the end of the pill)
Agree that the Travis build matrix seems useful here, if most of the test
time is in fact straightforwardly parallelizable.
…On Friday, 29 March 2019, Joe Bryan ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1120 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABxXho-G1P51CtKs0xv6Dfxnh-E_jwLAks5vbuccgaJpZM4cEoU2>
.
|
|
It's still only built on changes to sys, but when it's built it's build twice twice: first to stage the kernel changes, second to make the complete up-to-date pill that will be uploaded. This is of course stupid and inefficient, but was the easiest way to make the new pills not wrong. My grand plan is to let the pain of these slow builds increase until someone (maybe me!) snaps and makes it fast. |
|
In that case it is certainly correct to merge the 25-minute version B)
…On Friday, 29 March 2019, Joe Bryan ***@***.***> wrote:
It's still only built on changes to sys, but when it's built it's build
twice twice: first to stage the kernel changes, second to make the complete
up-to-date pill that will be uploaded. This is of course stupid and
inefficient, but was the easiest way to make the new pills not wrong. My
grand plan is to let the pain of these slow builds increase until someone
(maybe me!) snaps and makes it fast.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1120 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABxXhoXFduH_mLPkEjTSQwJ9zp9qnLORks5vbupugaJpZM4cEoU2>
.
|
A lot happening here. I've sent out a usage document, which should probably become public documentation at some point.
Main changes: