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

Upgrade go-msgpack to v2 #20173

Merged
merged 6 commits into from
Mar 21, 2024
Merged

Upgrade go-msgpack to v2 #20173

merged 6 commits into from
Mar 21, 2024

Conversation

schmichael
Copy link
Member

Upgraded to go-msgpack v2. Replaces #18812 because I'm better at writing new PRs than I am at resolving merge conflicts.

Manually tested with a set of 3 servers at different versions: this PR, 1.7.6, and 1.6.9. Ran a job on a 1.5.2 client agent just to really try and keep things wacky. Didn't notice anything unusual.

Please peek at go.mod and 772d35f in particular. Everything else is mechanically changed. See the commit messages on d467ab5 and 7e353e0 if you want to mock my ham fisted import rewriting.

Replaces #18812

Upgraded with:
```
find . -name '*.go' -exec sed -i s/"github.com\/hashicorp\/go-msgpack\/codec"/"github.com\/hashicorp\/go-msgpack\/v2\/codec/" '{}' ';'
find . -name '*.go' -exec sed -i s/"github.com\/hashicorp\/net-rpc-msgpackrpc"/"github.com\/hashicorp\/net-rpc-msgpackrpc\/v2/" '{}' ';'
go get
```
Commands:
```
go get -v -u github.com/hashicorp/raft-boltdb/v2
go get -v github.com/hashicorp/serf@5d32001edfaa18d1c010af65db707cdb38141e80
```
Copy link

@swenson swenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think everything here looks good, but there still appears to be a reference to go-msgpack 1.1.5 in GNUmakefile

@schmichael
Copy link
Member Author

@swenson great catch, thanks. Added in 6328341

Copy link

@swenson swenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shoenig
Copy link
Member

shoenig commented Mar 21, 2024

Just to make sure I understand, we are going to go through the time encoding change due to setting

MsgpackUseNewTimeFormat: true,

but this is okay, because the decoders on the other end in v2 can handle both cases?

// MsgpackUseNewTimeFormat when set to true, force the underlying msgpack
// codec to use the new format of time.Time when encoding (used in
// go-msgpack v1.1.5 by default). Decoding is not affected, as all
// go-msgpack v2.1.0+ decoders know how to decode both formats.
MsgpackUseNewTimeFormat [bool](https://pkg.go.dev/builtin#bool)

if so LGTM

@schmichael
Copy link
Member Author

Just to make sure I understand, we are going to go through the time encoding change due to setting

Not quite. Nomad was already on msgpack-1.1.5, so we were already using the "New" time encoding.

but this is okay, because the decoders on the other end in v2 can handle both cases?

This would not be upgrade safe, so not ok. If we had been on the msgpack-0.5.5 encoding then during upgrades the old agents would not understand times from new agents. Luckily we were on msgpack-1.1.5 before.

@shoenig
Copy link
Member

shoenig commented Mar 21, 2024

Ah thanks, I understand now. I was curious how Nomad could have gotten to v1.1.5 without breakage but our go.mod history starts at 15fb4c9 which already uses that version. So looking at vendor.json we introduce go-msgpack at version "b-fix-gomod" to replace ugorji at version "upstream-08f7b40", and everything before that is referenced by git hashes.

Whew, what a mess.

@schmichael schmichael merged commit 23e4b7c into main Mar 21, 2024
19 checks passed
@schmichael schmichael deleted the dep-msgpack branch March 21, 2024 18:44
@schmichael schmichael mentioned this pull request Mar 22, 2024
5 tasks
philrenaud pushed a commit that referenced this pull request Apr 18, 2024
Replaces #18812

Upgraded with:
```
find . -name '*.go' -exec sed -i s/"github.com\/hashicorp\/go-msgpack\/codec"/"github.com\/hashicorp\/go-msgpack\/v2\/codec/" '{}' ';'
find . -name '*.go' -exec sed -i s/"github.com\/hashicorp\/net-rpc-msgpackrpc"/"github.com\/hashicorp\/net-rpc-msgpackrpc\/v2/" '{}' ';'
go get
go get -v -u github.com/hashicorp/raft-boltdb/v2
go get -v github.com/hashicorp/serf@5d32001edfaa18d1c010af65db707cdb38141e80
```

see https://github.com/hashicorp/go-msgpack/releases/tag/v2.1.0
for details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants