Skip to content
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.

Sign up for GitHub

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

Jump to bottom

go/runtime: Reduce downtime for TEE runtime upgrades #5188

Merged
merged 1 commit into from
Feb 27, 2023
Merged

go/runtime: Reduce downtime for TEE runtime upgrades #5188

merged 1 commit into from
Feb 27, 2023

Conversation

kostko
Copy link
Member

@kostko kostko commented Feb 20, 2023

Based on #5187

@kostko kostko force-pushed the kostko/feature/rt-reattest-consensus-blocks branch 2 times, most recently from e6a3b0f to 64324fc Compare February 21, 2023 11:25
@kostko kostko force-pushed the kostko/feature/rt-tee-parallel branch from 9a63324 to fb367c5 Compare February 21, 2023 11:26
@kostko kostko force-pushed the kostko/feature/rt-reattest-consensus-blocks branch 2 times, most recently from 71eb9e4 to 1cfafd0 Compare February 22, 2023 20:17
Base automatically changed from kostko/feature/rt-reattest-consensus-blocks to master February 23, 2023 12:32
@kostko kostko force-pushed the kostko/feature/rt-tee-parallel branch 4 times, most recently from c27ec60 to 1538927 Compare February 23, 2023 14:15
@kostko kostko marked this pull request as ready for review February 23, 2023 14:16
@kostko kostko force-pushed the kostko/feature/rt-tee-parallel branch from 1538927 to 242c958 Compare February 23, 2023 14:16
@codecov
Copy link

codecov bot commented Feb 23, 2023 •
edited
Loading

Codecov Report

Merging #5188 (9a46f82) into master (cf5e508) will decrease coverage by 0.17%.
The diff coverage is 34.34%.

❗ Current head 9a46f82 differs from pull request most recent head 1b2b6b3. Consider uploading reports for the commit 1b2b6b3 to get more accurate results

@@            Coverage Diff             @@
##           master    #5188      +/-   ##
==========================================
- Coverage   61.47%   61.30%   -0.17%     
==========================================
  Files         512      512              
  Lines       54051    54211     +160     
==========================================
+ Hits        33227    33234       +7     
- Misses      16611    16745     +134     
- Partials     4213     4232      +19
Impacted Files Coverage Δ
go/registry/api/api.go 52.78% <0.00%> (-0.16%) ⬇️
go/worker/keymanager/worker.go 64.07% <0.00%> (+2.68%) ⬆️
go/worker/compute/executor/committee/node.go 59.01% <22.22%> (-0.16%) ⬇️
go/runtime/host/multi/multi.go 45.52% <24.80%> (-21.38%) ⬇️
go/worker/common/committee/node.go 57.36% <40.00%> (-0.58%) ⬇️
go/runtime/host/sandbox/sandbox.go 56.65% <50.00%> (-0.20%) ⬇️
go/runtime/registry/host.go 49.90% <66.66%> (+0.20%) ⬆️
go/registry/api/runtime.go 51.42% <100.00%> (+1.01%) ⬆️
go/runtime/config/config.go 62.06% <100.00%> (+0.66%) ⬆️
go/runtime/host/mock/mock.go 88.80% <100.00%> (+0.18%) ⬆️
... and 29 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kostko kostko force-pushed the kostko/feature/rt-tee-parallel branch 2 times, most recently from 7886471 to 53fc150 Compare February 24, 2023 09:41
peternose
peternose requested changes Feb 24, 2023
View reviewed changes
Copy link
Contributor

@peternose peternose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and warm-up works. Nice 👍

go/runtime/host/sandbox/sandbox.go Outdated
@@ -225,6 +241,13 @@ func (r *sandboxedRuntime) Stop() {

// Implements host.EmitEvent.
func (r *sandboxedRuntime) EmitEvent(ev *host.Event) {
// Update runtime's CapabilityTEE in case this is an update event.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this comment a bit confusing as it assumes all events can be handled here, but start/stop updates are not. However, the code still works as expected, as it knows that only update events can be sent through this method.
Also not sure if EmitEvent is the right place to change this value. Maybe we subscribe/listen to these events in the manager and change TEE capabilities only there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, refactored this a bit.

go/worker/common/committee/node.go Outdated
var nextVersion *version.Version
if config.GlobalConfig.Mode == config.ModeCompute {
nextDeploy := n.CurrentDescriptor.NextDeployment(n.CurrentEpoch)
if nextDeploy != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if nextDeploy != nil {
if nextDeploy != nil && nextDeploy.ValidFrom - n.CurrentEpoch < 12345 {

We probably don't want to start doing attestations to early.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, made this configurable, please take a look.

@kostko kostko force-pushed the kostko/feature/rt-tee-parallel branch 2 times, most recently from 299e4a6 to 9a46f82 Compare February 27, 2023 10:54
@kostko kostko enabled auto-merge February 27, 2023 12:14
@kostko kostko force-pushed the kostko/feature/rt-tee-parallel branch from 9a46f82 to 1b2b6b3 Compare February 27, 2023 20:32
@kostko kostko merged commit 047aaec into master Feb 27, 2023
@kostko kostko deleted the kostko/feature/rt-tee-parallel branch February 27, 2023 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peternose peternose peternose approved these changes

@peterjgilbert peterjgilbert Awaiting requested review from peterjgilbert peterjgilbert is a code owner

@pro-wh pro-wh Awaiting requested review from pro-wh pro-wh is a code owner

@ptrus ptrus Awaiting requested review from ptrus ptrus is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kostko @peternose