Upgrade go-msgpack to v2 2.1.1 #18812
Conversation
go.mod
Outdated
|
|
||
| replace github.com/hashicorp/raft => /Users/swenson/projects/raft | ||
|
|
||
| replace github.com/hashicorp/raft-boltdb/v2 => /Users/swenson/projects/raft-boltdb/v2 | ||
|
|
||
| replace github.com/hashicorp/memberlist => /Users/swenson/projects/memberlist | ||
|
|
||
| replace github.com/hashicorp/serf => /Users/swenson/projects/serf |
There was a problem hiding this comment.
woops! I think this is causing all the current CI failures
There was a problem hiding this comment.
Yep, there is a parameter being added in these (MsgpackUseNewTimeFormat) to help with compatibility, so this PR's build won't work until these are merged anyway.
There was a problem hiding this comment.
If there are branches in each of these projects that contain a SHA we can see, we can go get that version into here instead so that this can run through CI, rather than strictly holding for merge until we get some greens. I've had to do similar shenanigans with consul-template.
There was a problem hiding this comment.
Oh that's a good idea. I'll try that. Thanks.
There was a problem hiding this comment.
This LGTM but will want to do some local testing first. I'm also not sure why tests don't appear to have run. 🤔
Once all of the other PRs are approved, can we merge from the "inside out" so that we can switch from commit hash pins to real versions?
(I converted this to Draft to signal it shouldn't be merged even if the code looks good. We can drop the Draft marker and get it merged once the other PRs are approved.)
go.mod
Outdated
| replace github.com/hashicorp/nomad/client => ./client | ||
|
|
There was a problem hiding this comment.
Did you mean to commit this? ./api is only replaced because it is also a Go module while ./client is just a Go package.
There was a problem hiding this comment.
Something was breaking when I was trying to build. Not sure what this was about. I'll remove it.
And set the `time.Time` option to use the go-msgpack-1.1.5-compatible encoding in all the places, since that is the (now previous) version in `go.mod`. v2 2.1.1 was specifically designe to honor backwards compatibility with 1.1.5 and 0.5.5, and to clean up the code base to be more maintainable. There may performance lost with the 1.1.5 to 2.1.1 migration since the fastpath code was removed, but the increased safety is probably worth it. See [the release notes for go-msgkack 2.1.0](https://github.com/hashicorp/go-msgpack/releases/tag/v2.1.0) for more details. I tested this by running this code, and booting up a cluster with a node also running the prior version of Nomad (before the upgrade). Before I made the changes to set the right `time.Time` option, the previous-version node would throw a bunch of time-decoding errors. After fixing the option, the node came up smoothly, even after changing leadership between them. This relies on - [ ] hashicorp/serf#705 - [ ] hashicorp/raft-boltdb#38 - [ ] hashicorp/raft#577 - [ ] hashicorp/memberlist#292 and maybe - [ ] hashicorp/net-rpc-msgpackrpc#12
swenson
force-pushed
the
vault-19781/msgpack-upgrade
branch
from
a0f0c0f
to
b28874f
Compare
|
I think tests weren't running because it was a draft PR? I resolved conflicts and re-pushed. |
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 ```
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
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
And set the
time.Timeoption to use the go-msgpack-1.1.5-compatible encoding in all the places, since that is the (now previous) version ingo.mod.v2 2.1.1 was specifically designed to honor backwards compatibility with 1.1.5 and 0.5.5, and to clean up the code base to be more maintainable. There may performance lost with the 1.1.5 to 2.1.1 migration since the fastpath code was removed, but the increased safety is probably worth it. See
the release notes for go-msgkack 2.1.0 for more details.
I tested this by running this code, and booting up a cluster with a node also running the prior version of Nomad (before the upgrade). Before I made the changes to set the right
time.Timeoption, the previous-version node would throw a bunch of time-decoding errors. After fixing the option, the node came up smoothly, even after changing leadership between them.This relies on
and maybe
and will need to be updated after they are merged to get the
go.modfixes removed.