Merge lp:~themue/pyjuju/go-state-add-relation into lp:pyjuju/go

Proposed by Frank Mueller
Status: Merged
Merged at revision: 200
Proposed branch: lp:~themue/pyjuju/go-state-add-relation
Merge into: lp:pyjuju/go
Prerequisite: lp:~themue/pyjuju/go-state-topology-relation-endpoints
Diff against target: 280 lines (+250/-3)
3 files modified
state/relation.go (+33/-0)
state/state.go (+117/-0)
state/state_test.go (+100/-3)
To merge this branch: bzr merge lp:~themue/pyjuju/go-state-add-relation
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+106652@code.launchpad.net

Description of the change

state: Added two methods to add relations to State.

AddClientServerReleation() gets a client and a server endpoint,
AddPeerRelation() a peer endpoint. They return the relation and
both service relations for client/server or the one for peer.
The seperation into two methods with well defined arguments make
it easier to ensure a valid relation creation.

https://codereview.appspot.com/6223055/

To post a comment you must log in.
Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

https://codereview.appspot.com/6223055/diff/1/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6223055/diff/1/state/relation.go#newcode60
state/relation.go:60: func (r *Relation) Key() string {
The internal key is internal. :-)

https://codereview.appspot.com/6223055/diff/1/state/relation.go#newcode73
state/relation.go:73: // Key returns the internal key of a relation.
Ditto.

https://codereview.appspot.com/6223055/diff/1/state/relation.go#newcode78
state/relation.go:78: // ServiceKey returns the service key of a
relation.
Ditto.

https://codereview.appspot.com/6223055/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6223055/diff/1/state/state.go#newcode239
state/state.go:239: if clientEp.RelationRole != RoleClient {
As a general and vague comment, visually these functions do not look
nice. Maybe there's no way to avoid the complexity, but please look for
opportunities to separate logical blocks visually, and perhaps using
functions is adequate.

I'll hold-off on reviewing this until we sort out the pre-req, since
there are necessary changes there that will certainly affect this.

https://codereview.appspot.com/6223055/

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :
Download full text (6.6 KiB)

https://codereview.appspot.com/6223055/diff/9001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode50
state/relation.go:50: // Relation represents a connection between one or
more services.
// Relation represents a link between services, or within a
// service (in the case of peer relations).

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode56
state/relation.go:56: // ServiceRelation represents a relation between
one or more services.
// ServiceRelation represents an established relation from
// the viewpoint of a participant service.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode59
state/relation.go:59: key string
s/key/relationKey/; given the type name, this would be ambiguous.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode63
state/relation.go:63: name string
relationScope, relationRole, relationName, for the same reason.

Also on the methods below please (RelationRole, RelationName, etc).

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode66
state/relation.go:66: // Scope returns the scope of a relation.
s/a/the/

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode71
state/relation.go:71: // Role returns the role of a relation.
// RelationRole returns the service role within the relation.

https://codereview.appspot.com/6223055/diff/9001/state/relation.go#newcode76
state/relation.go:76: // Name returns the name of a relation.
// RelationName returns the name this relation has within the service.

https://codereview.appspot.com/6223055/diff/9001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode257
state/state.go:257: if relationKey != "" {
This is ignoring err. Even if it works, I'd prefer if we were more
conservative about it:

if _, ok := err.(*NoRelationError); ok {
     return false, nil
}
if err != nil {
     return false, err
}
return true, err

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode266
state/state.go:266: // addRelation creates the relation node depending
on the given scope.
I don't get what "depending on the given scope" is trying to tell me.

addRelation also seems like a bad name for this function. There's both
addRelation and AddRelation, and they are very different beasts.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode274
state/state.go:274: // Creation is per container for container scoped
relations and
I don't quite get what this comment is saying either.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode276
state/state.go:276: if scope == ScopeGlobal {
This seems weird. Why is it only introducing the settings if the
relation is global? I don't know if that makes sense, and if it does we
certainly need a clear comment here explaining the background.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode286
state/state.go:286: // and endpoint.
s/and endpoint/endpoint/, comments above hold here and in the function
below too.

https://codereview.apps...

Read more...

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

The code diff has a lot of unrelated code. Please let me know once the
code is clean and may be reviewed.

https://codereview.appspot.com/6223055/

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :
Download full text (3.9 KiB)

LGTM, but see the comments please:

https://codereview.appspot.com/6223055/diff/9001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode276
state/state.go:276: if scope == ScopeGlobal {
On 2012/05/31 16:23:59, TheMue wrote:
> On 2012/05/30 22:35:42, niemeyer wrote:
> > This seems weird. Why is it only introducing the settings if the
relation is
> > global? I don't know if that makes sense, and if it does we
certainly need a
> > clear comment here explaining the background.

> I tried to make the comments more clear, they have been almost a 1:1
copy of the
> Python code. We should talk about the container scope and if the whole
method(s)
> can't be modified to handle this too. So far I don't know enough about
the
> meaning of the scope and have to analyze it first.

Okay, this branch sounds fine besides what we already talked about and
the minor points, but please do understand the scope logic. As you have
seen in the relation+topology story, writing logic when you have no idea
of what is going on is hard.

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode334
state/state.go:334: relationKey, err := s.addRelation(scope)
On 2012/05/31 16:23:59, TheMue wrote:
> On 2012/05/30 22:35:42, niemeyer wrote:
> > Have we verified that the endpoints make sense at this point? It
looks like
> > we've checked that there is a good number of them, but I can't see
it
> verifying
> > that the endpoints make sense (service names, relation names,
interface names,
> > etc). It'd be bad to be creating state in ZooKeeper just to later
tell
> something
> > trivial to the user that we could have verified upfront. We
shouldn't be
> > duplicating this logic, though.

> Today this isn't done. I would like to do it in a new small branch
after this
> and RemoveRelation() are in.

I filed a bug to keep track of this and assigned to you:

https://bugs.launchpad.net/juju-core/+bug/1007373

https://codereview.appspot.com/6223055/diff/9001/state/state.go#newcode369
state/state.go:369: Service: serviceRelation.serviceKey,
On 2012/05/31 16:23:59, TheMue wrote:
> On 2012/05/30 22:35:42, niemeyer wrote:
> > serviceRelation has a public API that we can use it here, rather
than fiddling
> > with its internals assuming we know what's going on.
> >
> > Same below.

> The name can be retrieved with RelationName() but the service key is
internal so
> far. Shall a public accessor be added?

No, you're right. serviceKey is internal.

https://codereview.appspot.com/6223055/diff/10005/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6223055/diff/10005/state/state.go#newcode263
state/state.go:263: // container occurs in ServiceRelation.AddUnit().
That's a lot nicer, thanks.

Would you mind to just drop the () as we tend to refer to methods by
their name without the call syntax.

https://codereview.appspot.com/6223055/diff/10005/state/state.go#newcode279
state/state.go:279: if scope == ScopeGlobal {
That's a bit strange. This method only does anything at all if the scope
is not global, and the scope is only provided so it can be checked here
for not doing anything at all.

Ins...

Read more...

164. By Frank Mueller on 2012-06-04

state: Merge of trunk after LGTM.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'state/relation.go'
2--- state/relation.go 2012-05-25 16:48:30 +0000
3+++ state/relation.go 2012-06-01 12:44:24 +0000
4@@ -46,3 +46,36 @@
5 func (e RelationEndpoint) String() string {
6 return e.ServiceName + ":" + e.RelationName
7 }
8+
9+// Relation represents a link between services, or within a
10+// service (in the case of peer relations).
11+type Relation struct {
12+ st *State
13+ key string
14+}
15+
16+// ServiceRelation represents an established relation from
17+// the viewpoint of a participant service.
18+type ServiceRelation struct {
19+ st *State
20+ relationKey string
21+ serviceKey string
22+ relationScope RelationScope
23+ relationRole RelationRole
24+ relationName string
25+}
26+
27+// RelationScope returns the scope of the relation.
28+func (r *ServiceRelation) RelationScope() RelationScope {
29+ return r.relationScope
30+}
31+
32+// RelationRole returns the service role within the relation.
33+func (r *ServiceRelation) RelationRole() RelationRole {
34+ return r.relationRole
35+}
36+
37+// RelationName returns the name this relation has within the service.
38+func (r *ServiceRelation) RelationName() string {
39+ return r.relationName
40+}
41
42=== modified file 'state/state.go'
43--- state/state.go 2012-05-29 23:11:41 +0000
44+++ state/state.go 2012-06-01 12:44:24 +0000
45@@ -250,3 +250,120 @@
46 }
47 return service.Unit(name)
48 }
49+
50+// addRelationNode creates the relation node.
51+func (s *State) addRelationNode(scope RelationScope) (string, error) {
52+ path, err := s.zk.Create("/relations/relation-", "", zookeeper.SEQUENCE, zkPermAll)
53+ if err != nil {
54+ return "", err
55+ }
56+ relationKey := strings.Split(path, "/")[2]
57+ // Create the settings node only if the scope is global.
58+ // In case of container scoped relations the creation per
59+ // container occurs in ServiceRelation.AddUnit.
60+ if scope == ScopeGlobal {
61+ _, err = s.zk.Create(path+"/settings", "", 0, zkPermAll)
62+ if err != nil {
63+ return "", err
64+ }
65+ }
66+ return relationKey, nil
67+}
68+
69+// addRelationEndpointNode creates the endpoint role node below its relation node
70+// for the given relation endpoint.
71+func (s *State) addRelationEndpointNode(relationKey string, endpoint RelationEndpoint) error {
72+ path := fmt.Sprintf("/relations/%s/%s", relationKey, string(endpoint.RelationRole))
73+ _, err := s.zk.Create(path, "", 0, zkPermAll)
74+ return err
75+}
76+
77+// AddRelation creates a new relation with the given endpoints.
78+func (s *State) AddRelation(endpoints ...RelationEndpoint) (*Relation, []*ServiceRelation, error) {
79+ switch len(endpoints) {
80+ case 1:
81+ if endpoints[0].RelationRole != RolePeer {
82+ return nil, nil, fmt.Errorf("can't add non-peer relation with a single service")
83+ }
84+ case 2:
85+ if !endpoints[0].CanRelateTo(&endpoints[1]) {
86+ return nil, nil, fmt.Errorf("can't add relation between %s and %s", endpoints[0], endpoints[1])
87+ }
88+ default:
89+ return nil, nil, fmt.Errorf("can't add relations between %d services", len(endpoints))
90+ }
91+ t, err := readTopology(s.zk)
92+ if err != nil {
93+ return nil, nil, err
94+ }
95+ // Check if the relation already exists.
96+ relationKey, err := t.RelationKey(endpoints...)
97+ if err != nil {
98+ if _, ok := err.(*NoRelationError); !ok {
99+ return nil, nil, err
100+ }
101+ }
102+ if relationKey != "" {
103+ return nil, nil, fmt.Errorf("relation already exists")
104+ }
105+ scope := ScopeGlobal
106+ for _, endpoint := range endpoints {
107+ if endpoint.RelationScope == ScopeContainer {
108+ scope = ScopeContainer
109+ break
110+ }
111+ }
112+ // Add a new relation node depending on the scope. Afterwards
113+ // create a node and a service relation per endpoint.
114+ relationKey, err = s.addRelationNode(scope)
115+ if err != nil {
116+ return nil, nil, err
117+ }
118+ serviceRelations := []*ServiceRelation{}
119+ for _, endpoint := range endpoints {
120+ serviceKey, err := t.ServiceKey(endpoint.ServiceName)
121+ if err != nil {
122+ return nil, nil, err
123+ }
124+ // The relation endpoint node is only created if the scope is
125+ // global. In case of container scoped relations the creation
126+ // per container occurs in ServiceRelation.AddUnit.
127+ if scope == ScopeGlobal {
128+ if err = s.addRelationEndpointNode(relationKey, endpoint); err != nil {
129+ return nil, nil, err
130+ }
131+ }
132+ serviceRelations = append(serviceRelations, &ServiceRelation{
133+ st: s,
134+ relationKey: relationKey,
135+ serviceKey: serviceKey,
136+ relationScope: endpoint.RelationScope,
137+ relationRole: endpoint.RelationRole,
138+ relationName: endpoint.RelationName,
139+ })
140+ }
141+ // Add relation to topology.
142+ addRelation := func(t *topology) error {
143+ relation := &topoRelation{
144+ Interface: endpoints[0].Interface,
145+ Scope: scope,
146+ Services: map[RelationRole]*topoRelationService{},
147+ }
148+ for _, serviceRelation := range serviceRelations {
149+ if !t.HasService(serviceRelation.serviceKey) {
150+ return fmt.Errorf("state for service %q has changed", serviceRelation.serviceKey)
151+ }
152+ service := &topoRelationService{
153+ Service: serviceRelation.serviceKey,
154+ RelationName: serviceRelation.RelationName(),
155+ }
156+ relation.Services[serviceRelation.RelationRole()] = service
157+ }
158+ return t.AddRelation(relationKey, relation)
159+ }
160+ err = retryTopologyChange(s.zk, addRelation)
161+ if err != nil {
162+ return nil, nil, err
163+ }
164+ return &Relation{s, relationKey}, serviceRelations, nil
165+}
166
167=== modified file 'state/state_test.go'
168--- state/state_test.go 2012-05-30 01:11:22 +0000
169+++ state/state_test.go 2012-06-01 12:44:24 +0000
170@@ -1,6 +1,3 @@
171-// launchpad.net/juju/go/state
172-//
173-// Copyright (c) 2011-2012 Canonical Ltd.
174 package state_test
175
176 import (
177@@ -1017,3 +1014,103 @@
178 c.Assert(err, IsNil)
179 c.Assert(alive, Equals, false)
180 }
181+
182+func (s *StateSuite) TestAddRelation(c *C) {
183+ dummy := s.addDummyCharm(c)
184+ // Provider and requirer.
185+ s.st.AddService("mysqldb", dummy)
186+ s.st.AddService("wordpress", dummy)
187+ mysqlep := state.RelationEndpoint{"mysqldb", "blog", "db", state.RoleProvider, state.ScopeGlobal}
188+ blogep := state.RelationEndpoint{"wordpress", "blog", "db", state.RoleRequirer, state.ScopeGlobal}
189+ relation, serviceRelations, err := s.st.AddRelation(blogep, mysqlep)
190+ c.Assert(err, IsNil)
191+ c.Assert(relation, NotNil)
192+ c.Assert(serviceRelations, HasLen, 2)
193+ c.Assert(serviceRelations[0].RelationScope(), Equals, state.ScopeGlobal)
194+ c.Assert(serviceRelations[0].RelationRole(), Equals, state.RoleRequirer)
195+ c.Assert(serviceRelations[1].RelationScope(), Equals, state.ScopeGlobal)
196+ c.Assert(serviceRelations[1].RelationRole(), Equals, state.RoleProvider)
197+ c.Assert(serviceRelations[0].RelationName(), Equals, serviceRelations[1].RelationName())
198+ // Peer.
199+ s.st.AddService("riak", dummy)
200+ riakep := state.RelationEndpoint{"riak", "ring", "cache", state.RolePeer, state.ScopeGlobal}
201+ relation, serviceRelations, err = s.st.AddRelation(riakep)
202+ c.Assert(err, IsNil)
203+ c.Assert(relation, NotNil)
204+ c.Assert(serviceRelations, HasLen, 1)
205+ c.Assert(serviceRelations[0].RelationScope(), Equals, state.ScopeGlobal)
206+ c.Assert(serviceRelations[0].RelationRole(), Equals, state.RolePeer)
207+ c.Assert(serviceRelations[0].RelationName(), Equals, "cache")
208+
209+}
210+
211+func (s *StateSuite) TestAddRelationMissingService(c *C) {
212+ dummy := s.addDummyCharm(c)
213+ s.st.AddService("mysqldb", dummy)
214+ mysqlep := state.RelationEndpoint{"mysqldb", "blog", "db", state.RoleProvider, state.ScopeGlobal}
215+ blogep := state.RelationEndpoint{"wordpress", "blog", "db", state.RoleRequirer, state.ScopeGlobal}
216+ _, _, err := s.st.AddRelation(blogep, mysqlep)
217+ c.Assert(err, ErrorMatches, `service with name "wordpress" not found`)
218+}
219+
220+func (s *StateSuite) TestAddRelationMissingEndpoint(c *C) {
221+ dummy := s.addDummyCharm(c)
222+ s.st.AddService("mysqldb", dummy)
223+ mysqlep := state.RelationEndpoint{"mysqldb", "blog", "db", state.RoleProvider, state.ScopeGlobal}
224+ _, _, err := s.st.AddRelation(mysqlep)
225+ c.Assert(err, ErrorMatches, `can't add non-peer relation with a single service`)
226+}
227+
228+func (s *StateSuite) TestAddClientServerDifferentRoles(c *C) {
229+ dummy := s.addDummyCharm(c)
230+ s.st.AddService("mysqldb", dummy)
231+ s.st.AddService("riak", dummy)
232+ mysqlep := state.RelationEndpoint{"mysqldb", "ifce", "db", state.RoleRequirer, state.ScopeGlobal}
233+ riakep := state.RelationEndpoint{"riak", "ring", "cache", state.RolePeer, state.ScopeGlobal}
234+ _, _, err := s.st.AddRelation(mysqlep, riakep)
235+ c.Assert(err, ErrorMatches, `can't add relation between mysqldb:db and riak:cache`)
236+}
237+
238+func (s *StateSuite) TestAddRelationDifferentInterfaces(c *C) {
239+ dummy := s.addDummyCharm(c)
240+ s.st.AddService("mysqldb", dummy)
241+ s.st.AddService("wordpress", dummy)
242+ mysqlep := state.RelationEndpoint{"mysqldb", "ifce-a", "db", state.RoleProvider, state.ScopeGlobal}
243+ blogep := state.RelationEndpoint{"wordpress", "ifce-b", "db", state.RoleRequirer, state.ScopeGlobal}
244+ _, _, err := s.st.AddRelation(blogep, mysqlep)
245+ c.Assert(err, ErrorMatches, `can't add relation between wordpress:db and mysqldb:db`)
246+}
247+
248+func (s *StateSuite) TestAddClientServerRelationTwice(c *C) {
249+ dummy := s.addDummyCharm(c)
250+ // Provider and requirer.
251+ s.st.AddService("mysqldb", dummy)
252+ s.st.AddService("wordpress", dummy)
253+ mysqlep := state.RelationEndpoint{"mysqldb", "blog", "db", state.RoleProvider, state.ScopeGlobal}
254+ blogep := state.RelationEndpoint{"wordpress", "blog", "db", state.RoleRequirer, state.ScopeGlobal}
255+ _, _, err := s.st.AddRelation(blogep, mysqlep)
256+ c.Assert(err, IsNil)
257+ _, _, err = s.st.AddRelation(blogep, mysqlep)
258+ c.Assert(err, ErrorMatches, `relation already exists`)
259+ // Peer.
260+ s.st.AddService("riak", dummy)
261+ riakep := state.RelationEndpoint{"riak", "ring", "cache", state.RolePeer, state.ScopeGlobal}
262+ _, _, err = s.st.AddRelation(riakep)
263+ c.Assert(err, IsNil)
264+ _, _, err = s.st.AddRelation(riakep)
265+ c.Assert(err, ErrorMatches, `relation already exists`)
266+}
267+
268+func (s *StateSuite) TestAddPeerRelationIllegalEndpointNumber(c *C) {
269+ dummy := s.addDummyCharm(c)
270+ s.st.AddService("mysqldb", dummy)
271+ s.st.AddService("wordpress", dummy)
272+ s.st.AddService("riak", dummy)
273+ mysqlep := state.RelationEndpoint{"mysqldb", "ifce", "cache", state.RoleProvider, state.ScopeGlobal}
274+ blogep := state.RelationEndpoint{"wordpress", "ifce", "cache", state.RoleRequirer, state.ScopeGlobal}
275+ riakep := state.RelationEndpoint{"riak", "blog", "cache", state.RolePeer, state.ScopeGlobal}
276+ _, _, err := s.st.AddRelation()
277+ c.Assert(err, ErrorMatches, `can't add relations between 0 services`)
278+ _, _, err = s.st.AddRelation(mysqlep, blogep, riakep)
279+ c.Assert(err, ErrorMatches, `can't add relations between 3 services`)
280+}

Subscribers

People subscribed via source and target branches