Upgrade gogo/protobuf to 1.3.0 #2055

cyriltovena · 2020-01-30T19:04:56Z

Signed-off-by: Cyril Tovena cyril.tovena@gmail.com

What this PR does:

This PR upgrades gogo/protobuf generation tools from 1.2.1 to 1.3. I've dig into their changelog and this is a single version jump, only bug fixes (gogo/protobuf@v1.2.1...v1.3.0). The reason I'm upgrading is because there is an API break between 1.2.1 and 1.3.0 generated pb files, which means I can't import any proto files in the project I'm working on (Loki).

While you can't import generated 1.2 file with generated 1.3 file (compilation issue), they are although still backward compatible (two applications can communicate by using 1.2 and 1.3 respectively)

go vendoring was already using 1.3.0.

Checklist

Tests updated
Documentation added
[ x] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

cyriltovena · 2020-01-30T19:13:43Z

Ho no ! weaveworks needs to also be updated for the frontend.

import "github.com/weaveworks/common/httpgrpc/httpgrpc.proto";

The build error is the same problem I'm hitting in Loki. (MarshalToSizedBuffer undefined)

cyriltovena · 2020-01-30T19:49:58Z

see https://github.com/gogo/protobuf#release-v130

With this new release comes a new internal library version. This means any newly generated *pb.go files generated with the v1.3.0 library will not be compatible with the old library version (v1.2.1). However, current *pb.go files (generated with v1.2.1) should still work with the new library.

Please make sure you manage your dependencies correctly when upgrading your project. If you are still using v1.2.1 and you update your dependencies, one of which could include a new *pb.go (generated with v1.3.0), you could get a compile time error.

pracucci

Thanks @cyriltovena! Let's wait for weaveworks/common#177 so that we can proceed with this PR.

CHANGELOG.md

bboreham · 2020-01-31T12:37:21Z

still backward compatible (two applications can communicate by using 1.2 and 1.3 respectively)

Who has tested this? What was the process and results?

(We could add it to the "integration tests" in this repo)

cyriltovena · 2020-01-31T18:09:03Z

@bboreham wrote a test, found a bug with the custom type, I guess I have to regen those too, I'm working on it.

Everything else looks good will share more soon. I don't think there is a compatibility issue, I think I didn't regen code inside https://github.com/cortexproject/cortex/blob/master/pkg/ingester/client/timeseries.go

cyriltovena · 2020-01-31T21:31:48Z

@bboreham I added a full ingesters tests and there were only issues with custom type. Once regenerated everything works fine, I can assure you now that the wire format has not changed.

Not sure if you want to keep those tests they are very long (took me a day), but at least you have the proof, I'm happy to remove them if you want.

We're now blocked by weaveworks/common#177 which has no custom type usage so this will also be backward compatible.

bboreham · 2020-02-01T14:20:50Z

I don't think there is a compatibility issue

With respect, what you think matters a lot less than what the tests show us.

Are the three CircleCI failures something you can fix?

bboreham · 2020-02-01T14:22:47Z

I don't understand why we have extra data structure code for integration tests: why don't we just run the image from Cortex 0.6.0 ?

cyriltovena · 2020-02-01T15:47:59Z

Are the three CircleCI failures something you can fix?

The CI fails because of weaveworks/common#177. see my comment here #2055 (comment)

With respect, what you think matters a lot less than what the tests show us.

This is the test I wrote.
https://github.com/cortexproject/cortex/blob/f60de80bce45ed7e3f1b6633356b3bbaa65954d9/integration-tests/proto/proto_test.go

It uses the two different versions of the code (1.2.1 is master and 1.3.0 is this PR). It was more simple to experiment with proto services, I believe writing a full integration test on a CI that I don't control will be way more complex and will probably cover less (the feedback loop was way faster with go tests).

Those tests that I did actually covers the full ingesters proto service which is the most complex we have, I also tests marshalling of custom proto from 1.2 to 1.3 and vise versa because those were having issues (forgot to regenerate that copied code) so I put more effort into those. (see https://github.com/cortexproject/cortex/blob/f60de80bce45ed7e3f1b6633356b3bbaa65954d9/integration-tests/proto/proto_test.go)

I'm basically giving a proof of compatibility with that test, you can try it yourself and we can get rid of it once we all are happy.

bboreham · 2020-02-01T16:56:04Z

Can you put an override in go.mod so this PR builds off your branch in weaveworks/common?

I still don't get what's so complicated about running an integration test with two different image versions. E.g. 0.6.0 ingester against current distributor. We have push tests already; why isn't it a matter of copy-pasting that with a different image tag?

cyriltovena · 2020-02-01T17:57:59Z

I'll put the override, and give a go to integration tests.

cyriltovena · 2020-02-01T20:12:41Z

I removed my go test (although it allowed me to discover a bug in the query path) as this was a simple proof of backward compatibility.

I merged your PR.

Let me know.

cyriltovena · 2020-02-01T21:07:01Z

Looks like we need to update the CI with the new image, but I would need access or someone to do it for me.

check-protos runs against the old version which fails.

bboreham · 2020-02-02T17:41:14Z

The CI is controlled from .circleci/config.yml; you can change that as part of a PR.

Probably you can’t push an image under the same name, but for the purpose of testing you can use a different name.

cyriltovena · 2020-02-03T13:22:43Z

This is ready to go, I'll rebase to upstream/master once I have the green light.

pracucci

Thanks @cyriltovena for your patience addressing feeback! 🙏

In terms of backward compatibility within Cortex, I feel quite comfortable with this PR. I've picked the new integration tests (which include backward compatibility tests) from the PR #1839, merged them into this PR's branch, rebuilt the Docker image and run the integration tests: passed. See details below.

$ hub pr checkout 2055
$ git merge --squash setup-integration-tests-with-docker-compose-and-golang

# Then rebuilt cortex:latest image before running the integration tests:
$ go test -timeout 60s -v ./integration

=== RUN   TestBackwardCompatibilityWithChunksStorage
Starting dynamodb
Starting consul
dynamodb: Initializing DynamoDB Local with the following configuration:
dynamodb: Port:	8000
dynamodb: InMemory:	true
dynamodb: DbPath:	null
dynamodb: SharedDb:	true
dynamodb: shouldDelayTransientStatuses:	false
dynamodb: CorsParams:	*
consul: ==> Starting Consul agent...
consul: ==> Consul agent running!
consul: Version: 'v0.9.4'
consul: Node ID: 'a3ea2a25-0474-c943-9df7-8d2b4c017195'
consul: Node name: 'consul'
consul: Datacenter: 'dc1' (Segment: '<all>')
consul: Server: true (Bootstrap: false)
consul: Client Addr: 0.0.0.0 (HTTP: 8500, HTTPS: -1, DNS: 8600)
consul: Cluster Addr: 127.0.0.1 (LAN: 8301, WAN: 8302)
consul: Encrypt: Gossip: false, TLS-Outgoing: false, TLS-Incoming: false
consul: ==> Log data will now stream in as it occurs:
Starting table-manager
Starting ingester-1
consul: ==> Newer Consul version available: 1.6.3 (currently running: 0.9.4)
Starting distributor
Starting ingester-2
Stopping ingester-1
Starting querier
querier: 2020/02/06 08:12:09 Request.HasNextPage deprecated. Use Pagination type for configurable pagination of API operations
Stopping querier
Starting querier
querier: 2020/02/06 08:12:12 Request.HasNextPage deprecated. Use Pagination type for configurable pagination of API operations
Stopping querier
Killing ingester-2
ingester-2: level=warn ts=2020-02-06T08:12:13.4828736Z caller=transfer.go:422 msg="transfer attempt failed" err="cannot find ingester to transfer chunks to: no pending ingesters" attempt=1 max_retries=10
Killing distributor
Killing table-manager
Killing consul
Killing dynamodb
--- PASS: TestBackwardCompatibilityWithChunksStorage (19.51s)
=== RUN   TestIngesterFlushWithChunksStorage
Starting dynamodb
Starting consul
dynamodb: Initializing DynamoDB Local with the following configuration:
dynamodb: Port:	8000
dynamodb: InMemory:	true
dynamodb: DbPath:	null
dynamodb: SharedDb:	true
dynamodb: shouldDelayTransientStatuses:	false
dynamodb: CorsParams:	*
consul: ==> Starting Consul agent...
consul: ==> Consul agent running!
consul: Version: 'v0.9.4'
consul: Node ID: '2b8d9200-616b-ba16-cd27-02ea7c9f4fb7'
consul: Node name: 'consul'
consul: Datacenter: 'dc1' (Segment: '<all>')
consul: Server: true (Bootstrap: false)
consul: Client Addr: 0.0.0.0 (HTTP: 8500, HTTPS: -1, DNS: 8600)
consul: Cluster Addr: 127.0.0.1 (LAN: 8301, WAN: 8302)
consul: Encrypt: Gossip: false, TLS-Outgoing: false, TLS-Incoming: false
consul: ==> Log data will now stream in as it occurs:
Starting table-manager
Starting ingester-1
Starting querier
Starting distributor
querier: 2020/02/06 08:12:25 Request.HasNextPage deprecated. Use Pagination type for configurable pagination of API operations
Stopping ingester-1
Killing distributor
Killing querier
Killing table-manager
Killing consul
Killing dynamodb
--- PASS: TestIngesterFlushWithChunksStorage (12.73s)
=== RUN   TestIngesterHandOverWithBlocksStorage
Starting minio
Starting consul
minio: Attempting encryption of all config, IAM users and policies on MinIO backend
consul: ==> Starting Consul agent...
consul: ==> Consul agent running!
consul: Version: 'v0.9.4'
consul: Node ID: '33ddd101-4865-8bf9-77c5-44c9e8a5158c'
consul: Node name: 'consul'
consul: Datacenter: 'dc1' (Segment: '<all>')
consul: Server: true (Bootstrap: false)
consul: Client Addr: 0.0.0.0 (HTTP: 8500, HTTPS: -1, DNS: 8600)
consul: Cluster Addr: 127.0.0.1 (LAN: 8301, WAN: 8302)
consul: Encrypt: Gossip: false, TLS-Outgoing: false, TLS-Incoming: false
consul: ==> Log data will now stream in as it occurs:
Starting ingester-1
Starting querier
Starting distributor
Starting ingester-2
Stopping ingester-1
ingester-1: level=warn ts=2020-02-06T08:12:37.2829326Z caller=transfer.go:422 msg="transfer attempt failed" err="cannot find ingester to transfer blocks to: no pending ingesters" attempt=1 max_retries=10
Killing ingester-2
ingester-2: level=warn ts=2020-02-06T08:12:38.1423973Z caller=transfer.go:422 msg="transfer attempt failed" err="cannot find ingester to transfer blocks to: no pending ingesters" attempt=1 max_retries=10
Killing distributor
Killing querier
Killing consul
Killing minio
minio: Exiting on signal: TERMINATED
--- PASS: TestIngesterHandOverWithBlocksStorage (11.86s)
=== RUN   TestIngesterHandOverWithChunksStorage
Starting dynamodb
Starting consul
dynamodb: Initializing DynamoDB Local with the following configuration:
dynamodb: Port:	8000
dynamodb: InMemory:	true
dynamodb: DbPath:	null
dynamodb: SharedDb:	true
dynamodb: shouldDelayTransientStatuses:	false
dynamodb: CorsParams:	*
consul: ==> Starting Consul agent...
consul: ==> Consul agent running!
consul: Version: 'v0.9.4'
consul: Node ID: 'f339f5e6-fdbc-0041-38b3-783506cb222d'
consul: Node name: 'consul'
consul: Datacenter: 'dc1' (Segment: '<all>')
consul: Server: true (Bootstrap: false)
consul: Client Addr: 0.0.0.0 (HTTP: 8500, HTTPS: -1, DNS: 8600)
consul: Cluster Addr: 127.0.0.1 (LAN: 8301, WAN: 8302)
consul: Encrypt: Gossip: false, TLS-Outgoing: false, TLS-Incoming: false
consul: ==> Log data will now stream in as it occurs:
Starting table-manager
Starting ingester-1
Starting querier
Starting distributor
querier: 2020/02/06 08:12:49 Request.HasNextPage deprecated. Use Pagination type for configurable pagination of API operations
Starting ingester-2
Stopping ingester-1
ingester-1: level=warn ts=2020-02-06T08:12:50.7051847Z caller=transfer.go:422 msg="transfer attempt failed" err="cannot find ingester to transfer chunks to: no pending ingesters" attempt=1 max_retries=10
Killing ingester-2
ingester-2: level=warn ts=2020-02-06T08:12:51.521813Z caller=transfer.go:422 msg="transfer attempt failed" err="cannot find ingester to transfer chunks to: no pending ingesters" attempt=1 max_retries=10
Killing distributor
Killing querier
Killing table-manager
Killing consul
Killing dynamodb
--- PASS: TestIngesterHandOverWithChunksStorage (13.90s)
PASS
ok  	github.com/cortexproject/cortex/integration	58.028s

/cc @bboreham

CHANGELOG.md

.circleci/config.yml

docs/configuration/config-file-reference.md

go.mod

integration-tests/test-backcompat.sh

pkg/ingester/client/timeseries.go

pracucci

Thanks @cyriltovena! CI tests pass. I also did another round of the new integration tests (PR #1839) against this PR and passed.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Here we test pushing from the new distributor to an older ingester, and hand-over from the old ingester to a new ingester. The backwards-compatibility version is pinned at v0.6.0. Signed-off-by: Bryan Boreham <bryan@weave.works>

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com> go mod check Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com> update circle-ci image Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com> Rebased to master Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

gouthamve

LGTM with nit. I don't think this should be in the changelog.

CHANGELOG.md

bboreham

lgtm with one thought about a test.
The changelog line can be removed when we do the actual release; I think we've listed smaller changes there.

pkg/ingester/client/timeseries_test.go

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

cyriltovena · 2020-02-20T13:22:33Z

Merge squash when you're ready. (Those commits are not useful)

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

… update-gogoproto

This was referenced Jan 30, 2020

Update gogo proto to 1.3.0 weaveworks/common#177

Merged

Add statistics to query_range and instant_query API. grafana/loki#1615

Merged

pracucci reviewed Jan 31, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

bboreham mentioned this pull request Feb 1, 2020

Add integration test using older image #2064

Closed

cyriltovena force-pushed the update-gogoproto branch from 8ee3d69 to 6e528d5 Compare February 3, 2020 19:37

pracucci reviewed Feb 6, 2020

View reviewed changes

cyriltovena force-pushed the update-gogoproto branch from d5339d9 to b97288b Compare February 6, 2020 22:43

pracucci approved these changes Feb 7, 2020

View reviewed changes

cyriltovena and others added 8 commits February 18, 2020 16:12

Upgrade gogo/protobuf to 1.3.0

839adbd

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Update changelog

2744cbb

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Adding compatibility tests, found an issue

cd48c6a

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Fixes compatibility issue with labelpair custom type

3a9d070

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Full ingester tests.

129cfbb

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Move weavework/common to gogoproto 1.3 fork.

8ae691f

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Add integration test using older image

3a81bc2

Here we test pushing from the new distributor to an older ingester, and hand-over from the old ingester to a new ingester. The backwards-compatibility version is pinned at v0.6.0. Signed-off-by: Bryan Boreham <bryan@weave.works>

cyriltovena added 9 commits February 18, 2020 16:13

update build image to quay official repo.

4ba8e8c

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Remove changelog entry.

3a066f5

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

make doc, rebase, go mod update

eb11bb6

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Adds label pair marshal test

bbe92fb

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Rollback to my own build image.

c558afb

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Update go sum

8d7937f

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Update ruler proto with 1.3

645b847

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

quay build image.

9d52741

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Update doc.

db944b6

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

cyriltovena force-pushed the update-gogoproto branch from b228a31 to db944b6 Compare February 18, 2020 21:16

Regenerate new proto.

fbd92c1

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

gouthamve approved these changes Feb 20, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

bboreham approved these changes Feb 20, 2020

View reviewed changes

pkg/ingester/client/timeseries_test.go Show resolved Hide resolved

cyriltovena added 2 commits February 20, 2020 08:19

Add empty name tests for LabelAdapter Unmarshal.

b1b5f88

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Merge branch 'master' into update-gogoproto

6b5e996

cyriltovena added 2 commits February 20, 2020 08:24

Remove changelog item for this PR

63a87b2

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>

Merge branch 'update-gogoproto' of github.com:cyriltovena/cortex into…

cf1ff56

… update-gogoproto

pracucci merged commit c63a4f2 into cortexproject:master Feb 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade gogo/protobuf to 1.3.0 #2055

Upgrade gogo/protobuf to 1.3.0 #2055

cyriltovena commented Jan 30, 2020 •

edited

Loading

cyriltovena commented Jan 30, 2020

cyriltovena commented Jan 30, 2020

pracucci left a comment

bboreham commented Jan 31, 2020

cyriltovena commented Jan 31, 2020 •

edited

Loading

cyriltovena commented Jan 31, 2020

bboreham commented Feb 1, 2020

bboreham commented Feb 1, 2020

cyriltovena commented Feb 1, 2020 •

edited

Loading

bboreham commented Feb 1, 2020

cyriltovena commented Feb 1, 2020

cyriltovena commented Feb 1, 2020

cyriltovena commented Feb 1, 2020 •

edited

Loading

bboreham commented Feb 2, 2020

cyriltovena commented Feb 3, 2020

pracucci left a comment •

edited

Loading

pracucci left a comment

gouthamve left a comment

bboreham left a comment

cyriltovena commented Feb 20, 2020

Upgrade gogo/protobuf to 1.3.0 #2055

Upgrade gogo/protobuf to 1.3.0 #2055

Conversation

cyriltovena commented Jan 30, 2020 • edited Loading

cyriltovena commented Jan 30, 2020

cyriltovena commented Jan 30, 2020

pracucci left a comment

Choose a reason for hiding this comment

bboreham commented Jan 31, 2020

cyriltovena commented Jan 31, 2020 • edited Loading

cyriltovena commented Jan 31, 2020

bboreham commented Feb 1, 2020

bboreham commented Feb 1, 2020

cyriltovena commented Feb 1, 2020 • edited Loading

bboreham commented Feb 1, 2020

cyriltovena commented Feb 1, 2020

cyriltovena commented Feb 1, 2020

cyriltovena commented Feb 1, 2020 • edited Loading

bboreham commented Feb 2, 2020

cyriltovena commented Feb 3, 2020

pracucci left a comment • edited Loading

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

gouthamve left a comment

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

cyriltovena commented Feb 20, 2020

cyriltovena commented Jan 30, 2020 •

edited

Loading

cyriltovena commented Jan 31, 2020 •

edited

Loading

cyriltovena commented Feb 1, 2020 •

edited

Loading

cyriltovena commented Feb 1, 2020 •

edited

Loading

pracucci left a comment •

edited

Loading