Issue 6453061: implement RelationState persistence
|
Created:
11 years, 9 months ago by fwereade Modified:
11 years, 9 months ago Reviewers:
mp+117181 Visibility:
Public. |
Descriptionimplement RelationState persistence
RelationState gets a new field, Path, and two new methods:
(*RelationState) Validate(HookInfo) error
...which allows a client to verify the sanity of a hook before
it is run, and:
(*RelationState) Commit(HookInfo) error
...which persists the state change to disk, and should be called on
successful execution of a hook, or resolution of an error state.
RelationState is aggressive about error detection and should always
write consistent states; errors while committing a hook should not
cause any change to the RelationState or its persistence directory.
A new AllRelationStates function, which scans a complete relations
directory and returns a map of relation id to *RelationState stored
therein, will be convenient when reconciling persisted relation state
against the unit's service's latest relation membership.
https://code.launchpad.net/~fwereade/juju-core/persist-relation-state/+merge/117181
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 23
Patch Set 2 : implement RelationState persistence #
Total comments: 31
Patch Set 3 : implement RelationState persistence #
Total comments: 6
Patch Set 4 : implement RelationState persistence #
MessagesTotal messages: 10
Please take a look.
Sign in to reply to this message.
Looking nice. I'm not sure about Validate though - shouldn't we be confident enough that our code is working correctly? But perhaps putting a charm into error state because we're about to do something wrong *is* appropriate... although there's probably very little the user can do to extricate themselves from the situation. Maybe just a log message would be appropriate. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go File worker/uniter/relationstate.go (right): https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:74: walker := func(path string, fi os.FileInfo) error { given that your directory is flat, i can't quite see why you're using filepath.Walk. why not just create the directory and use os.ReadDir or ioutil.ReadDir? the same applies to AllRelationStates above, i think. i believe this would make the logic more direct and easier to follow. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:90: m := map[string]interface{}{} much better (not to mention more efficient) to use a struct directly: var settings struct { ChangedPending bool Version int64 } if err := goyaml.Unmarshal(data, &settings); err != nil { return err } https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:93: } else { this can go. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:121: err = fmt.Errorf("invalid %q for %q: %v", hi.HookKind, hi.RemoteUnit, err) "out-of-sequence hook %q for %q: %v" ? after all, the hook itself isn't invalid, just when it's being called. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:163: unit := struct { ah, i see you're using a struct here. why not just declare this struct at the top of the file and use it for both marshalling and unmarshalling? https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:165: ChangedPending bool `yaml:"changed-pending,omitempty"` any particular reason we can just use "ChangedPending" as the field name and lose the yaml annotation? there's no backward compatibility issue here. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:174: if err = ioutil.WriteFile(path+"~", data, 0777); err != nil { perhaps a comment is justified here: // Create the entry for the relation and atomically // rename it to replace the old one. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:211: func unitname(filename string) (string, error) { s/unitname/unitName/
Sign in to reply to this message.
On 2012/07/30 08:28:09, rog wrote: > Looking nice. I'm not sure about Validate though - shouldn't we be confident > enough that our code is working correctly? But perhaps putting a charm into > error state because we're about to do something wrong *is* appropriate... > although there's probably very little the user can do to extricate themselves > from the situation. Maybe just a log message would be appropriate. I'm checking for situations that can "never happen"; ie, if they do happen, we can be sure that *something*'s state is corrupt enough that it would be hugely foolhardy for us to attempt to run a hook in those conditions. I'm not entirely confident that I will not typo/thinko some aspect of Uniter while I develop it; I think the validation will help us to debug any such issues if and when they come up, and I think the extra verification costs us very little.
Sign in to reply to this message.
Thanks. A couple of responses follow; I'll get on the easy fixes in a mo. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go File worker/uniter/relationstate.go (right): https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:74: walker := func(path string, fi os.FileInfo) error { On 2012/07/30 08:28:09, rog wrote: > given that your directory is flat, i can't quite see why you're using > filepath.Walk. why not just create the directory and use os.ReadDir or > ioutil.ReadDir? > the same applies to AllRelationStates above, i think. > > i believe this would make the logic more direct and easier to follow. You're absolutely right, thanks. The code took a slightly surprising evolutionary path :). https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:90: m := map[string]interface{}{} On 2012/07/30 08:28:09, rog wrote: > much better (not to mention more efficient) to use a struct directly: > > var settings struct { > ChangedPending bool > Version int64 > } > if err := goyaml.Unmarshal(data, &settings); err != nil { > return err > } The trouble is that if the file is gbberish, goyaml will happily fill in a zero struct for me, which is not so helpful in this situation. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:93: } else { On 2012/07/30 08:28:09, rog wrote: > this can go. True https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:121: err = fmt.Errorf("invalid %q for %q: %v", hi.HookKind, hi.RemoteUnit, err) On 2012/07/30 08:28:09, rog wrote: > "out-of-sequence hook %q for %q: %v" ? > after all, the hook itself isn't invalid, just when it's being called. Good idea https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:163: unit := struct { On 2012/07/30 08:28:09, rog wrote: > ah, i see you're using a struct here. > why not just declare this struct at the top of the file and use it for both > marshalling and unmarshalling? Bad zero values, as described above. Not sure why I'm not just using a map here, honestly :). https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:165: ChangedPending bool `yaml:"changed-pending,omitempty"` On 2012/07/30 08:28:09, rog wrote: > any particular reason we can just use "ChangedPending" as the field name and > lose the yaml annotation? there's no backward compatibility issue here. I think doing it like this makes the relation unit storage format more pleasant to create in tests, and more pleasant to read in practice. Bad? https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:174: if err = ioutil.WriteFile(path+"~", data, 0777); err != nil { On 2012/07/30 08:28:09, rog wrote: > perhaps a comment is justified here: > // Create the entry for the relation and atomically > // rename it to replace the old one. Good idea. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:211: func unitname(filename string) (string, error) { On 2012/07/30 08:28:09, rog wrote: > s/unitname/unitName/ Similarly good idea.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go File worker/uniter/relationstate.go (right): https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:74: walker := func(path string, fi os.FileInfo) error { On 2012/07/30 08:42:11, fwereade wrote: > On 2012/07/30 08:28:09, rog wrote: > > given that your directory is flat, i can't quite see why you're using > > filepath.Walk. why not just create the directory and use os.ReadDir or > > ioutil.ReadDir? > > the same applies to AllRelationStates above, i think. > > > > i believe this would make the logic more direct and easier to follow. > > You're absolutely right, thanks. The code took a slightly surprising > evolutionary path :). Done. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:90: m := map[string]interface{}{} On 2012/07/30 08:42:11, fwereade wrote: > On 2012/07/30 08:28:09, rog wrote: > > much better (not to mention more efficient) to use a struct directly: > > > > var settings struct { > > ChangedPending bool > > Version int64 > > } > > if err := goyaml.Unmarshal(data, &settings); err != nil { > > return err > > } > > The trouble is that if the file is gbberish, goyaml will happily fill in a zero > struct for me, which is not so helpful in this situation. Discussed live; was enlightening. Done. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:93: } else { On 2012/07/30 08:42:11, fwereade wrote: > On 2012/07/30 08:28:09, rog wrote: > > this can go. > > True Done. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:121: err = fmt.Errorf("invalid %q for %q: %v", hi.HookKind, hi.RemoteUnit, err) On 2012/07/30 08:42:11, fwereade wrote: > On 2012/07/30 08:28:09, rog wrote: > > "out-of-sequence hook %q for %q: %v" ? > > after all, the hook itself isn't invalid, just when it's being called. > > Good idea "inappropriate"? https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:163: unit := struct { On 2012/07/30 08:42:11, fwereade wrote: > On 2012/07/30 08:28:09, rog wrote: > > ah, i see you're using a struct here. > > why not just declare this struct at the top of the file and use it for both > > marshalling and unmarshalling? > > Bad zero values, as described above. Not sure why I'm not just using a map here, > honestly :). Done. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:174: if err = ioutil.WriteFile(path+"~", data, 0777); err != nil { On 2012/07/30 08:42:11, fwereade wrote: > On 2012/07/30 08:28:09, rog wrote: > > perhaps a comment is justified here: > > // Create the entry for the relation and atomically > > // rename it to replace the old one. > > Good idea. Done. https://codereview.appspot.com/6453061/diff/1/worker/uniter/relationstate.go#... worker/uniter/relationstate.go:211: func unitname(filename string) (string, error) { On 2012/07/30 08:42:11, fwereade wrote: > On 2012/07/30 08:28:09, rog wrote: > > s/unitname/unitName/ > > Similarly good idea. Done.
Sign in to reply to this message.
LGTM, particularly the walk->readdir change. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.go File worker/uniter/relationstate.go (right): https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:19: defer errorContextf(&err, "cannot load relations state from %s: %v", dirpath, err) s/%s/%q/ https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:162: data, err := goyaml.Marshal(info) ... or in-line the diskInfo value? https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate_... File worker/uniter/relationstate_test.go (right): https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate_... worker/uniter/relationstate_test.go:202: expect := fmt.Sprintf(`inappropriate %q for %q: %s`, hi.HookKind, hi.RemoteUnit, t.err) not sure about using this error message as a regexp here, but i suppose if we're careful not to have re metachars it's ok...
Sign in to reply to this message.
Great stuff! A first round: https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.go File worker/uniter/relationstate.go (right): https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:19: defer errorContextf(&err, "cannot load relations state from %s: %v", dirpath, err) s/: %v// s/, err// https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:29: relationId, err := strconv.Atoi(fi.Name()) if err != nil || fi.Mode()&os.ModeType != 0 { continue } Acting on the positive side is a lot more resilient. If it's not our content, we don't care. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:73: // with "~", which will be ignored. If the directory does not exist, it will be These are often used as backups, and tempting to remove. A user doing "rm *~" could break the agent. I suggest being more explicit, with something like ".preparing" https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:76: path := filepath.Join(dirpath, fmt.Sprintf("%d", relationId)) strconv.Itoa? https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:77: defer errorContextf(&err, "cannot load relation state from %s: %v", path, err) s/: %v// s/, err// https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:87: if fi.IsDir() { Let's act on the positive side here as well. If it doesn't match our expectations for a name or is not a regular file, ignore it. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:122: // ensure that the system's guarantees about hook execution order hold true. Nice assurance. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:134: return fmt.Errorf(`unit already joined`) s/`/"/g https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:136: return fmt.Errorf("unit not joined") s/not/hasn't/ https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:150: defer errorContextf(&err, "failed to commit %q for %q: %v", hi.HookKind, hi.RemoteUnit) s/: %v// https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:151: name := strings.Replace(hi.RemoteUnit, "/", "-", -1) This can only ever have one slash. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:168: if err = ioutil.WriteFile(path+"~", data, 0777); err != nil { s/0777/0644/; no reason to give away more than that I believe? https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:189: return os.Mkdir(path, 0777) s/0777/0755/; this would be a significant security hole. The WriteFile above would allow anyone to overwrite any file on the system. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:206: func unitName(filename string) (string, error) { s/error/bool/; we don't care about details here. We won't be parsing or even complaining about unknown stuff.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.go File worker/uniter/relationstate.go (right): https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:19: defer errorContextf(&err, "cannot load relations state from %s: %v", dirpath, err) On 2012/07/31 01:40:08, niemeyer wrote: > s/: %v// > s/, err// Done. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:29: relationId, err := strconv.Atoi(fi.Name()) On 2012/07/31 01:40:08, niemeyer wrote: > if err != nil || fi.Mode()&os.ModeType != 0 { > continue > } > > Acting on the positive side is a lot more resilient. If it's not our content, we > don't care. Slightly different proposal: we ignore anything we don't recognise, but we enforce our expectations on things we do. So, in a relations dir, files/dirs with unknown names are just fine, but those with integer names must be relation directories; similarly, within a relation, things with valid unit names must be files with valid data. Reasonable? https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:73: // with "~", which will be ignored. If the directory does not exist, it will be On 2012/07/31 01:40:08, niemeyer wrote: > These are often used as backups, and tempting to remove. A user doing "rm *~" > could break the agent. I suggest being more explicit, with something like > ".preparing" Done. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:76: path := filepath.Join(dirpath, fmt.Sprintf("%d", relationId)) On 2012/07/31 01:40:08, niemeyer wrote: > strconv.Itoa? Done. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:77: defer errorContextf(&err, "cannot load relation state from %s: %v", path, err) On 2012/07/31 01:40:08, niemeyer wrote: > s/: %v// > s/, err// Done. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:87: if fi.IsDir() { On 2012/07/31 01:40:08, niemeyer wrote: > Let's act on the positive side here as well. If it doesn't match our > expectations for a name or is not a regular file, ignore it. Done. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:134: return fmt.Errorf(`unit already joined`) On 2012/07/31 01:40:08, niemeyer wrote: > s/`/"/g Done. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:136: return fmt.Errorf("unit not joined") On 2012/07/31 01:40:08, niemeyer wrote: > s/not/hasn't/ "has not", I think, for consistency. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:150: defer errorContextf(&err, "failed to commit %q for %q: %v", hi.HookKind, hi.RemoteUnit) On 2012/07/31 01:40:08, niemeyer wrote: > s/: %v// Done. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:151: name := strings.Replace(hi.RemoteUnit, "/", "-", -1) On 2012/07/31 01:40:08, niemeyer wrote: > This can only ever have one slash. Done. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:162: data, err := goyaml.Marshal(info) On 2012/07/30 13:52:30, rog wrote: > ... or in-line the diskInfo value? Done. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:168: if err = ioutil.WriteFile(path+"~", data, 0777); err != nil { On 2012/07/31 01:40:08, niemeyer wrote: > s/0777/0644/; no reason to give away more than that I believe? Done. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:189: return os.Mkdir(path, 0777) On 2012/07/31 01:40:08, niemeyer wrote: > s/0777/0755/; this would be a significant security hole. The WriteFile above > would allow anyone to overwrite any file on the system. Done. https://codereview.appspot.com/6453061/diff/6001/worker/uniter/relationstate.... worker/uniter/relationstate.go:206: func unitName(filename string) (string, error) { On 2012/07/31 01:40:08, niemeyer wrote: > s/error/bool/; we don't care about details here. We won't be parsing or even > complaining about unknown stuff. Done.
Sign in to reply to this message.
LGTM, with a few suggestions for test readability: https://codereview.appspot.com/6453061/diff/8001/worker/uniter/relationstate_... File worker/uniter/relationstate_test.go (right): https://codereview.appspot.com/6453061/diff/8001/worker/uniter/relationstate_... worker/uniter/relationstate_test.go:56: {nil, []string{"foo-bar-1"}, `.* is a directory`}, The previous format here was significantly more readable. Can we have it back? https://codereview.appspot.com/6453061/diff/8001/worker/uniter/relationstate_... worker/uniter/relationstate_test.go:89: return uniter.HookInfo{ Why do we need all of the helpers here? This: uniter.HookInfo{ RelationId: 1, HookInfo: "changed", RemoteUnit: "foo/1", ChangeVersion: 123, }, Is so much better than this: hookInfos([]hookInfo{ {"foo/1", "changed", 1, 123}, }) https://codereview.appspot.com/6453061/diff/8001/worker/uniter/relationstate_... worker/uniter/relationstate_test.go:118: }), msi{"foo/1": 1, "foo/2": 0}, "", "", These tests would also benefit from breaking down fields into their own lines. If there are many empty fields, or potentially confusing meaning, using field names would also work well.
Sign in to reply to this message.
*** Submitted: implement RelationState persistence RelationState gets a new field, Path, and two new methods: (*RelationState) Validate(HookInfo) error ...which allows a client to verify the sanity of a hook before it is run, and: (*RelationState) Commit(HookInfo) error ...which persists the state change to disk, and should be called on successful execution of a hook, or resolution of an error state. RelationState is aggressive about error detection and should always write consistent states; errors while committing a hook should not cause any change to the RelationState or its persistence directory. A new AllRelationStates function, which scans a complete relations directory and returns a map of relation id to *RelationState stored therein, will be convenient when reconciling persisted relation state against the unit's service's latest relation membership. R=rog, niemeyer CC= https://codereview.appspot.com/6453061 https://codereview.appspot.com/6453061/diff/8001/worker/uniter/relationstate_... File worker/uniter/relationstate_test.go (right): https://codereview.appspot.com/6453061/diff/8001/worker/uniter/relationstate_... worker/uniter/relationstate_test.go:56: {nil, []string{"foo-bar-1"}, `.* is a directory`}, On 2012/07/31 12:52:48, niemeyer wrote: > The previous format here was significantly more readable. Can we have it back? Done. https://codereview.appspot.com/6453061/diff/8001/worker/uniter/relationstate_... worker/uniter/relationstate_test.go:89: return uniter.HookInfo{ On 2012/07/31 12:52:48, niemeyer wrote: > Why do we need all of the helpers here? This: > > uniter.HookInfo{ > RelationId: 1, > HookInfo: "changed", > RemoteUnit: "foo/1", > ChangeVersion: 123, > }, > > Is so much better than this: > > hookInfos([]hookInfo{ > {"foo/1", "changed", 1, 123}, > }) You're right, it did turn out nicer. https://codereview.appspot.com/6453061/diff/8001/worker/uniter/relationstate_... worker/uniter/relationstate_test.go:118: }), msi{"foo/1": 1, "foo/2": 0}, "", "", On 2012/07/31 12:52:48, niemeyer wrote: > These tests would also benefit from breaking down fields into their own lines. > If there are many empty fields, or potentially confusing meaning, using field > names would also work well. Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
