respond to review comments

perfkitbenchmarker/benchmark_sets.py

@@ -160,6 +160,7 @@ def GetBenchmarksFromFlags():
     if benchmark_name in valid_benchmarks:
       benchmark_module_list.append(valid_benchmarks[benchmark_name])
     else:
-      raise ValueError('Invalid benchmark %s' % benchmark_name)
+      raise ValueError('Benchmark "%s" not valid on os_type "%s"' %
+                       (benchmark_name, FLAGS.os_type))
 
   return benchmark_module_list

perfkitbenchmarker/benchmark_spec.py

@@ -225,8 +225,9 @@ def CreateVirtualMachine(self, zone):
 
     vm_classes = CLASSES[self.cloud][VIRTUAL_MACHINE]
     if FLAGS.os_type not in vm_classes:
-      raise errors.Error('The cloud "%s" does not support VMs of type "%s".' %
-                         (self.cloud, FLAGS.os_type))
+      raise errors.Error(
+          'VMs of type %s" are not currently supported on cloud "%s".' %
+          (FLAGS.os_type, self.cloud))
     vm_class = vm_classes[FLAGS.os_type]
 
     vm_spec = virtual_machine.BaseVirtualMachineSpec(

perfkitbenchmarker/disk.py

@@ -64,6 +64,14 @@ def __init__(self, disk_spec):
     self.iops = disk_spec.iops
 
     # Windows related attributes.
+
+    # The disk number corresponds to the order in which disks were attached to
+    # the instance. The System Disk has a disk number of 0. Any local disks
+    # have disk numbers ranging from 1 to the number of local disks on the
+    # system. Any additional disks that were attached after boot will have
+    # disk numbers starting at the number of local disks + 1. These disk
+    # numbers are used in diskpart scripts in order to identify the disks
+    # that we want to operate on.
     self.disk_number = None
 
   @abc.abstractmethod

perfkitbenchmarker/gcp/gce_virtual_machine.py

@@ -237,6 +237,8 @@ class WindowsGceVirtualMachine(GceVirtualMachine,
                                windows_virtual_machine.WindowsMixin):
 
   DEFAULT_IMAGE = WINDOWS_IMAGE
+  BOOT_DISK_SIZE_GB = 50
+  BOOT_DISK_TYPE = disk.REMOTE_SSD
 
   def __init__(self, vm_spec):
     super(WindowsGceVirtualMachine, self).__init__(vm_spec)

perfkitbenchmarker/pkb.py

@@ -178,17 +178,6 @@ def ValidateBenchmarkInfo(benchmark_info):
   return True
 
 
-def ListUnknownBenchmarks():
-  """Identify invalid benchmark names specified in the command line flags."""
-  valid_windows_benchmark_names = frozenset(windows_benchmarks.VALID_BENCHMARKS)
-  valid_benchmark_names = frozenset(benchmarks.VALID_BENCHMARKS)
-  valid_benchmark_sets = frozenset(benchmark_sets.BENCHMARK_SETS)
-  specified_benchmark_names = frozenset(FLAGS.benchmarks)
-
-  return sorted(((specified_benchmark_names - valid_benchmark_names) -
-                valid_benchmark_sets) - valid_windows_benchmark_names)
-
-
 def DoPreparePhase(benchmark, name, spec, timer):
   """Performs the Prepare phase of benchmark execution.
 
@@ -392,12 +381,6 @@ def RunBenchmarks(publish=True):
       run_uri=FLAGS.run_uri)
   _LogCommandLineFlags()
 
-  unknown_benchmarks = ListUnknownBenchmarks()
-  if unknown_benchmarks:
-    logging.error('Unknown benchmark(s) provided: %s',
-                  ', '.join(unknown_benchmarks))
-    return 1
-
   if FLAGS.os_type == benchmark_spec.WINDOWS and not vm_util.RunningOnWindows():
     logging.error('In order to run benchmarks on Windows VMs, you must be '
                   'running on Windows.')
@@ -450,15 +433,19 @@ def RunBenchmarks(publish=True):
 def _GenerateBenchmarkDocumentation():
   """Generates benchmark documentation to show in --help."""
   benchmark_docs = []
-  for benchmark_module in benchmarks.BENCHMARKS:
+  for benchmark_module in (benchmarks.BENCHMARKS +
+                           windows_benchmarks.BENCHMARKS):
     benchmark_info = benchmark_module.BENCHMARK_INFO
     vm_count = benchmark_info.get('num_machines') or 'variable'
     scratch_disk_str = ''
     if benchmark_info.get('scratch_disk'):
       scratch_disk_str = ' with scratch volume'
 
+    name = benchmark_info['name']
+    if benchmark_module in windows_benchmarks.BENCHMARKS:
+      name += ' (Windows)'
     benchmark_docs.append('%s: %s (%s VMs%s)' %
-                          (benchmark_info['name'],
+                          (name,
                            benchmark_info['description'],
                            vm_count,
                            scratch_disk_str))

perfkitbenchmarker/windows_packages/__init__.py

@@ -25,8 +25,13 @@
 
 
 def _LoadPackages():
-  return dict([(module.__name__.split('.')[-1], module) for module in
-               import_util.LoadModulesForPath(__path__, __name__)])
+  """Imports all package modules and returns a dictionary of packages.
+
+  This imports all package modules in this directory and then creates a
+  mapping from module names to the modules themselves and returns it.
+  """
+  return {module.__name__.split('.')[-1]: module for module in
+          import_util.LoadModulesForPath(__path__, __name__)}
 
 
 PACKAGES = _LoadPackages()

perfkitbenchmarker/windows_virtual_machine.py

@@ -25,13 +25,13 @@
 
 FLAGS = flags.FLAGS
 
+DEFAULT_SMB_PORT = 445
+DEFAULT_WINRM_PORT = 5985
 STARTUP_SCRIPT = ('powershell -Command "Enable-PSRemoting -force; '
                   'Set-Item wsman:\\localhost\\client\\trustedhosts * -Force; '
-                  'Restart-Service WinRM; '
-                  'netsh advfirewall firewall add rule name=\'Port 5985\' '
-                  'dir=in action=allow protocol=TCP localport=5985"')
-DEFAULT_WINRM_PORT = 5985
-DEFAULT_SMB_PORT = 445
+                  'Restart-Service WinRM; netsh advfirewall firewall add rule '
+                  'name=\'Port {port}\' dir=in action=allow protocol=TCP '
+                  'localport={port}"').format(port=DEFAULT_WINRM_PORT)
 
 
 class WindowsMixin(virtual_machine.BaseOsMixin):
@@ -105,8 +105,8 @@ def RemoteCopy(self, local_path, remote_path='', copy_to=True):
     Raises:
       RemoteCommandError: If there was a problem copying the file.
     """
-    if ':' in remote_path:
-      remote_path = remote_path.split(':', 1)[1]
+    drive, remote_path = os.path.splitdrive(remote_path)
+    drive = (drive or self.system_drive).rstrip(':')
 
     set_error_pref = '$ErrorActionPreference="Stop"'
 
@@ -117,7 +117,7 @@ def RemoteCopy(self, local_path, remote_path='', copy_to=True):
         '.PSCredential -argumentlist %s,$pw' % (password, self.user_name))
 
     psdrive_name = self.name
-    root = '\\\\%s\\c$' % self.ip_address
+    root = '\\\\%s\\%s$' % (self.ip_address, drive)
     create_psdrive = (
         'New-PSDrive -Name %s -PSProvider filesystem -Root '
         '%s -Credential $cred' % (psdrive_name, root))
@@ -147,15 +147,17 @@ def RemoteCopy(self, local_path, remote_path='', copy_to=True):
   @vm_util.Retry(log_errors=False, poll_interval=1)
   def WaitForBootCompletion(self):
     """Waits until VM is has booted."""
-    resp, _ = self.RemoteCommand('hostname', suppress_warning=True)
+    stdout, _ = self.RemoteCommand('hostname', suppress_warning=True)
     if self.bootable_time is None:
       self.bootable_time = time.time()
     if self.hostname is None:
-      self.hostname = resp[:-1]
+      self.hostname = stdout.rstrip()
 
   def OnStartup(self):
     stdout, _ = self.RemoteCommand('echo $env:TEMP')
     self.temp_dir = os.path.join(stdout.strip(), 'pkb')
+    stdout, _ = self.RemoteCommand('echo $env:SystemDrive')
+    self.system_drive = stdout.strip()
     self.RemoteCommand('mkdir %s' % self.temp_dir)
     self.DisableGuestFirewall()
 
@@ -178,12 +180,13 @@ def Uninstall(self, package_name):
   def PackageCleanup(self):
     """Cleans up all installed packages.
 
-    Deletes the temp directory and uninstalls all PerfKit packages.
+    Deletes the Perfkit Benchmarker temp directory on the VM
+    and uninstalls all PerfKit packages.
     """
     for package_name in self._installed_packages:
       self.Uninstall(package_name)
     self.RemoteCommand('rm -recurse -force %s' % self.temp_dir)
-    self.DisableGuestFirewall()
+    self.EnableGuestFirewall()
 
   def _GetNumCpus(self):
     """Returns the number of logical CPUs on the VM.