Resource Cleanup (Layers, UVM and Hcsoci package relationship and more)
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 694
- Forks
- 304
- Avg merge
- 1d 19h
- Merged PRs (30d)
- 28
Description
This issue is more of a roadmap of what would be ideal to change in the codebase in the future in relation to how resources are allocated/freed/modified and the relation the internal/hcsoci and internal/uvm package currently have with handling these resources. The first part of this work has been started and is in this PR https://github.com/microsoft/hcsshim/pull/772. As things get merged I'll revise this issue and cross off work that has been done or if anything new crops up that would be nice to refactor in these areas I will add to this as well.
Current:
-
Most resources don't have concrete types and most of the Add, Remove, Modify functions simply deal with paths that these resources are mounted to.
-
To release all of a containers resources you call ReleaseResources https://github.com/microsoft/hcsshim/blob/fd0797d766b1933ade4b9f6c12c341ddc9c9f95f/internal/hcsoci/resources.go#L83 and pass in a *UtilityVM object, a *Resources object along with a boolean that signifies to clean up all mounts that aren't for layers. Network endpoints and layers are always released regardless of the bools value. SCSI mounts, VSMB shares, Plan9 mounts, Pipe mounts and the newly added VPCI devices all only get released if this bool is true and we would like to keep this functionality, although perhaps in a different way.
-
All of the resource release logic is done inline in ReleaseResources instead of separated out or done by the resources themselves (currently not possible as the resources are mostly just slices of paths to the mounts).
-
Most management of these resources (for example scsi and vsmb ref counting) is currently done in the uvm package and maybe should ideally be done in hcsoci.
PR https://github.com/microsoft/hcsshim/pull/772: MERGED
-
Make an interface that all resources will implement so that they can be generically released and we don't have to worry about what type the resource is. Currently all resources besides layers (more on this below) implement the interface. This makes it so we can simply loop through one slice of resources and call .Release(ctx) and let the objects themselves handle the teardown logic.
-
All resources now have backing structs and all Add<VSMB, SCSI, etc> functions return these types. All of these types implement the ResourceCloser interface mentioned above and have a handle to the UtilityVM they belong to so they are able to call the necessary uvm methods they need to get freed.
-
Simplified ReleaseResources as logic for release is now done by the objects themselves instead of inline.
Going Forward:
-
Ideally the ref-counting and other similar functions of resources that currently take place in the uvm package should be migrated to hcsoci.
-
All Remove functions should be unexported and have all resource removal strictly be done with the objects themselves. A caller should never have to call Remove<SCSI, VSMB> and friends. This would mostly be a pain point with certain tests at the moment but otherwise should be fairly simple.
-
Layers cleanup somehow needs to not rely on the all bool mentioned in Current for its decision to remove all layers or needs to get this value some other way to be able to implement ResourceCloser. There's also the question of should layers even be considered a resource?
-
In MountContainerLayers in the current PR, https://github.com/dcantah/hcsshim/blob/e950d23138be9f4001283d82025ece11426cd032/internal/hcsoci/layers.go#L54 if it is a xenon container and doesn't mount layers using wclayer, all objects returned from AddSCSI etc calls essentially get discarded. They aren't embedded in another object or returned in anyway. I don't know what the best solution for where to store them is (perhaps nested in an *ImageLayers and we change the return type of this func) but simply discarding them isn't ideal. This change would play well with UnmountContainerLayers also for obvious reasons.
-
Perhaps split some of the functionality of some functions that do various checks to determine if the container is vm isolated (if uvm != nil's and other checks) to separate functions and the caller of these can determine which to call instead of having all of the logic in one large monolithic function. May not be a good idea but would make readability easier.
-
Make ReleaseResources a method on the Resources struct.
-
Maybe make a new struct that houses the hcs.System and Resources, and this can be embedded in a uvm for the xenon case, otherwise can just be interacted with directly.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with internal/hcsoci/resources.go and layers.go, then review ReleaseResources, MountContainerLayers, and UnmountContainerLayers alongside the merged PR #772. The issue is a roadmap rather than a single scoped task; completion would require selecting and defining one of the listed resource-management refactors, with corresponding tests and cleanup behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- infrastructure, operating-systems
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 15/100