fix newline in raw string in e2e net perf case #46581

m1093782566 · 2017-05-28T15:07:08Z

Which issue this PR fixes

k8s-ci-robot · 2017-05-28T15:07:16Z

Hi @m1093782566. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

spxtr · 2017-05-28T15:21:36Z

fixes #46581

That's this PR!

m1093782566 · 2017-05-28T16:04:31Z

@spxtr updated.

spxtr · 2017-05-28T18:26:30Z

/unassign
/assign @cmluciano

cblecker · 2017-05-29T17:12:01Z

@k8s-bot ok to test
/release-note-none

m1093782566 · 2017-05-30T03:14:50Z

@cblecker test flakes?

gyliu513 · 2017-05-30T03:18:09Z

@k8s-bot pull-kubernetes-e2e-kops-aws test this

m1093782566 · 2017-05-30T03:25:14Z

ha ha. It seems that k8s-bot does not response to me. THANKS, @gyliu513

gyliu513 · 2017-05-30T03:35:27Z

Yes, for now only Member can run @k8s-bot ok to test, but you can run the command that the @k8s-ci-robot provided to re-run the test. ;-)

cblecker · 2017-05-30T06:10:06Z

@m1093782566 The federation test is flaky atm, but also non-blocking. We can leave that failed -- it won't block your PR from merging. Next step is waiting for one of the reviewers to look over your change.

m1093782566 · 2017-05-30T07:09:16Z

@cblecker Thanks for reminding.

cmluciano

I'm not sure if this actually would not still generate an error.

I tried an example in the Go playground here

m1093782566 · 2017-06-01T01:35:15Z

@cmluciano

Please check https://play.golang.org/p/XuelZdR6a9

cmluciano · 2017-06-01T17:48:54Z

Yep that example works since you convert prior to parsing. I should also ask what platform you are running these tests on and can you provide a test run that fails.

m1093782566 · 2017-06-02T03:04:15Z

@cmluciano My environment is centos 7.2 kubeadm cluster.

It's a bit difficult to create a failure case in CI environment which I think it's a debain OS. However, I guess trim newline in a string does no harm?

cmluciano · 2017-06-02T19:53:31Z

The tests use a specific library that makes this a little easier. I think just a unit test as you mentioned should suffice.

m1093782566 · 2017-06-03T02:59:23Z

@cmluciano Do you mean I should unit test intOrFail()?

I just don't know how to write unit test cases in e2e framework and how to make e2e run unit test cases. Could you please give me some hints? Thanks very much.

cmluciano · 2017-06-05T15:59:25Z

Sorry, I just realized that this actually WAS the e2e. We could try to stub that int in another case, but it probably is not worthwhile.

Let's just re-arrange the PR to strip the newline prior to parsing for now and then see what the reviewers think of another test case.

cmluciano · 2017-07-10T14:34:25Z

/retest

m1093782566 · 2017-08-12T05:31:17Z

@sstarcher Please look at here. Did you meet the same issue in AWS/CoreOS setup?

m1093782566 · 2017-08-12T05:34:11Z

@cmluciano

Sorry, but to be honest, I don't very understand your expectation - I can reproduce this issue in my local centos setup, and @sstarcher saw this issue in his AWS/CoreOS setup. I just don't know much about why it does not occur in k8s CI environment. Should we keep this PR open? Would much appreciate if you can give any idea. Thanks!

m1093782566 · 2017-08-12T05:44:16Z

@bowei

Would appreciate if you can give your opinions. Thanks a lot :)

sstarcher · 2017-08-12T10:49:34Z

I was using the https://github.com/coreos/tectonic-installer at the time when I reported I saw that issue running the conformance tests. It was installed in AWS using coreos.

cmluciano · 2017-10-23T20:25:02Z

/retest

bowei · 2017-10-24T21:25:22Z

test/e2e/network/util_iperf.go

@@ -97,6 +97,8 @@ func (s StrSlice) get(i int) string {

 // intOrFail is a convenience function for parsing integers.
 func intOrFail(debugName string, rawValue string) int64 {
+	// Trim newline in raw string before parsing it
+	rawValue = strings.Replace(rawValue, "\n", "", -1)


This doesn't seem great to me. We should be Trim()ing the string before handing to this function. Also, this function makes

"1\n2\n3\n5" become a valid integer.

Fixed. PTAL. Thanks!

bowei

I would fix the calling places where the newline is being introduced.

m1093782566 · 2017-10-25T02:06:11Z

Glad to receive your feedback, @bowei

I fixed the newline in the beginning of NewIPerf(), please take a look when you have a chance. Thanks!

bowei · 2017-10-25T05:00:24Z

test/e2e/network/util_iperf.go

@@ -69,6 +69,8 @@ func (i *IPerfResults) ToTSV() string {

 // NewIPerf parses an IPerf CSV output line into an IPerfResult.
 func NewIPerf(csvLine string) *IPerfResult {
+	// Trim newline in csv string line before parsing it


This comment is not needed.

Okay, removed this comment.

fixed

m1093782566 · 2017-11-09T09:19:56Z

@bowei @cmluciano

All comments are fixed now and this PR has been pending for a while...

bowei · 2017-11-09T18:55:44Z

/lgtm

bowei · 2017-11-09T18:55:54Z

/approve no-issue

k8s-github-robot · 2017-11-09T18:58:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, m1093782566

Associated issue: 46083

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~test/e2e/network/OWNERS~~ [bowei]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-10T11:30:14Z

Automatic merge from submit-queue (batch tested with PRs 46581, 55426, 54849). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 28, 2017

k8s-github-robot assigned gmarek and spxtr May 28, 2017

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels May 28, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 28, 2017

k8s-ci-robot assigned cmluciano and unassigned spxtr May 28, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 29, 2017

cmluciano previously requested changes May 31, 2017

View reviewed changes

gmarek assigned bowei Jun 12, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2017

m1093782566 closed this Aug 4, 2017

m1093782566 reopened this Aug 12, 2017

m1093782566 force-pushed the fix-net-perf branch from 98c450b to 117260f Compare August 12, 2017 05:24

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2017

dujun1990 mentioned this pull request Aug 12, 2017

[e2e test] Networking IPerf failed due to invalid bandwidth string with newline #46083

Closed

m1093782566 force-pushed the fix-net-perf branch from 117260f to 701744d Compare August 12, 2017 05:42

m1093782566 force-pushed the fix-net-perf branch from 701744d to f0a39fb Compare September 18, 2017 02:50

bowei reviewed Oct 24, 2017

View reviewed changes

bowei suggested changes Oct 24, 2017

View reviewed changes

m1093782566 force-pushed the fix-net-perf branch from f0a39fb to 66fee82 Compare October 25, 2017 02:04

bowei reviewed Oct 25, 2017

View reviewed changes

fix newline in raw string in e2e net perf case

7ffd5b2

m1093782566 force-pushed the fix-net-perf branch from 66fee82 to 7ffd5b2 Compare October 25, 2017 05:27

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2017

k8s-github-robot merged commit c7644dd into kubernetes:master Nov 10, 2017

fix newline in raw string in e2e net perf case #46581

fix newline in raw string in e2e net perf case #46581

Conversation

m1093782566 commented May 28, 2017 • edited

k8s-ci-robot commented May 28, 2017

spxtr commented May 28, 2017

m1093782566 commented May 28, 2017

spxtr commented May 28, 2017

cblecker commented May 29, 2017

m1093782566 commented May 30, 2017

gyliu513 commented May 30, 2017

m1093782566 commented May 30, 2017 • edited

gyliu513 commented May 30, 2017

cblecker commented May 30, 2017

m1093782566 commented May 30, 2017

cmluciano left a comment

Choose a reason for hiding this comment

m1093782566 commented Jun 1, 2017

cmluciano commented Jun 1, 2017

m1093782566 commented Jun 2, 2017 • edited

cmluciano commented Jun 2, 2017

m1093782566 commented Jun 3, 2017 • edited

cmluciano commented Jun 5, 2017

cmluciano commented Jul 10, 2017

m1093782566 commented Aug 12, 2017

m1093782566 commented Aug 12, 2017 • edited

m1093782566 commented Aug 12, 2017

sstarcher commented Aug 12, 2017

cmluciano commented Oct 23, 2017

bowei Oct 24, 2017

Choose a reason for hiding this comment

m1093782566 Oct 25, 2017

Choose a reason for hiding this comment

bowei left a comment

Choose a reason for hiding this comment

m1093782566 commented Oct 25, 2017 • edited

bowei Oct 25, 2017

Choose a reason for hiding this comment

m1093782566 Oct 25, 2017

Choose a reason for hiding this comment

m1093782566 commented Nov 9, 2017

bowei commented Nov 9, 2017

bowei commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

k8s-github-robot commented Nov 10, 2017

m1093782566 commented May 28, 2017 •

edited

m1093782566 commented May 30, 2017 •

edited

m1093782566 commented Jun 2, 2017 •

edited

m1093782566 commented Jun 3, 2017 •

edited

m1093782566 commented Aug 12, 2017 •

edited

m1093782566 commented Oct 25, 2017 •

edited