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

minimega/vmconfiger: several bug fixes, fix #854 #889

Merged
merged 9 commits into from
May 12, 2017

Conversation

jcrussell
Copy link
Contributor

Few related things in this PR:

CPU wasn't included in the list.
Adds pretty simple support for maps to vmconfiger.
Should have been update for commas in slices.
When printing maps.
Allows you to specify multiple volumes to bind mount for a container.

Fixes sandia-minimega#854.
Use new vmconfiger map support to remove Tags special case.
@jcrussell jcrussell added this to the 2.4 milestone Mar 13, 2017
Copy link
Contributor

@djfritz djfritz left a comment

Choose a reason for hiding this comment

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

LGTM; I'd like to test with some use cases before merging.

@jcrussell
Copy link
Contributor Author

Note: I left a TODO to handle spaces in the paths.

@activeshadow: want to give this a test drive?

@activeshadow
Copy link
Contributor

activeshadow commented Mar 17, 2017 via email

@activeshadow
Copy link
Contributor

Sorry, got waylaid last week w/ work. I'm going to take a look at this right now.

@activeshadow
Copy link
Contributor

@jcrussell OK, so I'm seeing some errors when I try to use the new volume config... perhaps I'm going about it wrong.

Here's my config (/tmp/mm-volume-test.txt):

vm config memory 512
vm config filesystem /opt/minimega/sceptre_container_rootfs/

vm config net 10
vm config volume /etc/network/interfaces /tmp/kronos-interfaces
vm launch container kronos

vm config net 10
vm config volume /etc/network/interfaces /tmp/solver-interfaces
vm config volume /tmp/6-bus.pb /tmp/6-bus.pb
vm launch container solver

cc filter hostname=kronos
cc exec sceptre-kronosd --endpoint 172.16.10.10 --command start

cc filter hostname=solver
cc exec pysceptre-dummy-power-server -d -k 172.16.10.10 -pb /tmp/miniccc/files/sceptre/6-bus.pb 2>&1 > /tmp/solver.log

And here's the commands I ran (as root):

$ minimega
minimega, Copyright (2014) Sandia Corporation.
Under the terms of Contract DE-AC04-94AL85000 with Sandia Corporation,
the U.S. Government retains certain rights in this software.
minimega$ read /tmp/mm-volume-test.txt
2017/03/27 12:04:02 WARN vlans.go:334: Blacklisting manually specified VLAN 10
Error (minibuntu): freezer: open /sys/fs/cgroup/freezer/minimega/0/freezer.state: no such file or directory
Error (minibuntu): freezer: open /sys/fs/cgroup/freezer/minimega/1/freezer.state: no such file or directory
minimega$ .columns name,state,type,volume vm info
host      | name   | state | type      | volume
minibuntu | kronos | ERROR | container | {"/etc/network/interfaces":"/tmp/solver-interfaces","/tmp/6-bus.pb":"/tmp/6-bus.pb"}
minibuntu | solver | ERROR | container | {"/etc/network/interfaces":"/tmp/solver-interfaces","/tmp/6-bus.pb":"/tmp/6-bus.pb"}

Since I launched the kronos container prior to changing the volume for /etc/network/interfaces and adding the volume for /tmp/6-bus-pb, I would have expected the host volume to be different for each container, and I wouldn't have expected to see the /tmp/6-bus.pb volume on the kronos container. However, it seems as though the last volume setting is getting applied to both containers.

Do the source/target have to be directories, or can they be actual files too (like I'm assuming they can be here)?

@jcrussell
Copy link
Contributor Author

I know what the bug is -- need to deep copy a map. I can fix it later today.

Hm, I had only considered directories initially but it might be possible to handle files as well. @djfritz, what do you think?

@activeshadow
Copy link
Contributor

activeshadow commented Mar 28, 2017 via email

Need to deep copy volumes map so that all VMs don't share the same one.
@jcrussell
Copy link
Contributor Author

@activeshadow -- that bug should be fixed. You still won't be able to use files as volumes though.

@activeshadow
Copy link
Contributor

@jcrussell OK, I'll test it out shortly and let you know how it goes. Thanks!

@djfritz
Copy link
Contributor

djfritz commented Mar 28, 2017

While you can bind mount an individual file, it's dangerous. If something like vim rewrites the file (which is does by default), a new inode will get created, effectively undoing the bind mount. The only safe way to write to a bind mounted file is to append to it, which isn't terribly useful.

How does docker do this? A symlink?

@djfritz
Copy link
Contributor

djfritz commented Mar 28, 2017

docker has the same issue:

moby/moby#6011

@djfritz
Copy link
Contributor

djfritz commented Mar 28, 2017

From the docker docs:

Note: Many tools used to edit files including vi and sed --in-place may result in an inode change. Since Docker v1.1.0, this will produce an error such as “sed: cannot rename ./sedKdJ9Dy: Device or resource busy”. In the case where you want to edit the mounted file, it is often easiest to instead mount the parent directory.

@djfritz
Copy link
Contributor

djfritz commented Mar 28, 2017

So, no, I suspect we won't support individual files anymore than docker does.

@djfritz
Copy link
Contributor

djfritz commented Mar 28, 2017

@jcrussell maybe support this in the same way but bind mount non-directories as RO and throw a warning?

@jcrussell
Copy link
Contributor Author

It sounds like directories are good enough for @activeshadow's use case. I could add a check to make sure that the source is a directory (and error otherwise).

@activeshadow
Copy link
Contributor

@jcrussell @djfritz volume mounts are working for me!

So. Very. Awesome.

Thanks!

Use helper function instead of generating the same code every time. Move
header from a bunch of printfs to single template.
Copy link
Contributor

@djfritz djfritz left a comment

Choose a reason for hiding this comment

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

LGTM

@djfritz djfritz merged commit 139ce34 into sandia-minimega:master May 12, 2017
@jcrussell jcrussell deleted the issue-854 branch May 12, 2017 20:19
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.

3 participants