etcdserver: change protobuf field type from int to int64 (#12000) #12106

bart0sh · 2020-07-02T10:31:06Z

Cherry-picked the fix for panic in several k/k e2e tests

Fixed CI job failure

See more details in kubernetes/kubernetes#91937

bart0sh · 2020-07-02T11:21:14Z

/cc @wenjiaswe @tbg @YoyinZyc @jingyih @jpbetz

bart0sh · 2020-07-02T20:22:12Z

/assign @gyuho

bart0sh · 2020-07-03T10:37:26Z

/cc @xiang90

bart0sh · 2020-07-06T08:33:25Z

@philips @Gyuh @mitake @mitake @jingyih @jpbetz @spzala @hexfusion @xiang90 @bdarnell @tbg @wenjiaswe Any chance to get this PR reviewed and approved? The underlying issue is causing a lot of test failures in kubernetes e2e tests.

ptabor · 2020-07-06T10:31:47Z

[edited] I see its technically a backport to 3.4 of #12000 already merged into master.

We should have a regression test that shows in which circumstances the error triggers.

bart0sh · 2020-07-06T15:50:35Z

@ptabor

We should have a regression test that shows in which circumstances the error triggers.

That's hard to do as the error can only be triggered with github.com/golang/protobuf >= v1.4.0. etcd uses protobuf v1.3.2 as far as I can see.

With protobuf > 1.4.0 the error can be triggered with this test:

package etcdserverpb

import (
	"testing"
)

func TestLoggablePutRequestStringer(t *testing.T) {
	loggablePutRequest := NewLoggablePutRequest(&PutRequest{})
	expected := "value_size:0 "
	result := loggablePutRequest.String()
	if result != expected {
		t.Errorf("Got result: %s, expected: %s", result, expected)
	}
}

this is how traceback looks like in the release-3.4 branch:

Running tool: /usr/local/go/bin/go test -timeout 30s go.etcd.io/etcd/etcdserver/etcdserverpb -run ^TestLoggablePutRequestStringer$

=== RUN   TestLoggablePutRequestStringer
--- FAIL: TestLoggablePutRequestStringer (0.00s)
panic: invalid Go type int for field go_etcd_io.etcd.etcdserver.etcdserverpb.loggablePutRequest.value_size [recovered]
	panic: invalid Go type int for field go_etcd_io.etcd.etcdserver.etcdserverpb.loggablePutRequest.value_size

goroutine 19 [running]:
testing.tRunner.func1.1(0x9e52c0, 0xc0002da760)
	/usr/local/go/src/testing/testing.go:940 +0x2f5
testing.tRunner.func1(0xc0002f6240)
	/usr/local/go/src/testing/testing.go:943 +0x3f9
panic(0x9e52c0, 0xc0002da760)
	/usr/local/go/src/runtime/panic.go:969 +0x166
google.golang.org/protobuf/internal/impl.newSingularConverter(0xbb9d20, 0x9e4840, 0xbb75e0, 0xc0002b95e0, 0xbb1d01, 0xc000130fe0)
	/home/ed/git/etcd/vendor/google.golang.org/protobuf/internal/impl/convert.go:143 +0xff5
google.golang.org/protobuf/internal/impl.NewConverter(0xbb9d20, 0x9e4840, 0xbb75e0, 0xc0002b95e0, 0x203000, 0x203000)
	/home/ed/git/etcd/vendor/google.golang.org/protobuf/internal/impl/convert.go:60 +0xd3
google.golang.org/protobuf/internal/impl.fieldInfoForScalar(0xbb75e0, 0xc0002b95e0, 0x9dc56d, 0x9, 0x0, 0x0, 0xbb9d20, 0x9e4840, 0x9dc578, 0x2e, ...)
	/home/ed/git/etcd/vendor/google.golang.org/protobuf/internal/impl/message_reflect_field.go:234 +0x119
google.golang.org/protobuf/internal/impl.(*MessageInfo).makeKnownFieldsFunc(0xc0002dc640, 0xffffffffffffffff, 0xffffffffffffffff, 0xffffffffffffffff, 0xffffffffffffffff, 0xc0001f3530, 0xc0001f3560, 0xc0001f3590, 0xc0001f35c0)
	/home/ed/git/etcd/vendor/google.golang.org/protobuf/internal/impl/message_reflect.go:67 +0x928
google.golang.org/protobuf/internal/impl.(*MessageInfo).makeReflectFuncs(0xc0002dc640, 0xbb9d20, 0xa8b2e0, 0xffffffffffffffff, 0xffffffffffffffff, 0xffffffffffffffff, 0xffffffffffffffff, 0xc0001f3530, 0xc0001f3560, 0xc0001f3590, ...)
	/home/ed/git/etcd/vendor/google.golang.org/protobuf/internal/impl/message_reflect.go:36 +0x63
google.golang.org/protobuf/internal/impl.(*MessageInfo).initOnce(0xc0002dc640)
	/home/ed/git/etcd/vendor/google.golang.org/protobuf/internal/impl/message.go:91 +0x182
google.golang.org/protobuf/internal/impl.(*MessageInfo).init(0xc0002dc640)
	/home/ed/git/etcd/vendor/google.golang.org/protobuf/internal/impl/message.go:73 +0x3c
google.golang.org/protobuf/internal/impl.(*messageReflectWrapper).Has(0xc0002da680, 0xbb75e0, 0xc0002b9500, 0xc0002b9500)
	/home/ed/git/etcd/vendor/google.golang.org/protobuf/internal/impl/message_reflect_gen.go:185 +0x33
github.com/golang/protobuf/proto.(*textWriter).writeMessage(0xc0001f3500, 0xbb6480, 0xc0002da680, 0x0, 0x0)
	/home/ed/git/etcd/vendor/github.com/golang/protobuf/proto/text_encode.go:278 +0x91f
github.com/golang/protobuf/proto.(*TextMarshaler).marshal(0xf9f29c, 0xbad480, 0xc0001f34a0, 0xf, 0xf9be08, 0x24, 0x37e, 0x5023b0)
	/home/ed/git/etcd/vendor/github.com/golang/protobuf/proto/text_encode.go:86 +0x18a
github.com/golang/protobuf/proto.(*TextMarshaler).Text(...)
	/home/ed/git/etcd/vendor/github.com/golang/protobuf/proto/text_encode.go:44
github.com/golang/protobuf/proto.CompactTextString(...)
	/home/ed/git/etcd/vendor/github.com/golang/protobuf/proto/text_encode.go:106
go.etcd.io/etcd/etcdserver/etcdserverpb.(*loggablePutRequest).String(...)
	/home/ed/git/etcd/etcdserver/etcdserverpb/raft_internal_stringer.go:182
go.etcd.io/etcd/etcdserver/etcdserverpb.TestLoggablePutRequestStringer(0xc0002f6240)
	/home/ed/git/etcd/etcdserver/etcdserverpb/raft_internal_stringer_test.go:25 +0x107
testing.tRunner(0xc0002f6240, 0xafb810)
	/usr/local/go/src/testing/testing.go:991 +0xdc
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1042 +0x357
FAIL	go.etcd.io/etcd/etcdserver/etcdserverpb	0.014s
FAIL

Any suggestions how to proceed further with this?

ptabor · 2020-07-06T16:10:21Z

Thank you for the test.
I think that bumping protobuf to 1.4 and submitting the tests would be a good idea (at least for the master branch).
I don't have powers to merge, but will try to give it some visibility.

bart0sh · 2020-07-06T16:45:52Z

@ptabor I'll submit the test to the master branch, but we need the fix to go to the release-3.4 branch. We can't use etcd master in Kubernetes go.mod. Is there any chance to do this?

bart0sh · 2020-07-06T16:47:55Z

@ptabor Can you elaborate on "Backport of #12000 already in master down to 3.2#." please?
I've checked release-3.4 branch. The fix is not there yet.

ptabor · 2020-07-06T17:32:51Z

I meant 3.4. Sorry for confusion. Fixed the comment.

bart0sh · 2020-07-06T18:04:20Z

@ptabor but the fix is not in the release-3.4 branch. Should it be there? That's the reason for this PR.

jpbetz · 2020-07-07T18:31:27Z

cc @dims

dims · 2020-07-07T19:47:57Z

thanks @jpbetz looks like needs a rebase LGTM otherwise!

bart0sh · 2020-07-08T07:21:44Z

@dims thank you for the review. rebased.

dims · 2020-07-08T10:37:41Z

/lgtm

liggitt · 2020-07-15T15:11:41Z

lgtm, any blockers for merging this and cutting a v3.4.10?

jingyih · 2020-07-15T17:03:32Z

Just opened #12141 to update the changelog. Could we get it merged and cut 3.4.10? @gyuho @jpbetz

spzala · 2020-07-16T23:51:16Z

3.4.10 is released just now! Thanks @gyuho

…106-upstream-release-3.3 Automated cherry pick of #12106

ref. #12106 Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

etcdserver: change protobuf field type from int to int64 (#12000)

ed28c76

bart0sh mentioned this pull request Jul 8, 2020

Add test case for "invalid Go type int" panic #12116

Merged

liggitt mentioned this pull request Jul 15, 2020

kubernetes unit tests fail with Panic messages stating "invalid type" kubernetes/kubernetes#92070

Closed

jingyih merged commit 7b82704 into etcd-io:release-3.4 Jul 15, 2020

jingyih mentioned this pull request Jul 15, 2020

CHANGELOG: update from 12000 #12141

Merged

bart0sh mentioned this pull request Jul 15, 2020

go.mod: update etcd to fix e2e tests kubernetes/kubernetes#93118

Merged

jingyih mentioned this pull request Jul 16, 2020

Automated cherry pick of #12000 #12021

Closed

xiang90 mentioned this pull request Jul 20, 2020

crash when acquiring lock in 3.3.23 #12156

Closed

wenjiaswe mentioned this pull request Aug 13, 2020

Automated cherry pick of #12106 #12215

Merged

gyuho added a commit that referenced this pull request Aug 14, 2020

Merge pull request #12215 from wenjiaswe/automated-cherry-pick-of-#12…

f78bdce

…106-upstream-release-3.3 Automated cherry pick of #12106

gyuho added a commit that referenced this pull request Aug 18, 2020

etcdserver/etcdserverpb: change protobuf field type from int to int64

7619b2f

ref. #12106 Signed-off-by: Gyuho Lee <leegyuho@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcdserver: change protobuf field type from int to int64 (#12000) #12106

etcdserver: change protobuf field type from int to int64 (#12000) #12106

bart0sh commented Jul 2, 2020 •

edited

Loading

bart0sh commented Jul 2, 2020

bart0sh commented Jul 2, 2020

bart0sh commented Jul 3, 2020

bart0sh commented Jul 6, 2020

ptabor commented Jul 6, 2020 •

edited

Loading

bart0sh commented Jul 6, 2020 •

edited

Loading

ptabor commented Jul 6, 2020

bart0sh commented Jul 6, 2020

bart0sh commented Jul 6, 2020

ptabor commented Jul 6, 2020

bart0sh commented Jul 6, 2020

jpbetz commented Jul 7, 2020

dims commented Jul 7, 2020

bart0sh commented Jul 8, 2020

dims commented Jul 8, 2020

liggitt commented Jul 15, 2020

jingyih commented Jul 15, 2020

spzala commented Jul 16, 2020

etcdserver: change protobuf field type from int to int64 (#12000) #12106

etcdserver: change protobuf field type from int to int64 (#12000) #12106

Conversation

bart0sh commented Jul 2, 2020 • edited Loading

bart0sh commented Jul 2, 2020

bart0sh commented Jul 2, 2020

bart0sh commented Jul 3, 2020

bart0sh commented Jul 6, 2020

ptabor commented Jul 6, 2020 • edited Loading

bart0sh commented Jul 6, 2020 • edited Loading

ptabor commented Jul 6, 2020

bart0sh commented Jul 6, 2020

bart0sh commented Jul 6, 2020

ptabor commented Jul 6, 2020

bart0sh commented Jul 6, 2020

jpbetz commented Jul 7, 2020

dims commented Jul 7, 2020

bart0sh commented Jul 8, 2020

dims commented Jul 8, 2020

liggitt commented Jul 15, 2020

jingyih commented Jul 15, 2020

spzala commented Jul 16, 2020

bart0sh commented Jul 2, 2020 •

edited

Loading

ptabor commented Jul 6, 2020 •

edited

Loading

bart0sh commented Jul 6, 2020 •

edited

Loading