-
Notifications
You must be signed in to change notification settings - Fork 40
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
Remove loader.entrypoint
from manifest template
#225
base: master
Are you sure you want to change the base?
Remove loader.entrypoint
from manifest template
#225
Conversation
This is a commit corresponding to commit 87c752f "[PAL] Drop deprecated syntax of loader.entrypoint" in core Gramine repository. Signed-off-by: Kailun Qin <[email protected]>
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.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required)
I additionally tested the PR in Gramine-Direct mode, and found that the fix works only for Gramine-SGX but causes issues in Gramine-Direct. I've attached both manifest files for your reference. You'll notice that entrypoint.manifest.sgx includes loader.entrypoint, whereas entrypoint.manifest does not. Could you please investigate this further? Logs:
|
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.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required)
a discussion (no related file):
Thanks @anjalirai-intel, good catch!
I think the issue lies in the manifest preprocessing (which auto-fills loader.entrypoint.uri
, see here) implemented in core Gramine is not invoked when generating entrypoint.manifest
in gsc (which is used by gramine-direct
). Note that this is implicitly called during gsc sign-image
/gramine-sgx-sign
(see here), so it works fine in gramine-sgx
mode.
I think the simplest solution would be to keep loader.entrypoint.uri
(w/ the updated name) in the manifest templates. Alternatively, we could implement/call similar logic in gsc
.
@kailun-qin Please review PR #223 ; includes |
@DukeDavis12 PR #223 is also not fixing the issue in gramine-direct. I am getting below error when running with Gramine-Direct Error:
|
@kailun-qin Any updates on the PR? |
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.
@anjalirai-intel: I think @DukeDavis12 would want to introduce a fix which is somewhat tangled w/ #223?
Reviewable status: all files reviewed, 1 unresolved discussion
@kailun-qin please go ahead merge this fix. @DukeDavis12 will rebase after. |
Can we merge 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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anjalirai-intel)
a discussion (no related file):
Previously, kailun-qin (Kailun Qin) wrote…
Thanks @anjalirai-intel, good catch!
I think the issue lies in the manifest preprocessing (which auto-fills
loader.entrypoint.uri
, see here) implemented in core Gramine is not invoked when generatingentrypoint.manifest
in gsc (which is used bygramine-direct
). Note that this is implicitly called duringgsc sign-image
/gramine-sgx-sign
(see here), so it works fine ingramine-sgx
mode.I think the simplest solution would be to keep
loader.entrypoint.uri
(w/ the updated name) in the manifest templates. Alternatively, we could implement/call similar logic ingsc
.
So, this is still broken? What's the status of this issue?
Description of the changes
This is a commit corresponding to commit 87c752f "[PAL] Drop deprecated syntax of loader.entrypoint" in core Gramine repository.
Fixes #224.
How to test this PR?
Manual testing.
This change is