Three small state features #307
Conversation
License: MIT Signed-off-by: Wyatt Daviau <wdaviau@cs.stanford.edu>
ghost
assigned 
License: MIT Signed-off-by: Wyatt Daviau <wdaviau@cs.stanford.edu>
License: MIT Signed-off-by: Wyatt Daviau <wdaviau@cs.stanford.edu>
ZenGround0
force-pushed
the
feat/auto-migration
branch
from
e4fce52 to
2262adf
Compare
ipfs-cluster-service/main.go
Outdated
| @@ -193,6 +193,10 @@ func main() { | |||
| Value: "disk-freespace", | |||
| Usage: "allocation strategy to use [disk-freespace,disk-reposize,numpin].", | |||
| }, | |||
| cli.BoolFlag{ | |||
There was a problem hiding this comment.
I think I like this better as a flag specific for the daemon subcommand, rather than a global. ipfs-cluster-service daemon --ugprade
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| err = stateFromSnap.Unmarshal(raw) |
There was a problem hiding this comment.
Something is not right here I think. If the state is completely different, Unmarshal might just fail because it doesn't know how to do it. That's why we did not unmarshal before and directly call Migrate. So it may make migrations fail.
Even if unmarshal does not fail, it might just not replace the Version field, which will be set to mapstate.Version, and give a false positive when doing GetVersion() == mapstate.Version below.
Looking around I see that we have this problem in the mapstate.Migrate() function too.
The straightfoward fix is that we make first-byte-indicates-version a requirement for all state marshaled-formats, and that we figure out the versions by just reading that first byte, and not by unmarshaling unknown formats on top of the current state types. A state helper (thinking GetRawVersion(r io.Reader) int) might help there. We have to remember to avoid loading full states into memory unless we need to (i.e. if we are going to fail or exist, because a version is not the expected, we shouldn't need to do ReadAll on the raw state, but just read the first byte.
There was a problem hiding this comment.
The straightfoward fix is that we make first-byte-indicates-version a requirement for all state marshaled-formats, and that we figure out the versions by just reading that first byte, and not by unmarshaling unknown formats on top of the current state types.
My understanding is that this is what we are doing now. I haven't been crystal clear about the requirement that first byte (and after a certain point varint as in multiformats) indicates version so thanks for bringing this up. Right now it is in the godoc for mapstate.Unmarshal but I can probably add it elsewhere and more clearly. As you can see from within Unmarshal however right now we are not unmarshaling unknown formats onto the state type without checking version. The logic of the proposed GetRawVersion is effectively called from within Unmarshal (146 - 150). If the version doesn't match, unmarshal won't decode. There is no way the state object's version is populated with a false positive match-with-current-version assuming the first byte is correct. (You could have false negatives but the state would be corrupt anyway in that case)
I agree with you that it is not ideal to read all the bytes before passing to Unmarshal because sometimes the first byte will suffice. I am good with changing Unmarshal to take in a reader, consume the first byte to check for version, potentially quit if out of date, and continue reading otherwise. Note that this is going to require some changes (easy I think) to libp2p-raft Restore, and DecodeSnapshot.
Am I understanding all of your concerns correctly? Did I miss anything? Are you ok with the suggested fix?
Thanks!
There was a problem hiding this comment.
I totally missed that unmarshal checked the version byte before actually unmarshaling because it seemed we were doing that exact check afterwards. It's super weird that it doesn't return an error when it hasn't Unmarshaled anything because of a version mismatch. I think that's what confused me.
Let me have another look tomorrow with a fresh brain.
There was a problem hiding this comment.
@ZenGround0 sorry, I dropped this, LGTM for the moment. You just need to address the comment above about the flag's scope
License: MIT Signed-off-by: Wyatt Daviau <wdaviau@cs.stanford.edu>
Feat #300 --
--upgradeflag for automatically running upgrades when launching serviceFix #296 -- current state formats no longer cause upgrades. No error but a warning
Feat #298 --
ipfs-cluster-service state versionreports the mapstate version used by this cluster binary