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.

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

Support adding mount to running containers #1918

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

kiashok
Copy link
Contributor

@kiashok kiashok commented Sep 27, 2023

This PR adds hcsshim support to mount host volumes/directories to running windows containers.

HCS supports mounting host volumes/directories to running windows containers by modifying the hcs compute system with ResourcePath of Containers/MappedDirectories. These changes leverage this by extending the hcsTask.Update() function to process and add mounts for running process isolated and hyperV windows containers.

@kiashok kiashok requested a review from a team as a code owner September 27, 2023 22:34
@kiashok kiashok force-pushed the sqlmi-mount-mainBranch branch 2 times, most recently from c831568 to 973e9b0 Compare September 28, 2023 19:56
Copy link
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

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

Can we not rely on (*UtilityVM) Share and add an analogous function to internal/hcs.System?
Id rather not bring HCS specifics into cmd

@kiashok kiashok force-pushed the sqlmi-mount-mainBranch branch 2 times, most recently from 3752fc7 to 50b6e1d Compare September 28, 2023 20:31
@kiashok
Copy link
Contributor Author

kiashok commented Sep 28, 2023

Can we not rely on (*UtilityVM) Share and add an analogous function to internal/hcs.System? Id rather not bring HCS specifics into cmd

what HCS specifics are you referring to? task_hcs.go is already sending ht.c.Modify() modify requests for updating CPU and memory resources.

@kiashok
Copy link
Contributor Author

kiashok commented Sep 28, 2023

cc @kharpMSFT

@helsaawy
Copy link
Contributor

helsaawy commented Oct 2, 2023

what HCS specifics are you referring to? task_hcs.go is already sending ht.c.Modify() modify requests for updating CPU and memory resources.

the logic in updateWCOWContainerMount; we already have it implemented in uVM code, and (aside for portions of the update and properties/stats code for hcsTask), most of the HCS schema creation logic is pushed to hcsoci or elsewhere instead of in cmd\containerd-shim-runhcs-v1

@kiashok
Copy link
Contributor Author

kiashok commented Oct 2, 2023

what HCS specifics are you referring to? task_hcs.go is already sending ht.c.Modify() modify requests for updating CPU and memory resources.

the logic in updateWCOWContainerMount; we already have it implemented in uVM code, and (aside for portions of the update and properties/stats code for hcsTask), most of the HCS schema creation logic is pushed to hcsoci or elsewhere instead of in cmd\containerd-shim-runhcs-v1

The uvm.Share() function seems to be slightly different. It is sending a request with a different resource path type and is also a common code path and is not adding the share to release resource that is being mounted (not sure why this is ) . I think it would be better to have the mounting logic for running containers exclusive to this add-mount logic file as a non-exportable function if possible.

@kiashok kiashok force-pushed the sqlmi-mount-mainBranch branch 3 times, most recently from 0485caa to 7f25b56 Compare October 2, 2023 22:01
@kiashok
Copy link
Contributor Author

kiashok commented Oct 2, 2023

what HCS specifics are you referring to? task_hcs.go is already sending ht.c.Modify() modify requests for updating CPU and memory resources.

the logic in updateWCOWContainerMount; we already have it implemented in uVM code, and (aside for portions of the update and properties/stats code for hcsTask), most of the HCS schema creation logic is pushed to hcsoci or elsewhere instead of in cmd\containerd-shim-runhcs-v1

The uvm.Share() function seems to be slightly different. It is sending a request with a different resource path type and is also a common code path and is not adding the share to release resource that is being mounted (not sure why this is ) . I think it would be better to have the mounting logic for running containers exclusive to this add-mount logic file as a non-exportable function if possible.

@helsaawy Refactored the code a little to reuse some part of it from uvm.Share() in the second commit of this PR. But no sure this can entirely move out to internal/* though. Let me know what you think

- Extend hcsTask.Update() to process and add
mount for running process isolated and hyperV
wcow containers

Signed-off-by: Kirtana Ashok <kiashok@microsoft.com>
@kiashok kiashok merged commit aad7467 into microsoft:main Oct 12, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants