Storage flag #1360

sspano · 2017-05-09T23:33:31Z

@stfeng @gareth-ferneyhough
Adapt mysql_service_benchmark to handle storage size flag. Remove conflicting comments.

FYI @gareth-ferneyhough I did not change RDS side of this other than replacing the hard coded storage size with the flag value. This means that volume size is 1 so IOPS is capped will be capped by this.

ghost · 2017-05-10T16:26:29Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

@@ -22,6 +22,25 @@

 As other cloud providers deliver a managed MySQL service, we will add it here.

+To make this benchmark run for GCP:
+To run this benchmark for GCP if is required to install a non-default gcloud


typo: "if"

specify reasons why it's required to install a non-default gcloud component (because ...), also specify the time/date of this requirement ("As of May 2017")

I see the reasons are specified below, properly better to pull that up to here for clarity.

Handled in new commit.

ghost · 2017-05-10T16:28:38Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

@@ -61,6 +80,7 @@
 flags.DEFINE_integer('sysbench_report_interval', 2,
                     'The interval, in seconds, we ask sysbench to report '
                     'results.')
+flags.DEFINE_integer('storage_size', 3400, 'Storage size for SQL instance.')


specify the units for storage size, e.g., in GB

Also, does the default has to be this large? It has cost consequence. Probably we should specify a smaller default for those who just want to try out the benchmarks. 100?

Handled in new commit.

ghost · 2017-05-10T16:33:56Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

@@ -668,13 +680,23 @@ def Prepare(self, vm):
    logging.info('Preparing MySQL Service benchmarks for Google Cloud SQL.')

    vm.db_instance_name = 'pkb%s' % FLAGS.run_uri
+    storage_size = self._validate_size(FLAGS.storage_size)


Can we have a simple validation for RDS too? (for completeness).

Handled in new commit.

ghost · 2017-05-10T16:34:12Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

+
+    GCP supports storage sizes from 0 to 64TB currently.
+    """
+    if size < 0 or size > 64000:


0 is not supported either.

Handled in new commit.

ghost · 2017-05-10T16:35:29Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

+    """
+    if size < 0 or size > 64000:
+      size = 3334
+    return size


If size is invalid we should throw an exception and fail the benchmark run. We can't silently change user's input to a default value....

Handled in new commit.

ghost

looks pretty good as first iteration. There are a few minor comments, please take a look.

gareth-ferneyhough · 2017-05-10T18:09:37Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

@@ -558,6 +572,21 @@ def Prepare(self, vm):
    logging.info('Successfully created an RDS DB instance. Address is %s',
                 vm.db_instance_address)
    logging.info('Complete output is:\n %s', response)
+
+  def _validate_size(self, size):


For some reason, PKB's style is CamelCase for function names, so please change it to be consistent with the rest of the file. You can leave the underscore.

Fixed in new commit.

gareth-ferneyhough · 2017-05-10T18:12:48Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

@@ -558,6 +572,21 @@ def Prepare(self, vm):
    logging.info('Successfully created an RDS DB instance. Address is %s',
                 vm.db_instance_address)
    logging.info('Complete output is:\n %s', response)
+
+  def _validate_size(self, size):
+    """Validates flag for storage size, throws exception if invalid.


Change to "Validate flag for storage size and throw exception if invalid." The style guide requests that summary doc strings are written as a command, not a description. Also, move "AWS supports storage sizes from 1GB to 16TB currently." Up so that it is one line below the summary and above the args and returns.

Fixed in new commit.

ghost · 2017-05-10T19:39:20Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

@@ -22,11 +22,31 @@

 As other cloud providers deliver a managed MySQL service, we will add it here.

+As of May 2017 to make this benchmark run for GCP you must install the
+gcloud beta component. This is necessary because creating a SQL instance with


s/SQL/"Cloud SQL"/

Fixed in new commit.

ghost · 2017-05-10T19:43:38Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

+    AWS supports storage sizes from 1GB to 16TB currently.
+    """
+    if size < 1 or size > 16000:
+      sys.exit('Storage size flag given is not valid. Must be between '


instead of doing sys.exit() can you throw an exception instead? (follow the example of error handling logic in other places of this code).

By throwing exception here, the parent classes of PKB may decide to catch those and initiate a cleanup if necessary, however, doing sys.exit() would void all that possibility.

(As FYI: Currently PKB doesn't usually do cleanup upon benchmark failure, so if a user sees an exception here, they will need to call pkb.py --run_stage=cleanup --run_uri == this run uri, but the key here by throwing an exception up from here, we all the possibility to implement cleanup at the parent class in the future).

Fixed in new commit.

ghost · 2017-05-10T19:44:08Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

+    GCP supports storage sizes from 1GB to 64TB currently.
+    """
+    if size < 1 or size > 64000:
+      sys.exit('Storage size flag given is not valid. Must be between '


same as above.

Fixed in new commit.

ghost · 2017-05-10T19:48:47Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

@@ -440,6 +454,7 @@ def Prepare(self, vm):

    # Get a list of zones and pick one that's different from the zone VM is in.
    new_subnet_zone = None
+    storage_size = self._ValidateSize(FLAGS.storage_size)


It doesn't seem necessary to return anything from the validatesize() function, if it never changes the size.

ghost · 2017-05-10T19:49:08Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

+      size: (GB).
+    """
+    if size < 1 or size > 16000:
+      sys.exit('Storage size flag given is not valid. Must be between '


change sys.exit() to exception.

Fixed in new commit.

ghost

more comments, getting close....

gareth-ferneyhough · 2017-05-10T20:30:49Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

@@ -751,6 +791,18 @@ def Prepare(self, vm):
    logging.info('Set root password completed. Stdout:\n%s\nStderr:\n%s',
                 stdout, stderr)

+  def _ValidateSize(self, size):
+    """Validate flag for storage size and throw exception if invalid


add "." to the end. The google linter complains if there is no punctuation at the end of the summary.

Fixed in new commit.

ghost · 2017-05-10T21:16:18Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

@@ -440,6 +458,8 @@ def Prepare(self, vm):

    # Get a list of zones and pick one that's different from the zone VM is in.
    new_subnet_zone = None
+    storage_size = FLAGS.storage_size


local variable storage_size is unnecessary, you can just use FLAGS.storage_size here and below.

Same comment for GCP.

Fixed in new commit.

ghost · 2017-05-10T21:17:13Z

perfkitbenchmarker/linux_benchmarks/mysql_service_benchmark.py

+      size: (GB).
+    """
+    if size < 1 or size > 64000:
+      sys.exit('Storage size flag given is not valid. Must be between '


forgot this one? (sys.exit)

Yes haha. Fixed in new commit.

ghost

there is a travis build error with your most recent commit, please fix it.

Other than that, the rest of the code looks good.

ghost · 2017-05-10T23:00:22Z

LGTM from me, please wait for LGTM from Gareth as well. Once you have that, please do an end-to-end test with the final code, reply back when that is done.

sspano · 2017-05-11T22:37:55Z

End to end testing with valid and invalid cases, acts as expected.

googlebot added the cla: yes label May 9, 2017

ghost reviewed May 10, 2017

View reviewed changes

ghost suggested changes May 10, 2017

View reviewed changes

gareth-ferneyhough suggested changes May 10, 2017

View reviewed changes

ghost reviewed May 10, 2017

View reviewed changes

gareth-ferneyhough reviewed May 10, 2017

View reviewed changes

ghost reviewed May 10, 2017

View reviewed changes

mysql_service_benchmark handles storage size flag

19241f4

sspano force-pushed the storage_flag branch from 56c33b4 to 19241f4 Compare May 10, 2017 22:50

ghost approved these changes May 10, 2017

View reviewed changes

gareth-ferneyhough approved these changes May 11, 2017

View reviewed changes

gareth-ferneyhough merged commit 8430252 into GoogleCloudPlatform:master May 11, 2017

sspano deleted the storage_flag branch May 11, 2017 22:42

Storage flag #1360

Storage flag #1360

Conversation

sspano commented May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost May 10, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost commented May 10, 2017

sspano commented May 11, 2017

sspano commented May 9, 2017 •

edited

Loading

ghost May 10, 2017 •

edited by ghost

Loading