-
Notifications
You must be signed in to change notification settings - Fork 178
Docker instances created by Cloud Orchestrator enables vhost_user_vsock as true. #1721
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
Open
0405ysj
wants to merge
3
commits into
google:main
Choose a base branch
from
0405ysj:enable_vhost_user_vsock
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
If vhost_user_vsock is part of the canonical configuration sent in the create cvd request why do we need this as part of the HO configuration as well?
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.
What I want to achieve is enabling vhost_user_vsock option for any HO in docker instance launched by CO. If vhost_user_vsock is disabled, we cannot launch CF instances across multiple docker instances. It's enabled on arm64, but it's disabled on x86_64.
When HO receives
POST /cvdsAPI request, it may or may not contain canonical configuration as input. If so, it callscvd loadto launch CF with given canonical configuration. Or, it callscvd createto launch CF based on option flags. Ascvdr createdeals with both kinds viaPOST /cvds, HO in docker instance should be able to handle both approaches to achieve above one.Uh oh!
There was an error while loading. Please reload this page.
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.
POST /cvdswithout canonical config is deprecated, please do not add anything new to this path.Having said that, the source of truth for cuttlefish creation in HO is the canonical config received in the request, this ensure reproducibility, I can copy/paste the config received by the HO and create the same device in my desktop using
cvd createdirectly, this something I do on a regular basis when device create fails in remote hosts. By adding this config parameter as a HO service configuration you are disrupting this behavior adding a new source of configuration towards device creation breaking the HO device creation reproducibility capability.If you need to alter the canonical config in Docker Deployment, please do that at the Cloud Orchestrator layer, where you can intercept the request and modify it, there's no need to add another source of configurations towards creating cuttlefish devices, canonical configurations should be the only one.
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 would take a look whether intercepting and modifying HTTP request from https://github.com/google/cloud-android-orchestration/blob/main/pkg/app/instances/hostclient.go by CO is applicable or not. If it's applicable, I think CO with DockerIM can adjust canonical config by itself to add
vhost_user_vsockastrue.However, I still think I need to support enabling
vhost_user_vsockvia HO forPOST /cvdswithout canonical config too, though it's deprecated. I'm understanding one of main flows at cvdr iscvdr createwith--build_idorbuild_targetoptions, but it still doesn't use canonical config at HO. Also, I'm not even sure whether canonical config supports build ID/target, which may require help fromcvd fetch. I agree with deprecating all flows without canonical config from HO or cvdr, but I think enablingvhost_user_vsockshould be orthogonal from the conversion task, not a blocker. HDYT? @ser-ioUh oh!
There was an error while loading. Please reload this page.
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.
You need to make relevant changes to
cvdrhaving commands likecvdr create --build_id --build_targetusing canonical config behind scenes, this is something I've been trying to do but don't have the time.google/cloud-android-orchestration#374 is a reference of migrating to canonical config when using local artifacts.