-
Notifications
You must be signed in to change notification settings - Fork 0
TA4: Enclave restart test #215
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
base: celo-integration-rebase-13.2
Are you sure you want to change the base?
Conversation
1ce717f to
9051371
Compare
dailinsubjam
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 good.
|
Why is it possible to run the test locally even without a TEE? |
|
@philippecamacho If you're not on a enclave-enabled instance, you'll hit this line and it will automatically skip enclave-related operations. |
| testRestart(t, false) | ||
| } |
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 just realized to restart op-batcher-tee we not only need profile to be tee but also need things like restarting the service op-batcher-tee specifically.
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.
Oh, that's a big catch for sure, thanks. No idea how it passed for me in the first place.
I see, but when this test runs in CI, is the batcher executed inside the TEE? |
No, it isn't. Only non-tee test runs. |
4bf678e to
3a01ee2
Compare
74b5abf to
5475f6d
Compare
|
@dailinsubjam @philippecamacho this works now, I've added a Not set up to run in CI unfortunately, building dockers on AWS machine takes too long and GitHub kills the action. To run this in CI, we need to set up building dockers in one action and re-using in subsequent ones, including uploading them to the AWS Nitro instance, which I'd argue out of scope for this PR, LMK if you disagree. |
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.
TestEnclaveRestart pass for me.
But docker ps after the test, I can see
docker ps
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
224fe2454142 op-proposer:espresso "/bin/entrypoint.sh …" 13 minutes ago Up 10 minutes espresso-op-proposer-1
78fefd376a3b op-challenger:espresso "/bin/entrypoint.sh …" 13 minutes ago Up 10 minutes espresso-op-challenger-1
099c18b6b26a op-batcher:espresso "op-batcher --espres…" 13 minutes ago Up 10 minutes espresso-op-batcher-1
remains running.
We can open a separate ticket for cleaning them up.
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 see we’ve added a backup of the backup of the original… bold move 😎
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.
Oh heck, this is from mergiraf, I'll remove it stat 🤣
| func (d *Devnet) ServiceDown(service Service) error { | ||
| serviceName := d.getServiceName(service) | ||
| log.Info("shutting down service", "service", serviceName) | ||
| cmd := exec.CommandContext( | ||
| d.ctx, | ||
| "docker", "compose", "down", service, | ||
| "docker", "compose", "--profile", d.getProfile(), "down", serviceName, | ||
| ) | ||
| return cmd.Run() | ||
| } |
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.
This reminds me of one thing: op-batcher-tee container spawns an enclave container (batcher-enclaver-xxx) that doesn’t stop automatically when op-batcher-tee stops. In Docker Compose I’ve been cleaning it up manually via espresso/scripts/shutdown.sh, but that’s probably not the best approach.
I'm thinking we may see the same issue here, even if op-batcher-tee exits, batcher-enclaver-xxx may keep running.
One possible way is to add a cleanup hook when shutting downop-batcher-tee (not sure whether it's supported) or also add a manual shutdown here. WDYT?
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.
Oh interesting. I'll see what we can do here
dailinsubjam
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.
Oops, I just found tee is not triggered automatically though I'm on a AWS Nitro, will take a further look.
tee is not triggered in my run for the test
|
@dailinsubjam I've mixed up order of operations during rebase, my tee flag was set after the devnet was spun up 🤦 |
|
I got this Started in tmux so I didn't fetch the full log TAT I can fetch the complete logs with a new run later |
e76deaa to
f961472
Compare
Closes #<ISSUE_NUMBER>
This PR:
This PR does not:
Key places to review: