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

kvm/qemu mounting #598

Merged
merged 5 commits into from
Sep 22, 2020
Merged

kvm/qemu mounting #598

merged 5 commits into from
Sep 22, 2020

Conversation

ronaudinho
Copy link
Contributor

RFC

  1. currently using lousy implementation of JSON store to keep track of created volumes, maybe use some more established KV store?
  2. ops volume create to create volume is necessary before running ops run --mounts <uuid:mount_path> since UUID is necessary. my idea is that when user wants to mount on run, they could perhaps just provide host_path and that new volume will be created and attached on the fly?

sample usage can be seen at ronaudinho/ops-examples@722fff5
(i tested JSON read/write, but on write, json.Unmarshal result is weird, so i do not call it in the main)

references

  1. https://github.com/solo-io/unik/blob/master/pkg/client/volumes.go
  2. https://github.com/solo-io/unik/blob/master/docs/cli.md#create-a-volume
  3. [WIP] Implement VirtioFS device backend firecracker-microvm/firecracker#1351

TODO

  1. work with non-empty volume
  2. update volume state on attach/detach
  3. refactor volumeStore

`ops volume attach` lets user attach volume to a stopped instance.
as of this commit, `ops run` overwrite this config though.

`ops run --mounts` lets user mount volume to an instance on run, but
does not persist the config. as of this commit, changes made to volume
during run is visible, but gets discarded on subsequent run without
`--skipbuild`
@ronaudinho ronaudinho marked this pull request as ready for review September 21, 2020 18:44

// saveImageConfig saves image config as JSON
// for volume attach/detach purposes
func saveImageConfig(c Config) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is fine for now but something we should prob. fix in the future - a good chunk of the variables being stored here are dynamic in nature and don't belong here;

also - 'manifests' have an inherent meaning already and probably wise to change this to a different phrase

Copy link
Contributor

@eyberg eyberg left a comment

Choose a reason for hiding this comment

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

i'd dbl chk that dupe comment && might be worth adding a test of some kind but otherwise we can merge

}

var vmpath string
if hostpath[0] != '/' {
Copy link
Contributor

Choose a reason for hiding this comment

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

dupe? not needed

@eyberg eyberg merged commit 7ebc83e into nanovms:master Sep 22, 2020
@eyberg
Copy link
Contributor

eyberg commented Sep 22, 2020

merging since original comments were fixed && i've created tkts for the other issues and this has been sitting for a few weeks now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants