Issue 9084045: state: SetCharm checks rel/endpoint compatibility (Closed)
Descriptionstate: SetCharm checks rel/endpoint compatibility
This closes the loop around charm upgrades:
now you cannot upgrade to a charm without
compatible endpoints or relations.
It also fixes bug #1032557.
https://code.launchpad.net/~dimitern/juju-core/039-upgrade-charm-relations/+merge/162116
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 10
Patch Set 2 : state: SetCharm checks rel/endpoint compatibility #
Total comments: 12
Patch Set 3 : state: SetCharm checks rel/endpoint compatibility #
Total comments: 10
Patch Set 4 : state: SetCharm checks rel/endpoint compatibility #
MessagesTotal messages: 11
Please take a look.
Sign in to reply to this message.
Wrong endpoints, I'm afraid -- we don't want to prevent changes that don't actively break state. https://codereview.appspot.com/9084045/diff/1/state/service.go File state/service.go (right): https://codereview.appspot.com/9084045/diff/1/state/service.go#newcode370 state/service.go:370: asserts := []txn.Op{{ I'd really like to avoid multiple ops on the same document -- please put this assert in the op that modifies the service. https://codereview.appspot.com/9084045/diff/1/state/service.go#newcode384 state/service.go:384: eps, err := s.Endpoints() We only need to check the endpoints that correspond to relations that exist. Doesn't matter if we drop support for an endpoint we're not using. https://codereview.appspot.com/9084045/diff/1/state/service.go#newcode504 state/service.go:504: if err = s.Refresh(); err != nil { Refreshing the service changes state unexpectedly under the client -- please get a fresh s instead. https://codereview.appspot.com/9084045/diff/1/state/service_test.go File state/service_test.go (right): https://codereview.appspot.com/9084045/diff/1/state/service_test.go#newcode137 state/service_test.go:137: err: `charm "local:series/series-mysql-3" does not implement "fakemysql:server"`, These should pass, unless there's a relation using that endpoint. I'd expect errors like "cannot upgrade service %qto charm %q: would break relation %q", maybe? https://codereview.appspot.com/9084045/diff/1/worker/uniter/uniter_test.go File worker/uniter/uniter_test.go (left): https://codereview.appspot.com/9084045/diff/1/worker/uniter/uniter_test.go#ol... worker/uniter/uniter_test.go:563: ut( This should still work.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/9084045/diff/1/state/service.go File state/service.go (right): https://codereview.appspot.com/9084045/diff/1/state/service.go#newcode370 state/service.go:370: asserts := []txn.Op{{ On 2013/05/02 15:44:32, fwereade wrote: > I'd really like to avoid multiple ops on the same document -- please put this > assert in the op that modifies the service. Done. https://codereview.appspot.com/9084045/diff/1/state/service.go#newcode384 state/service.go:384: eps, err := s.Endpoints() On 2013/05/02 15:44:32, fwereade wrote: > We only need to check the endpoints that correspond to relations that exist. > Doesn't matter if we drop support for an endpoint we're not using. Done. https://codereview.appspot.com/9084045/diff/1/state/service.go#newcode504 state/service.go:504: if err = s.Refresh(); err != nil { On 2013/05/02 15:44:32, fwereade wrote: > Refreshing the service changes state unexpectedly under the client -- please get > a fresh s instead. Done. https://codereview.appspot.com/9084045/diff/1/state/service_test.go File state/service_test.go (right): https://codereview.appspot.com/9084045/diff/1/state/service_test.go#newcode137 state/service_test.go:137: err: `charm "local:series/series-mysql-3" does not implement "fakemysql:server"`, On 2013/05/02 15:44:32, fwereade wrote: > These should pass, unless there's a relation using that endpoint. I'd expect > errors like "cannot upgrade service %qto charm %q: would break relation %q", > maybe? Done, added more tests as well. https://codereview.appspot.com/9084045/diff/1/worker/uniter/uniter_test.go File worker/uniter/uniter_test.go (left): https://codereview.appspot.com/9084045/diff/1/worker/uniter/uniter_test.go#ol... worker/uniter/uniter_test.go:563: ut( On 2013/05/02 15:44:32, fwereade wrote: > This should still work. Indeed; restored.
Sign in to reply to this message.
LGTM with only one substantive question. https://codereview.appspot.com/9084045/diff/5001/state/service.go File state/service.go (right): https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode361 state/service.go:361: var asserts []txn.Op asserts := make([]txn.Op, 0, len(relations)) ? not that it makes much difference. https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode362 state/service.go:362: // Also all relations must still exist and their endpoints are implemented by the charm. "Also" ? https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode367 state/service.go:367: return nil, fmt.Errorf("cannot upgrade service %q to charm %q: would break relation %q", s.doc.Name, ch, rel) nice error https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode444 state/service.go:444: if len(relations) != s.doc.RelationCount { why do we care about the relation count stored in the doc?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/9084045/diff/5001/state/service.go File state/service.go (right): https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode361 state/service.go:361: var asserts []txn.Op On 2013/05/02 17:19:48, rog wrote: > asserts := make([]txn.Op, 0, len(relations)) > ? > not that it makes much difference. Done. https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode362 state/service.go:362: // Also all relations must still exist and their endpoints are implemented by the charm. On 2013/05/02 17:19:48, rog wrote: > "Also" ? Good catch https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode367 state/service.go:367: return nil, fmt.Errorf("cannot upgrade service %q to charm %q: would break relation %q", s.doc.Name, ch, rel) On 2013/05/02 17:19:48, rog wrote: > nice error William's idea, and I liked it https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode444 state/service.go:444: if len(relations) != s.doc.RelationCount { On 2013/05/02 17:19:48, rog wrote: > why do we care about the relation count stored in the doc? Changed RC means relations have changed, so we might as well get a fresh copy of ourselves and start over. Also an early warning not to run all the rest.
Sign in to reply to this message.
https://codereview.appspot.com/9084045/diff/5001/state/service.go File state/service.go (right): https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode444 state/service.go:444: if len(relations) != s.doc.RelationCount { On 2013/05/02 17:44:58, dimitern wrote: > On 2013/05/02 17:19:48, rog wrote: > > why do we care about the relation count stored in the doc? > > Changed RC means relations have changed, so we might as well get a fresh copy of > ourselves and start over. Also an early warning not to run all the rest. But haven't we just retrieved the relations? What other logic relies on the relation count stored in the doc?
Sign in to reply to this message.
Sorry, that wasn't a complete description perhaps, let me try again: https://codereview.appspot.com/9084045/diff/5001/state/service.go File state/service.go (right): https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode444 state/service.go:444: if len(relations) != s.doc.RelationCount { On 2013/05/02 18:05:58, rog wrote: > On 2013/05/02 17:44:58, dimitern wrote: > > On 2013/05/02 17:19:48, rog wrote: > > > why do we care about the relation count stored in the doc? > > > > Changed RC means relations have changed, so we might as well get a fresh copy > of > > ourselves and start over. Also an early warning not to run all the rest. > > But haven't we just retrieved the relations? > What other logic relies on the relation count > stored in the doc? If s.doc.RelationCount has changed, anything else there might be already stale, including the charm URL. It ofc makes any later steps in SetCharm potentially dangerous, moreover we assert on a RC being unchanged - if it already changed it means we have stale relations; even if it's unchanged though is not enough to guarantee a relations was not added and some other removed, hence the asserts for each relation document. In other words, stale RC invalidates later relation assertions anyway.
Sign in to reply to this message.
https://codereview.appspot.com/9084045/diff/5001/state/service.go File state/service.go (right): https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode444 state/service.go:444: if len(relations) != s.doc.RelationCount { On 2013/05/02 18:28:46, dimitern wrote: > On 2013/05/02 18:05:58, rog wrote: > > On 2013/05/02 17:44:58, dimitern wrote: > > > On 2013/05/02 17:19:48, rog wrote: > > > > why do we care about the relation count stored in the doc? > > > > > > Changed RC means relations have changed, so we might as well get a fresh > copy > > of > > > ourselves and start over. Also an early warning not to run all the rest. > > > > But haven't we just retrieved the relations? > > What other logic relies on the relation count > > stored in the doc? > > If s.doc.RelationCount has changed, anything else there might be already stale, > including the charm URL. ISTM that checking the relation count is neither necessary nor sufficient to check whether the charm URL has changed, and after a quick glance I can't see anything else in the doc that we need to rely on. > It ofc makes any later steps in SetCharm potentially > dangerous, moreover we assert on a RC being unchanged - if it already changed it > means we have stale relations; even if it's unchanged though is not enough to > guarantee a relations was not added and some other removed, hence the asserts > for each relation document. In other words, stale RC invalidates later relation > assertions anyway. I don't believe it does. We're predicating those assertions on the relations we've just retrieved, not anything in the doc, so even if the doc is stale, our assertions may still succeed. Or perhaps I'm still missing something - quite possible! Can't we just use s.doc.RelationCount below instead of len(relations) and avoid the errRefresh check?
Sign in to reply to this message.
https://codereview.appspot.com/9084045/diff/5001/state/service.go File state/service.go (right): https://codereview.appspot.com/9084045/diff/5001/state/service.go#newcode444 state/service.go:444: if len(relations) != s.doc.RelationCount { On 2013/05/03 07:35:11, rog wrote: > On 2013/05/02 18:28:46, dimitern wrote: > > On 2013/05/02 18:05:58, rog wrote: > > > On 2013/05/02 17:44:58, dimitern wrote: > > > > On 2013/05/02 17:19:48, rog wrote: > > > > > why do we care about the relation count stored in the doc? > > > > > > > > Changed RC means relations have changed, so we might as well get a fresh > > copy > > > of > > > > ourselves and start over. Also an early warning not to run all the rest. > > > > > > But haven't we just retrieved the relations? > > > What other logic relies on the relation count > > > stored in the doc? > > > > If s.doc.RelationCount has changed, anything else there might be already > stale, > > including the charm URL. > > ISTM that checking the relation count is neither necessary nor sufficient to > check whether the charm URL has changed, and after a quick glance I can't see > anything else in the doc that we need to rely on. > That's why we're also asserting on each relation separately. RC is necessary, but not enough to tell whether the relations have changed, but it's a good indicator to skip checking the relations, if RC is different. > > It ofc makes any later steps in SetCharm potentially > > dangerous, moreover we assert on a RC being unchanged - if it already changed > it > > means we have stale relations; even if it's unchanged though is not enough to > > guarantee a relations was not added and some other removed, hence the asserts > > for each relation document. In other words, stale RC invalidates later > relation > > assertions anyway. > > I don't believe it does. We're predicating those assertions on the relations > we've just retrieved, not anything in the doc, so even if the doc is stale, our > assertions may still succeed. Or perhaps I'm still missing something - quite > possible! > > Can't we just use s.doc.RelationCount below instead of len(relations) and avoid > the errRefresh check? We cannot avoid refreshing when we know the data is stale, and we need to assert on len(relations), because RC might be stale, as discussed already. Maybe fwereade can explain it better I hope?
Sign in to reply to this message.
LGTM given a quick chat online for confirmation https://codereview.appspot.com/9084045/diff/9001/state/service.go File state/service.go (right): https://codereview.appspot.com/9084045/diff/9001/state/service.go#newcode367 state/service.go:367: return nil, fmt.Errorf("cannot upgrade service %q to charm %q: would break relation %q", s.doc.Name, ch, rel) fwiw you could just have s instead of s.doc.Name, I'm pretty sure it has a sensible String() method. https://codereview.appspot.com/9084045/diff/9001/state/service.go#newcode375 state/service.go:375: return asserts, nil Just a thought -- maybe you could construct sameRelCount in here and return it? Not sure though... if you think it's stupid, don't bother. https://codereview.appspot.com/9084045/diff/9001/state/service.go#newcode449 state/service.go:449: sameRelCount := D{{"relationcount", s.doc.RelationCount}} I think we discussed last night that we don't care about the current value of s.doc.RelationCount -- we can skip the length check, and just assert that the execution-time value is the same as the value we'd expect from having seen N relations. So I think the above can just be: relations, err := s.Relations() if err != nil { return nil, err } sameRelCount := D{{"relationcount", len(relations)}} ...and then there's no need for the errRefresh handling. https://codereview.appspot.com/9084045/diff/9001/state/service.go#newcode456 state/service.go:456: Assert: sameRelCount, // txn.DocExists is implicit and cannot be combined with another assert Hmm. I think we should have a check that it's alive... I don't think there's any point upgrading a dying service, and the aborted check in SetCharm implies that we service death is a reason to abort. Sorry about the DocExists, it was almost certainly my bug. https://codereview.appspot.com/9084045/diff/9001/state/service.go#newcode504 state/service.go:504: } Without errRefresh, I think we can revert this whole block.
Sign in to reply to this message.
*** Submitted: state: SetCharm checks rel/endpoint compatibility This closes the loop around charm upgrades: now you cannot upgrade to a charm without compatible endpoints or relations. It also fixes bug #1032557. R=fwereade, rog CC= https://codereview.appspot.com/9084045 https://codereview.appspot.com/9084045/diff/9001/state/service.go File state/service.go (right): https://codereview.appspot.com/9084045/diff/9001/state/service.go#newcode367 state/service.go:367: return nil, fmt.Errorf("cannot upgrade service %q to charm %q: would break relation %q", s.doc.Name, ch, rel) On 2013/05/03 10:13:18, fwereade wrote: > fwiw you could just have s instead of s.doc.Name, I'm pretty sure it has a > sensible String() method. Done. https://codereview.appspot.com/9084045/diff/9001/state/service.go#newcode375 state/service.go:375: return asserts, nil On 2013/05/03 10:13:18, fwereade wrote: > Just a thought -- maybe you could construct sameRelCount in here and return it? > Not sure though... if you think it's stupid, don't bother. I think it's more logical to do that in changeCharmOps anyway, since that's where we get the relations and construct the services collection operation. https://codereview.appspot.com/9084045/diff/9001/state/service.go#newcode449 state/service.go:449: sameRelCount := D{{"relationcount", s.doc.RelationCount}} On 2013/05/03 10:13:18, fwereade wrote: > I think we discussed last night that we don't care about the current value of > s.doc.RelationCount -- we can skip the length check, and just assert that the > execution-time value is the same as the value we'd expect from having seen N > relations. > > So I think the above can just be: > > relations, err := s.Relations() > if err != nil { > return nil, err > } > sameRelCount := D{{"relationcount", len(relations)}} > > ...and then there's no need for the errRefresh handling. Done. https://codereview.appspot.com/9084045/diff/9001/state/service.go#newcode456 state/service.go:456: Assert: sameRelCount, // txn.DocExists is implicit and cannot be combined with another assert On 2013/05/03 10:13:18, fwereade wrote: > Hmm. I think we should have a check that it's alive... I don't think there's any > point upgrading a dying service, and the aborted check in SetCharm implies that > we service death is a reason to abort. Sorry about the DocExists, it was almost > certainly my bug. Added isAliveDoc + sameRelCount as asserts. https://codereview.appspot.com/9084045/diff/9001/state/service.go#newcode504 state/service.go:504: } On 2013/05/03 10:13:18, fwereade wrote: > Without errRefresh, I think we can revert this whole block. Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
