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

Adding support to ContainerizedVM #333

Merged
merged 4 commits into from
Jun 26, 2015

Conversation

gablg1
Copy link
Contributor

@gablg1 gablg1 commented Jun 23, 2015

This PR adds the ContainerizedDebian class, which allows anyone to run benchmarks within a Ubuntu docker container. Every call to RemoteCommand runs within the Docker container via docker exec, whereas any call to RemoteHostCommand is guaranteed to run the command in the VM, outside the container.

This was only tested in GCE so far.

@Amey-D
Copy link

Amey-D commented Jun 23, 2015

@ehankland

if FLAGS.scratch_disk_type == disk.LOCAL:
vm.SetupLocalDisks()
for disk_spec in vm.disk_specs:
vm.CreateScratchDisk(disk_spec)

# This must come after Scratch Disk creation to support the
# Containerized VM case
vm.OnStartup()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will break RHEL for benchmarks that use disks - when they boot, you can't use sudo over ssh without using the '-t' option - we do some setup in the startup that gets around this. The Windows support I'm adding also expects OnStartup to be called immediately after boot before configuring the disks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, is moving the package-manager specific commands (i.e. AptUpdate) to a different method (maybe PrepareVMEnvironment?) that gets called after Scratch disk creation a viable idea? docker run has to be called after mounting the volumes, but it has to be called before any apt-get commands

@ehankland
Copy link
Contributor

test this please.

@ehankland
Copy link
Contributor

("test this please" triggers our integration/unit tests)

@ehankland
Copy link
Contributor

Looks like our linter failed - can you please run "flake8" locally and fix the errors it gives you?

if container_path[-1] == '/':
container_path = os.path.join(container_path, file_name)

command = "cat %s | sudo docker exec -i %s bash -c 'cat > %s'" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the recommended way to do the copy? The solution here seems cleaner to me:
http://stackoverflow.com/questions/22907231/copying-files-from-host-to-docker-container

@gablg1
Copy link
Contributor Author

gablg1 commented Jun 24, 2015

test this please.

@gablg1
Copy link
Contributor Author

gablg1 commented Jun 24, 2015

@ehankland

@ehankland
Copy link
Contributor

test this please.

@@ -215,11 +215,16 @@ def AddMetadata(self, **kwargs):
vm_util.IssueCommand(cmd)


class ContainerizedGceVirtualMachine(GceVirtualMachine,
linux_vm.ContainerizedDebianMixin):
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a default image here

"""
pass


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove 1 of the 2 blank lines

@gablg1
Copy link
Contributor Author

gablg1 commented Jun 26, 2015

Fixed the issues from yesterday @ehankland

copy_to: True to copy to VM, False to copy from VM.
"""
if copy_to:
file_name = posixpath.basename(file_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this has to be:
if copy_to:
base_name = os.path.basename(file_path)
tmp_path = posixpath.join(vm_util.VM_TMP_DIR, base_name)
self.RemoteHostCopy(file_path, tmp_path, copy_to)
self.ContainerCopy(base_name, remote_path, copy_to)
else:
....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed now

@ehankland
Copy link
Contributor

Thanks - seems to be working on a windows controller. One last question - are there any benchmarks that this doesn't work on? Or does it work for everything?

@gablg1
Copy link
Contributor Author

gablg1 commented Jun 26, 2015

Awesome. I tested everything that doesn't require a .boto file (we weren't
very interested in that so I didn't spend time configuring that). It works
on all benchmarks but bonnie++ because it can't run as root by default. The
workaround for Bonnie++ is to add -u root but my adviser said this was
likely not a long term solution so I left it out of the PR.

On Friday, June 26, 2015, ehankland notifications@github.com wrote:

Thanks - seems to be working on a windows controller. One last question -
are there any benchmarks that this doesn't work on? Or does it work for
everything?


Reply to this email directly or view it on GitHub
#333 (comment)
.

@ehankland
Copy link
Contributor

Can you document that you can't run bonnie++ with that VM type? Other than that, LGTM.

@ehankland ehankland added the LGTM label Jun 26, 2015
ehankland added a commit that referenced this pull request Jun 26, 2015
Adding support to ContainerizedVM
@ehankland ehankland merged commit a09ea21 into GoogleCloudPlatform:dev Jun 26, 2015
klausw added a commit that referenced this pull request Jul 9, 2015
Release 0.18.0.

(See also #369
which includes this change log with clickable GH-* links.)

* New features:
  * Support OpenStack as cloud provider (GH-305, GH-353, thanks @kivio and
    @mateusz-blaszkowski)
  * Support Rackspace as cloud provider (GH-336, thanks @meteorfox and @jrperritt)
  * Add support for ContainerizedVM using docker exec (GH-333, thanks @gablg1)
  * Windows guest VM support on Static VM (GH-350), Azure (GH-349, GH-374), AWS
    (GH-347), and GCE (GH-338)
  * Add NTttcp Windows networking benchmark (GH-348)

* Enhancements:
  * Support using proxies in VMs (GH-339, GH-337, thanks @kivio)
  * Enable optional migration on GCE (GH-343)
  * Execute long running commands via a remote agent (GH-310)
  * Add resource creation/deletion times to logs (GH-316)

* Bugfixes and maintenance updates:
  * Update PKB to work with Azure version 0.9.3 (GH-312)
  * Fix AWS CLI usage on Windows host (GH-313)
  * Auto-fetch AMI IDs for AWS images (GH-364)
  * Fix publisher missing info for default image and machine type (GH-357)
  * Fix 'no attribute pkb_thread_log_context' error for sub-thread logs (GH-322)

* Benchmark-specific changes:
  * aerospike: config/flag handling bugfixes (GH-367, GH-360, GH-354)
  * cassandra_ycsb: move num_vms prerequisite check
  * fio: add latency percentiles for results (GH-344)
  * hadoop_terasort: Fix bad SSH option (GH-328)
  * iperf: add lower bounds to arguments (GH-314)
  * iperf: add timeout to parallel benchmark runs to handle iperf hangs (GH-375)
  * netperf: Support confidence intervals, increase test length, report stddev
    (GH-317, GH-306)
  * ycsb: Drop unaggregatable results from samples (GH-324)

* Development and testing:
  * **Breaking Change** Automated testing now uses `tox` (GH-330)
  * Refactored hook scripts, including new opt-in pre-push hook (GH-363)
  * Use travis for CI testing (GH-340)
  * Speed up tests using timeouts (GH-299)

* Internals:
  * Move defaults from benchmark_spec to VM classes, move network instantiation
    out of benchmark spec (GH-342)
  * Add event hook support (GH-315)
  * Refactor VM classes (GH-321)
@klausw klausw mentioned this pull request Jul 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants