Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jul 15, 2020
Merged

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

merged 1 commit into from
Jul 15, 2020

Conversation

bart0sh
Copy link
Contributor

@bart0sh bart0sh commented Jul 2, 2020

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

Fixed CI job failure

See more details in kubernetes/kubernetes#91937

@bart0sh
Copy link
Contributor Author

bart0sh commented Jul 2, 2020

@bart0sh
Copy link
Contributor Author

bart0sh commented Jul 2, 2020

/assign @gyuho

@bart0sh
Copy link
Contributor Author

bart0sh commented Jul 3, 2020

/cc @xiang90

@bart0sh
Copy link
Contributor Author

bart0sh commented Jul 6, 2020

@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
Copy link
Contributor

ptabor commented Jul 6, 2020

[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
Copy link
Contributor Author

bart0sh commented Jul 6, 2020

@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
Copy link
Contributor

ptabor commented Jul 6, 2020

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
Copy link
Contributor Author

bart0sh commented Jul 6, 2020

@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
Copy link
Contributor Author

bart0sh commented Jul 6, 2020

@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
Copy link
Contributor

ptabor commented Jul 6, 2020

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

@bart0sh
Copy link
Contributor Author

bart0sh commented Jul 6, 2020

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

@jpbetz
Copy link
Contributor

jpbetz commented Jul 7, 2020

cc @dims

@dims
Copy link
Contributor

dims commented Jul 7, 2020

thanks @jpbetz looks like needs a rebase LGTM otherwise!

@bart0sh
Copy link
Contributor Author

bart0sh commented Jul 8, 2020

@dims thank you for the review. rebased.

@dims
Copy link
Contributor

dims commented Jul 8, 2020

/lgtm

@liggitt
Copy link
Contributor

liggitt commented Jul 15, 2020

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

@jingyih
Copy link
Contributor

jingyih commented Jul 15, 2020

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

@spzala
Copy link
Member

spzala commented Jul 16, 2020

3.4.10 is released just now! Thanks @gyuho

gyuho added a commit that referenced this pull request Aug 14, 2020
…106-upstream-release-3.3

Automated cherry pick of #12106
gyuho added a commit that referenced this pull request Aug 18, 2020
ref. #12106

Signed-off-by: Gyuho Lee <leegyuho@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants