Skip to content
This repository has been archived by the owner on May 6, 2020. It is now read-only.

Refine metrics CI invocations #909

Merged
merged 3 commits into from
Feb 21, 2018

Conversation

grahamwhaley
Copy link
Contributor

Refine our metrics CI invocations somewhat:

  • stop running tests that we don't currently check the results for
  • refine the iperf3 test run to only test what we do check
    • which involved adding a commandline to the iperf3 test so we could tweak it

@grahamwhaley
Copy link
Contributor Author

Marked as WIP, as I realised I need to update the README.md.

I've tweaked the jenkins metrics CI machines so they should be compatible with these changes - I guess we'll see...

@clearcontainersbot
Copy link

kubernetes qa-failed 👎

@grahamwhaley grahamwhaley force-pushed the 20180214_iperf3_cmdline branch from fa627d6 to 36945c8 Compare February 14, 2018 15:50
@grahamwhaley
Copy link
Contributor Author

A quick note on the README update - yes, the two spaces added to the end of a line in the bullet list are deliberate - they force a line break, and hence a new paragraph, within that bullet.
Dropping the WIP now...

@grahamwhaley grahamwhaley removed the WIP label Feb 14, 2018
@clearcontainersbot
Copy link

kubernetes qa-passed 👍

Copy link
Contributor

@GabyCT GabyCT left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -125,18 +125,8 @@ pushd "$CURRENTDIR/../metrics"
# ops/second
bash network/network-nginx-ab-benchmark.sh

# ping latency
bash network/network-latency.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that we still run these tests in metrics/run_all_metrics.sh, but I'm now wondering if we shouldn't just have a single parameterised script to handle all metrics tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as in a single script where we can say 'script --all' or 'script --kpis' for instance, to remove the duplication between the two scripts?

We could do that. In my mind we'd re-factor the run_all_metrics (well, and change the name). We'd have to add the KSM handling and some other trickery from the run_metrics_ci script as well (like the results storing options as well).

How about I open an Issue to note we could do with a refactor of the scripts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks - Sounds good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #912.

@devimc
Copy link

devimc commented Feb 19, 2018

lgtm

Approved with PullApprove Approved with PullApprove

@jodh-intel
Copy link
Contributor

jodh-intel commented Feb 19, 2018

lgtm

@klynnrif - could you take a look please?

Approved with PullApprove

Copy link

@klynnrif klynnrif left a comment

Choose a reason for hiding this comment

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

A couple small changes to keep an active voice. Thanks!

@@ -35,7 +35,9 @@ and packet-per-second throughput using iperf3 on single threaded connections.
The bandwidth test shows the speed of the data transfer. On the other hand,
the jitter test measures the variation in the delay of received packets.
The packet-per-second tests show the maximum number of (smallest sized) packets
we can get through the transports.
we can get through the transports.

Choose a reason for hiding this comment

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

allowed through the transports.

@@ -35,7 +35,9 @@ and packet-per-second throughput using iperf3 on single threaded connections.
The bandwidth test shows the speed of the data transfer. On the other hand,
the jitter test measures the variation in the delay of received packets.
The packet-per-second tests show the maximum number of (smallest sized) packets
we can get through the transports.
we can get through the transports.
Command line options are used to choose which tests to run, for how long, and

Choose a reason for hiding this comment

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

Command-line options choose what tests to run, for how long, and...

@grahamwhaley grahamwhaley force-pushed the 20180214_iperf3_cmdline branch from 36945c8 to 19dbee5 Compare February 20, 2018 10:48
@grahamwhaley
Copy link
Contributor Author

@klynnrif updated, thanks.

@clearcontainersbot
Copy link

kubernetes qa-passed 👍

@grahamwhaley
Copy link
Contributor Author

hmm, looks like something went wonky with my rebase - let me fix and push that again..

Graham whaley added 3 commits February 20, 2018 14:41
The iperf3 test always ran all the tests, with a fixed
iteration (1) and duration (5s). Add a command line parse
to allow us to choose:
 - which tests to run
 - how many times to run them
 - for how long to run each one

This will help us:
 - reduce running tests we don't actually need
 - improve stability by defining test duration and iterations

Fixes: clearcontainers#901

Signed-off-by: Graham whaley <[email protected]>
Rather than run all the iperf3 tests, and then only use
one result, refine the invocation with the new cmdline
options to run just the one test, but multiple times to
reduce variation in the results.

Signed-off-by: Graham whaley <[email protected]>
We are running some tests, but not currently using their
results as part of the CI check as they are somewhat
noisy and need some study and refinement.
Remove them from the run for now.

Signed-off-by: Graham whaley <[email protected]>
@grahamwhaley grahamwhaley force-pushed the 20180214_iperf3_cmdline branch from 19dbee5 to d54b008 Compare February 20, 2018 14:44
@grahamwhaley
Copy link
Contributor Author

fixed

@clearcontainersbot
Copy link

kubernetes qa-passed 👍

@devimc devimc merged commit 386c031 into clearcontainers:master Feb 21, 2018
mcastelino pushed a commit to mcastelino/tests that referenced this pull request Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants