-
Notifications
You must be signed in to change notification settings - Fork 275
HvSocket support for containers #2353
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
Conversation
4d40dbd to
caf030c
Compare
helsaawy
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.
does the address info ioctl have to go through before the container is created?
if not, then we could probably remove the HCSIDToGUID code and instead add the call to CreateAddressInfo in hcsoci.Create after the system is created and we have its GUID from HCS (so we don't rely on their internal logic for generating the GUID):
func CreateContainer(/* ... */) (_ cow.Container, _ *resources.Resources, err error) {
// ...
if gcsDocument != nil {
c, err := coi.HostingSystem.CreateContainer(ctx, coi.actualID, gcsDocument)
if err != nil {
return nil, r, err
}
if p, err := c.PropertiesV2(ctx); err != nil {
return nil, r, fmt.Errorf("retrieve created container compute system properties: %w", err)
}
hvsockAI, err := hvsocket.CreateAddressInfo(p.RuntimeId, coi.HostingSystem.RuntimeID(), true)
if err != nil {
return nil, r, fmt.Errorf("exposing container Hyper-V socket: %w", err)
}
r.Add(hvsockAI)
return c, r, nil
}
}|
on a side note, how are users expected to get the runtime ID of the container for creating hv sockets? |
|
What is the use case here? Is user expected to connect to inside the UVM using the containerId? |
caf030c to
774f1a4
Compare
turned out RuntimeID is empty... I had to update the schema and explicitly ask for systemID, which is what we need. |
The use case is a client/server application running on containerd host and server/client (respectively) running in a Xenon container communicating via HvSocket. shimdiag wouldn't be able to provide this type of communication. |
Can the application running on containerd host not use network stack to talk to inside container? What is the use case for using hvsockets? With Live Migration, container Id changes when containers start on destination. With hvsocket communication to the container, LM will have to consider that this additional connection does not break. |
| if err != nil { | ||
| return nil, r, fmt.Errorf("query created container properties failed: %w", err) | ||
| } | ||
| containerSystemGUID, err := guid.FromString(props.SystemGUID) |
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.
Would this value of containerSystemGUID be same as the return value of running HCSIDToGUID(coi.actualID)?
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.
yeah, they are the same.
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.
Can we just use the same function here too then? Just so that any changes will have to be made only at one place?
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.
The function, really, is just a convenience/alternative way to get the VMID. The recommended way would be to query HCS.
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.
Hmm, then can the function just call HcsGetComputeSystemProperties and get this ID? My only concern is that something can change in HCS and then our implementation of generating this ID won't be correct anymore, so better to use the same implementation at both places.
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.
as discussed offline, moving this to internal/tools. I'm a bit hesitant making opening additional handles to compute system to query for properties.
434aa74 to
57059f2
Compare
internal/hvsocket/hvsocket.go
Outdated
| return windows.CloseHandle(aic.handle) | ||
| } | ||
|
|
||
| func CreateAddressInfo(cid, vmid guid.GUID, passthru bool) (resources.ResourceCloser, error) { |
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.
Do we anticipate cases where the passthru value will be false? If not, can we make this an implementation detail of this function and always pass the flag?
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.
it can be relevant for Krypton... but we can revise this if necessary.
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 am leaning towards not keeping this for now and later on if required we can add feature specific options here that internally will translate to this flag being set. But if we do want to keep this flag, we should include function comments explaining what it does.
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've done some refactoring and added comments. lmk your thoughts.
3ea601c to
6b567c4
Compare
e1c7a5e to
c9ea816
Compare
f311137 to
bcab197
Compare
Applications connecting from the host into the container should use container-specific VMID. This ID will need to be the same as the container's VMID inside the guest, which is calculated by HCS/GCS like it's done in this PR by `HCSIDToGUID`. To allow the container ID to work with HvSocket on the host, we need to set up an AddressInfo mapping to tell HvSocket to redirect the call into the UVM, which is done in this PR by default for all WCOW containers. Add `hvsocketaddr.exe` that clients can use to generate VM ID for container. Signed-off-by: Maksim An <[email protected]>
add a generic function for creating HvSocket address info mapping. export a function that creates a mapping for containers only. Signed-off-by: Maksim An <[email protected]>
Signed-off-by: Maksim An <[email protected]>
bcab197 to
ba1a4cd
Compare
Applications connecting from the host into the container should use
container-specific VMID. This ID will need to be the same as the
container's VMID inside the guest, which is calculated by HCS/GCS
like it's done in this PR by
HCSIDToGUID.To allow the container ID to work with HvSocket on the host, we
need to set up an AddressInfo mapping to tell HvSocket to redirect
the call into the UVM, which is done in this PR by default for
all WCOW containers.