feat: add new disk service #515
Conversation
core/ports/services.go
Outdated
| //Path is the filesystem path of where to create the disk. | ||
| Path string | ||
| // Size is how big the disk should be. It uses human readable formats | ||
| // suck as 8Mb, 10Kb. |
There was a problem hiding this comment.
| // suck as 8Mb, 10Kb. | |
| // such as 8Mb, 10Kb. |
| if _, err := s.fs.Stat(input.Path); err == nil { | ||
| if removeErr := s.fs.Remove(input.Path); removeErr != nil { | ||
| return fmt.Errorf("removing disk %s: %w", input.Path, removeErr) | ||
| } |
There was a problem hiding this comment.
can you explain this bit here: why do we delete whatever it at the input path first? it is to clean whatever is there before we create over it?
There was a problem hiding this comment.
Yep it's basically that. If the disk image exists already then delete and we will re-create.
What do you think to that default behaviour? Should i make it configurable? Perhaps add an option to DiskCreateInput valled OverwriteExisting?
There was a problem hiding this comment.
maybe.
worst case is someone sets Path to something they should not and it is erased. having it error unless told explicitly to overwrite may be useful
There was a problem hiding this comment.
Agreed, changing now...
There was a problem hiding this comment.
@Callisto13 - all changed.
| return fmt.Errorf("creating disk %s: %w", input.Path, err) | ||
| } | ||
|
|
||
| createdDisk.LogicalBlocksize = 512 |
There was a problem hiding this comment.
can we have that in a const?
There was a problem hiding this comment.
Yep, that would've been a const to start with 👍
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
could be return err but maybe that would read weird
There was a problem hiding this comment.
I like it, a lot neater.
| func appPorts(repo ports.MicroVMRepository, prov ports.MicroVMService, es ports.EventService, is ports.IDService, ns ports.NetworkService, ims ports.ImageService, fs afero.Fs) *ports.Collection { | ||
| func appPorts(repo ports.MicroVMRepository, prov ports.MicroVMService, es ports.EventService, is ports.IDService, ns ports.NetworkService, ims ports.ImageService, fs afero.Fs, ds ports.DiskService) *ports.Collection { |
There was a problem hiding this comment.
hrmmm could do with a way of shrinking this signature
There was a problem hiding this comment.
I did link about that but thought as this is for DI i wouldn't bother....it can get a bit messy with wire.
This change adds a new port for a "disk service" that will be used to create disk images (i.e. img/iso files). An infrastructure implemenetation has been added using godisk. Note: this isn't used by anything in this change. But it will be used in the future with the Cloud Hypervisor implementation. Signed-off-by: Richard Case <[email protected]>
What this PR does / why we need it:
This change adds a new port for a "disk service" that will be used to
create disk images (i.e. img/iso files). An infrastructure
implementation has been added using godisk.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Relates #417
Special notes for your reviewer:
Note: this isn't used by anything in this change. But it will be used in
the future with the Cloud Hypervisor implementation.
Checklist: