use Reader.ReadLine instead of bufio.Scanner to support bigger yaml #23265

adohe-zz · 2016-03-21T03:42:07Z

@smarterclayton ptal. Also refer #19603 #23125 for more details.

This change is

smarterclayton · 2016-03-21T16:10:48Z

pkg/util/yaml/decoder_test.go

@@ -25,42 +24,6 @@ import (
 	"testing"
 )

-func TestSplitYAMLDocument(t *testing.T) {


I would expect this test case to be preserved and still pass.

If you need to move the test cases or reduce their complexity that's fine, but we should be testing all of these edge cases still.

@smarterclayton ok will add this back.

k8s-github-robot · 2016-03-31T10:40:50Z

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

smarterclayton · 2016-04-16T18:02:19Z

pkg/util/yaml/decoder_test.go

 func TestDecodeYAML(t *testing.T) {
 	s := NewYAMLToJSONDecoder(bytes.NewReader([]byte(`---
 stuff: 1

---       


Add new test case with lines > 64kb

Maybe https://golang.org/pkg/bytes/#Repeat can help

smarterclayton · 2016-06-06T23:14:31Z

If you can add this test we can probably get it into 1.3 (since it blocks users with long fields like secrets and config maps)

SleepyBrett · 2016-08-11T20:02:38Z

What's the status here? Just ran into this problem myself.

adohe-zz · 2016-08-12T02:30:05Z

@SleepyBrett generally it should be ok, tests are needed.

adohe-zz · 2016-08-15T07:06:24Z

@smarterclayton I just update this, ptal.

k8s-bot · 2016-08-15T07:43:45Z

GCE e2e build/test passed for commit 2d06408.

smarterclayton · 2016-08-15T17:00:05Z

pkg/util/yaml/decoder.go

+
+// Read returns a full YAML document.
+func (r *YAMLReader) Read() ([]byte, error) {
+	var buffer bytes.Buffer


Hrm - this implementation requires us to load the whole object into memory, rather than being line driven as before. I'd prefer if we didn't require that - we could have very large config objects on disk that this would then cause issues on.

@smarterclayton I am sorry I don't get the point. I think in the old scanner based approach, we still need to read a full yaml document into buffer before we decode it.

That's right, because we don't have a scanning parser. Ok, withdrawn.

k8s-github-robot · 2016-08-17T08:35:59Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-08-17T09:26:20Z

GCE e2e build/test passed for commit 2d06408.

k8s-github-robot · 2016-08-17T09:26:46Z

Automatic merge from submit-queue

gmarek · 2016-08-17T14:39:14Z

This most likely broke kubernetes-test-go. Reverting.

ncdc · 2016-08-17T14:40:41Z

I have a fix

ncdc · 2016-08-17T14:41:37Z

The fix is #30772

ncdc · 2016-08-17T14:49:45Z

@pwittrock @ixdy @lavalamp the "Jenkins unit/integration" job for this PR reported SUCCESS but it in fact had a test "failure" (see https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/23265/kubernetes-pull-test-unit-integration/39955/). This same "failure" was detected by the subsequent kubernetes-test-go job that ran after this PR was merged to master. The reason I put failure in quotes is because the test actually passed, but due to a missing newline in some test output, the junit processor thought the test had actually failed.

Do you all know why the unit/int job reported success here? We need to fix that.

gmarek · 2016-08-17T14:52:13Z

cc @fejta

adohe-zz · 2016-08-17T23:51:14Z

@ncdc thanks for help fix this, the jenkins unit/int job behavior is a little bit strange.

ixdy · 2016-08-18T22:42:11Z

Yeah, the unit test passed, but go-junit-report parses the verbose test output, and since the newline was missing, it didn't think the test case passed.

We explicitly made PR Jenkins not fail on any junit failures in kubernetes/test-infra#246. I don't remember why now (though it links to #27898). CI Jenkins still fails on junit failures.

cc @lavalamp to see if he remembers.

ncdc · 2016-08-18T22:46:40Z

Seems like PR Jenkins should fail on any junit failures.

On Thu, Aug 18, 2016 at 6:43 PM, Jeff Grafton notifications@github.com
wrote:

Yeah, the unit test passed, but go-junit-report
https://github.com/jstemmer/go-junit-report parses the verbose test
output, and since the newline was missing, it didn't think the test case
passed.

We explicitly made PR Jenkins not fail on any junit failures in
kubernetes/test-infra#246
kubernetes/test-infra#246. I don't remember why
now (though it links to #27898
#27898). CI Jenkins still
fails on junit failures.

cc @lavalamp https://github.com/lavalamp to see if he remembers.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#23265 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAABYlvHU-NbWIA4LVhREhKGEmAQv0Jvks5qhN_3gaJpZM4H03KM
.

lavalamp · 2016-08-18T22:49:02Z

@ixdy you're saying the junit file incorrectly records a failure, when the test actually passed?

We rely on the test job to report success/failure, and not on the junit. This lets us, in ginkgo, report success but still record a failure (due to a flake) in the junit.

lavalamp · 2016-08-18T22:49:39Z

Post-submit tests detect flakes in a more reliable way so they behave differently.

ixdy · 2016-08-18T22:51:49Z

I think the complaint here is that PR and CI Jenkins handle junit differently. Should we make CI Jenkins ignore junit? (I'm guessing the answer here is yes.)

lavalamp · 2016-08-18T22:57:28Z

I dunno. I think this is WAI and issues like this should be rare.

ncdc · 2016-08-18T23:03:17Z

Wouldn't you want to avoid a PR merging that immediately turns
kubernetes-test-go red and blocks the submit queue? That's what happened
here. And it took several hours to resolve.

On Thursday, August 18, 2016, Daniel Smith notifications@github.com wrote:

I dunno. I think this is WAI and issues like this should be rare.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#23265 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAABYvdPpPhfL5SecU284iTn1TuLO_kEks5qhOONgaJpZM4H03KM
.

lavalamp · 2016-08-19T00:45:32Z

Well, when you put it like that...

Yes, I'm in favor of them being the same, as long as you leave the flake finding optimization in place. (that is, if the test section says failures="0" then it's not a suite failure even if an individual test failure is present)

googlebot added the cla: yes label Mar 21, 2016

This was referenced Mar 21, 2016

Cannot create secrets from big (~6MiB) YAML file #23125

Closed

Stop using bufio.Scanner in kubernetes/pkg/util/yaml #19603

Closed

k8s-github-robot assigned thockin Mar 21, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 21, 2016

smarterclayton reviewed Mar 21, 2016
View reviewed changes

k8s-github-robot added needs-ok-to-merge and removed needs-ok-to-merge labels Mar 31, 2016

thockin assigned smarterclayton and unassigned thockin Apr 16, 2016

smarterclayton reviewed Apr 16, 2016
View reviewed changes

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

dims mentioned this pull request Jun 8, 2016

Fix "bufio.Scanner: token too long" failures #26992

Closed

onorua mentioned this pull request Jun 26, 2016

yaml parser fails with "bufio.Scanner: token too long" #12582

Closed

use Reader.ReadLine instead of bufio.Scanner to support bigger yaml

2d06408

adohe-zz force-pushed the big_yaml branch from 9f2e273 to 2d06408 Compare August 15, 2016 07:04

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

smarterclayton reviewed Aug 15, 2016
View reviewed changes

smarterclayton added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 16, 2016

k8s-github-robot merged commit 7b49d0c into kubernetes:master Aug 17, 2016

adohe-zz deleted the big_yaml branch August 17, 2016 10:36

gmarek mentioned this pull request Aug 17, 2016

Revert "use Reader.ReadLine instead of bufio.Scanner to support bigger yaml" #30771

Closed

adohe-zz mentioned this pull request Aug 18, 2016

Large Environmental Variables crash pods #30827

Closed

jim-minter mentioned this pull request Sep 21, 2016

"bufio.Scanner: token too long" when reading large template with openshift client openshift/origin#9007

Closed

smarterclayton mentioned this pull request Sep 27, 2016

Kubectl 1.4 does not trim --- seperator #33588

Closed

t3hmrman mentioned this pull request Dec 8, 2021

Having many mounts causes logging failure which cascades into kubelet reboot loop google/cadvisor#3025

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use Reader.ReadLine instead of bufio.Scanner to support bigger yaml #23265

use Reader.ReadLine instead of bufio.Scanner to support bigger yaml #23265

adohe-zz commented Mar 21, 2016 •

edited by k8s-oncall

Loading

smarterclayton Mar 21, 2016

smarterclayton Mar 21, 2016

adohe-zz Mar 21, 2016

k8s-github-robot commented Mar 31, 2016

smarterclayton Apr 16, 2016

0xmichalis Jun 9, 2016

smarterclayton commented Jun 6, 2016

SleepyBrett commented Aug 11, 2016

adohe-zz commented Aug 12, 2016

adohe-zz commented Aug 15, 2016

k8s-bot commented Aug 15, 2016

smarterclayton Aug 15, 2016

adohe-zz Aug 16, 2016

smarterclayton Aug 16, 2016

k8s-github-robot commented Aug 17, 2016

k8s-bot commented Aug 17, 2016

k8s-github-robot commented Aug 17, 2016

gmarek commented Aug 17, 2016

ncdc commented Aug 17, 2016

ncdc commented Aug 17, 2016

ncdc commented Aug 17, 2016

gmarek commented Aug 17, 2016

adohe-zz commented Aug 17, 2016

ixdy commented Aug 18, 2016

ncdc commented Aug 18, 2016

lavalamp commented Aug 18, 2016

lavalamp commented Aug 18, 2016

ixdy commented Aug 18, 2016

lavalamp commented Aug 18, 2016

ncdc commented Aug 18, 2016

lavalamp commented Aug 19, 2016

use Reader.ReadLine instead of bufio.Scanner to support bigger yaml #23265

use Reader.ReadLine instead of bufio.Scanner to support bigger yaml #23265

Conversation

adohe-zz commented Mar 21, 2016 • edited by k8s-oncall Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Mar 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jun 6, 2016

SleepyBrett commented Aug 11, 2016

adohe-zz commented Aug 12, 2016

adohe-zz commented Aug 15, 2016

k8s-bot commented Aug 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Aug 17, 2016

k8s-bot commented Aug 17, 2016

k8s-github-robot commented Aug 17, 2016

gmarek commented Aug 17, 2016

ncdc commented Aug 17, 2016

ncdc commented Aug 17, 2016

ncdc commented Aug 17, 2016

gmarek commented Aug 17, 2016

adohe-zz commented Aug 17, 2016

ixdy commented Aug 18, 2016

ncdc commented Aug 18, 2016

lavalamp commented Aug 18, 2016

lavalamp commented Aug 18, 2016

ixdy commented Aug 18, 2016

lavalamp commented Aug 18, 2016

ncdc commented Aug 18, 2016

lavalamp commented Aug 19, 2016

adohe-zz commented Mar 21, 2016 •

edited by k8s-oncall

Loading