Add base layer/uvm helpers to computestorage package #902
Conversation
dcantah
force-pushed
the
computestorage-helpers
branch
2 times, most recently
from
cc2426c to
7468926
Compare
dcantah
force-pushed
the
computestorage-helpers
branch
from
7468926 to
7f9e8d4
Compare
dcantah
force-pushed
the
computestorage-helpers
branch
from
7f9e8d4 to
6c45a15
Compare
computestorage/helpers.go
Outdated
| // already exist. `SetupBaseOSLayer` will create these files internally. We also remove the base and | ||
| // differencing disks if they exist in case we're asking for a different size. | ||
| if _, err := os.Stat(hivesPath); err == nil { | ||
| os.RemoveAll(hivesPath) |
There was a problem hiding this comment.
should we worry about any potential issues here? or paths validity/permissions will happen before calling this? I might be off here and those concerns are not relevant at all.
There was a problem hiding this comment.
Could you elaborate? I'm not fully following
There was a problem hiding this comment.
we're ignoring the error from os.RemoveAll. I took a quick look and some of them are (e.g.) handle wrong paths (with dots in them as one of the cases) and potentially wrong permissions (I think). Is this something we should worry about? for example we're unable to remove something due to wrong path/permissions and we proceed and potentially run into something unexpected?
There was a problem hiding this comment.
Ahh gotcha gotcha. We can check the error for RemoveAll as we're going to error out in HCS if they couldn't remove anyways, better to just fail earlier. Thanks for the clarification!
|
|
||
| // Create the differencing disk that will be what's copied for the final rw layer | ||
| // for a container. | ||
| if err = vhd.CreateDiffVhd(diffVhdPath, baseVhdPath, defaultVHDXBlockSizeInMB); err != nil { |
There was a problem hiding this comment.
the shared logic should be applied to both container and pod? if yes, is there a clean way to incapsulate it into one place, so we don't need to worry about having to remember to check both places? nothing needs to change, more of a patterns question here.
There was a problem hiding this comment.
Ditto here, not fully following the question :(
There was a problem hiding this comment.
mostly referring to similarities in what SetupContainerBaseLayer and SetupSandboxBaseLayer do in the beginning of the method (i.e. os.RemoveAll calls) on the same paths as well as in the end CreateDiffVhd/Grant*calls in the end, which is some shared code at the start and at the end with some different functionality in between.
so my question basically comes down to: can we dedup this somehow?
again, I'm not too worried about this. just seen some code that does stuff like:
func DoSomethingGenericWithSideEffect(func sideEffectAction(...), ...) {
stuffAtTheStart()
sideEffectAction(...)
stuffAtTheEnd()
}
this is obviously not a place to do it, but for mostly educational purposes I was asking about other options (patterns) do we usually have?
There was a problem hiding this comment.
I wouldn't worry about this too much and we can discuss this offline
* Add helper functions to setup the disks for a base WCOW layer. Signed-off-by: Daniel Canter <[email protected]>
dcantah
force-pushed
the
computestorage-helpers
branch
from
6c45a15 to
b0ed708
Compare
Related work items: microsoft#173, microsoft#839, microsoft#856, microsoft#877, microsoft#881, microsoft#886, microsoft#887, microsoft#888, microsoft#889, microsoft#890, microsoft#893, microsoft#894, microsoft#896, microsoft#899, microsoft#900, microsoft#902, microsoft#904, microsoft#905, microsoft#906, microsoft#907, microsoft#908, microsoft#910, microsoft#912, microsoft#913, microsoft#914, microsoft#916, microsoft#918, microsoft#923, microsoft#925, microsoft#926, microsoft#928, microsoft#929, microsoft#932, microsoft#933, microsoft#934, microsoft#938, microsoft#939, microsoft#942, microsoft#943, microsoft#945, microsoft#946, microsoft#947, microsoft#949, microsoft#951, microsoft#952, microsoft#954
4 participants
Signed-off-by: Daniel Canter [email protected]