go/runtime: Support hot-loading of runtime bundles #5961
Conversation
✅ Deploy Preview for oasisprotocol-oasis-core canceled.
|
peternose
force-pushed
the
peternose/feature/hot-loading
branch
3 times, most recently
from
9e3cb33 to
5da378e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5961 +/- ##
==========================================
+ Coverage 64.69% 65.33% +0.63%
==========================================
Files 629 631 +2
Lines 64297 64752 +455
==========================================
+ Hits 41599 42304 +705
+ Misses 17775 17485 -290
- Partials 4923 4963 +40 ☔ View full report in Codecov by Sentry. |
peternose
requested review from
kostko,
peterjgilbert,
pro-wh and
ptrus
as code owners
go/runtime/bundle/discovery.go
Outdated
| // Init sets up bundle discovery using node configuration and adds configured | ||
| // and cached bundles to the registry. | ||
| func (d *Discovery) Init() error { | ||
| // Consolidate all bundles in one place, which could be useful | ||
| // if we implement P2P sharing in the future. | ||
| if err := d.copyBundles(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Add copied and cached bundles to the registry. | ||
| if err := d.Discover(); err != nil { | ||
| return err |
There was a problem hiding this comment.
Init only adds configured bundles to the registry. In addition, some of them may be already previously exploded (cached)? If this is the case somewhere nested WriteExploded will only verify...
However, it does not guarantee to add all the "cached" bundles, as looking at Discover method you only add the *.orc bundles to the registry (i.e. the ones that were configured). This is desired as some of the exploded bundles may be stale, i.e. no longer part of the config. (Automatic removal will be handled as part of #5737).
Do I understand your logic correctly? If so I would suggest removing cached bundles reference from the Init toplevel docs and before d.Discover() to avoid confusion?
Thanks for your reply! :)
There was a problem hiding this comment.
All bundles are copied to the bundle folder.:
- Bundles from the config are copied in
Initon line 79. - Bundles fetched from the web are first verified and then copied to the bundle folder.
So the Discovery loads all bundles in the config, all bundles that used to be in the config, and all bundles that were downloaded. The idea is that you always have bundle and the extracted folder in the bundle directory. This way, it will be easy to implement p2p2 sharing as one can just check which orc files are in the bundle directory.
There was a problem hiding this comment.
I see and thanks.
So what I suggest to change (super minor but to avoid confusion):
// Init sets up bundle discovery using node configuration and adds configured
// and cached bundles to the registry.
and
// Add copied and cached bundles to the registry.to
// Init sets up bundle discovery using node configuration and adds configured
// bundles (that are guaranteed to be exploded) to the registry.
and
// Add copied bundles (that are guaranteed to be exploded) to the registry.or smtg along the lines, just to emphasize that we are not also moving stale cached bundles (see issue #5737)
Hope it makes sense(?), and feel free to reword as you see fit...
There was a problem hiding this comment.
all bundles that used to be in the config
Correct since at init time the .orc file (if configured) was moved to /data/runtimes/bundles and exploded.
Note, the nodes that were running previous software (prior to this PR/current version), may have some bundles exploded, later removed them from config (and thus also bundle), so only the exploded part will stay. I guess with #5737 the idea was to remove those stale "exploded" bundles + the ones whose version is now outdated.
Now if we push for the p2p sharing, I guess we go in the other direction, i.e. we further increase the storage load on the nodes as they store essentially all the historical bundles and their exploded paths (technically we could be removing those once outdated and only store bundles). On the positive side we get a more resilient data availability for our software components.
Thinking aloud, if we want to have some kind of p2p sharing we may want only archive nodes responsible for that? For the normal nodes I don't think we want to store outdated bundles and exploded components. In any case we may want to adapt #5737 cleanup requirements?
Happy to tackle this myself in the follow-up once we align on the strategy.
cc @kostko
There was a problem hiding this comment.
There is no sense in storing or sharing historic bundles, so any pruning should get rid of them IMO.
There was a problem hiding this comment.
OK, this make sense. RONL + system components in one bundle, and ROFL components in detached bundles.
Also note that ROFL components can only be fetched from detached bundles, not from bundles that contain RONLs.
Our code doesn't distinguish between ROFL and system components. But it would be nice to have a check that ROFLs are always in a detached bundle, and system components in the runtime bundle.
There was a problem hiding this comment.
Should we add ROFLSystemKind Kind= "rofl-system" or SystemKind Kind = "system" and verify this in the code?
There was a problem hiding this comment.
Ups, maybe we don't need this and we just mark the component as system ROFL once is read from an undetached bundle.
There was a problem hiding this comment.
I have thought "system components" can only be ROFL component with current design? (update expect for one and only one RONL)
But it would be nice to have a check that ROFLs are always in a detached bundle, and system components in the runtime bundle.
If not is this really "always" then, if system component is not rofl and may be used by different bundles, then you also want to package it separately, i.e. detached?
There was a problem hiding this comment.
So "system ROFL component" = non-detached ROFL. There is already the detached flag for this, but maybe it is not obvious? Or there could be an additional component attribute (e.g. system), which would only be allowed to be set if the bundle is non-detached?
go/runtime/bundle/discovery.go
Outdated
| // Init sets up bundle discovery using node configuration and adds configured | ||
| // and cached bundles to the registry. | ||
| func (d *Discovery) Init() error { | ||
| // Consolidate all bundles in one place, which could be useful | ||
| // if we implement P2P sharing in the future. | ||
| if err := d.copyBundles(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Add copied and cached bundles to the registry. | ||
| if err := d.Discover(); err != nil { | ||
| return err |
There was a problem hiding this comment.
I see and thanks.
So what I suggest to change (super minor but to avoid confusion):
// Init sets up bundle discovery using node configuration and adds configured
// and cached bundles to the registry.
and
// Add copied and cached bundles to the registry.to
// Init sets up bundle discovery using node configuration and adds configured
// bundles (that are guaranteed to be exploded) to the registry.
and
// Add copied bundles (that are guaranteed to be exploded) to the registry.or smtg along the lines, just to emphasize that we are not also moving stale cached bundles (see issue #5737)
Hope it makes sense(?), and feel free to reword as you see fit...
go/runtime/bundle/discovery.go
Outdated
| // Init sets up bundle discovery using node configuration and adds configured | ||
| // and cached bundles to the registry. | ||
| func (d *Discovery) Init() error { | ||
| // Consolidate all bundles in one place, which could be useful | ||
| // if we implement P2P sharing in the future. | ||
| if err := d.copyBundles(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Add copied and cached bundles to the registry. | ||
| if err := d.Discover(); err != nil { | ||
| return err |
There was a problem hiding this comment.
There is no sense in storing or sharing historic bundles, so any pruning should get rid of them IMO.
peternose
force-pushed
the
peternose/feature/hot-loading
branch
from
49e86ad to
45b2903
Compare
peternose
force-pushed
the
peternose/feature/hot-loading
branch
from
45b2903 to
979c1c2
Compare
Since runtime versions can be configured dynamically, and none of them may be active at a given moment, this function has no meaningful purpose in its current form and how it is currently used.
Extracted the creation of the provisioner, host info and caching quote service into a dedicated helper function to improve code readability.
Fixes a bug that occurs when attempting to abort the runtime immediately after it starts.
The node can now fetch and verify runtime bundles from remote repositories and automatically update to new versions.
peternose
force-pushed
the
peternose/feature/hot-loading
branch
from
979c1c2 to
3716fee
Compare
Issue: #5755
Example status output: