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

[Frank Mueller]

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/

[Gustavo Niemeyer (niemeyer)]

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/

[Gustavo Niemeyer (niemeyer)]

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...

[Gustavo Niemeyer (niemeyer)]

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/

[Gustavo Niemeyer (niemeyer)]

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...

[lp:~themue/pyjuju/go-state-add-relation]

164. By Frank Mueller on 2012-06-04

state: Merge of trunk after LGTM.

=== modified file 'state/relation.go'
--- state/relation.go	2012-05-25 16:48:30 +0000
+++ state/relation.go	2012-06-01 12:44:24 +0000
@@ -46,3 +46,36 @@
 func (e RelationEndpoint) String() string {
 	return e.ServiceName + ":" + e.RelationName
 }
+
+// Relation represents a link between services, or within a
+// service (in the case of peer relations).
+type Relation struct {
+	st  *State
+	key string
+}
+
+// ServiceRelation represents an established relation from
+// the viewpoint of a participant service.
+type ServiceRelation struct {
+	st            *State
+	relationKey   string
+	serviceKey    string
+	relationScope RelationScope
+	relationRole  RelationRole
+	relationName  string
+}
+
+// RelationScope returns the scope of the relation.
+func (r *ServiceRelation) RelationScope() RelationScope {
+	return r.relationScope
+}
+
+// RelationRole returns the service role within the relation. 
+func (r *ServiceRelation) RelationRole() RelationRole {
+	return r.relationRole
+}
+
+// RelationName returns the name this relation has within the service.
+func (r *ServiceRelation) RelationName() string {
+	return r.relationName
+}
 
=== modified file 'state/state.go'
--- state/state.go	2012-05-29 23:11:41 +0000
+++ state/state.go	2012-06-01 12:44:24 +0000
@@ -250,3 +250,120 @@
 	}
 	return service.Unit(name)
 }
+
+// addRelationNode creates the relation node.
+func (s *State) addRelationNode(scope RelationScope) (string, error) {
+	path, err := s.zk.Create("/relations/relation-", "", zookeeper.SEQUENCE, zkPermAll)
+	if err != nil {
+		return "", err
+	}
+	relationKey := strings.Split(path, "/")[2]
+	// Create the settings node only if the scope is global.
+	// In case of container scoped relations the creation per
+	// container occurs in ServiceRelation.AddUnit.
+	if scope == ScopeGlobal {
+		_, err = s.zk.Create(path+"/settings", "", 0, zkPermAll)
+		if err != nil {
+			return "", err
+		}
+	}
+	return relationKey, nil
+}
+
+// addRelationEndpointNode creates the endpoint role node below its relation node 
+// for the given relation endpoint.
+func (s *State) addRelationEndpointNode(relationKey string, endpoint RelationEndpoint) error {
+	path := fmt.Sprintf("/relations/%s/%s", relationKey, string(endpoint.RelationRole))
+	_, err := s.zk.Create(path, "", 0, zkPermAll)
+	return err
+}
+
+// AddRelation creates a new relation with the given endpoints.  
+func (s *State) AddRelation(endpoints ...RelationEndpoint) (*Relation, []*ServiceRelation, error) {
+	switch len(endpoints) {
+	case 1:
+		if endpoints[0].RelationRole != RolePeer {
+			return nil, nil, fmt.Errorf("can't add non-peer relation with a single service")
+		}
+	case 2:
+		if !endpoints[0].CanRelateTo(&endpoints[1]) {
+			return nil, nil, fmt.Errorf("can't add relation between %s and %s", endpoints[0], endpoints[1])
+		}
+	default:
+		return nil, nil, fmt.Errorf("can't add relations between %d services", len(endpoints))
+	}
+	t, err := readTopology(s.zk)
+	if err != nil {
+		return nil, nil, err
+	}
+	// Check if the relation already exists.
+	relationKey, err := t.RelationKey(endpoints...)
+	if err != nil {
+		if _, ok := err.(*NoRelationError); !ok {
+			return nil, nil, err
+		}
+	}
+	if relationKey != "" {
+		return nil, nil, fmt.Errorf("relation already exists")
+	}
+	scope := ScopeGlobal
+	for _, endpoint := range endpoints {
+		if endpoint.RelationScope == ScopeContainer {
+			scope = ScopeContainer
+			break
+		}
+	}
+	// Add a new relation node depending on the scope. Afterwards
+	// create a node and a service relation per endpoint.
+	relationKey, err = s.addRelationNode(scope)
+	if err != nil {
+		return nil, nil, err
+	}
+	serviceRelations := []*ServiceRelation{}
+	for _, endpoint := range endpoints {
+		serviceKey, err := t.ServiceKey(endpoint.ServiceName)
+		if err != nil {
+			return nil, nil, err
+		}
+		// The relation endpoint node is only created if the scope is 
+		// global. In case of container scoped relations the creation 
+		// per container occurs in ServiceRelation.AddUnit.
+		if scope == ScopeGlobal {
+			if err = s.addRelationEndpointNode(relationKey, endpoint); err != nil {
+				return nil, nil, err
+			}
+		}
+		serviceRelations = append(serviceRelations, &ServiceRelation{
+			st:            s,
+			relationKey:   relationKey,
+			serviceKey:    serviceKey,
+			relationScope: endpoint.RelationScope,
+			relationRole:  endpoint.RelationRole,
+			relationName:  endpoint.RelationName,
+		})
+	}
+	// Add relation to topology.
+	addRelation := func(t *topology) error {
+		relation := &topoRelation{
+			Interface: endpoints[0].Interface,
+			Scope:     scope,
+			Services:  map[RelationRole]*topoRelationService{},
+		}
+		for _, serviceRelation := range serviceRelations {
+			if !t.HasService(serviceRelation.serviceKey) {
+				return fmt.Errorf("state for service %q has changed", serviceRelation.serviceKey)
+			}
+			service := &topoRelationService{
+				Service:      serviceRelation.serviceKey,
+				RelationName: serviceRelation.RelationName(),
+			}
+			relation.Services[serviceRelation.RelationRole()] = service
+		}
+		return t.AddRelation(relationKey, relation)
+	}
+	err = retryTopologyChange(s.zk, addRelation)
+	if err != nil {
+		return nil, nil, err
+	}
+	return &Relation{s, relationKey}, serviceRelations, nil
+}
 
=== modified file 'state/state_test.go'
--- state/state_test.go	2012-05-30 01:11:22 +0000
+++ state/state_test.go	2012-06-01 12:44:24 +0000
@@ -1,6 +1,3 @@
-// launchpad.net/juju/go/state
-//
-// Copyright (c) 2011-2012 Canonical Ltd.
 package state_test
 
 import (
@@ -1017,3 +1014,103 @@
 	c.Assert(err, IsNil)
 	c.Assert(alive, Equals, false)
 }
+
+func (s *StateSuite) TestAddRelation(c *C) {
+	dummy := s.addDummyCharm(c)
+	// Provider and requirer.
+	s.st.AddService("mysqldb", dummy)
+	s.st.AddService("wordpress", dummy)
+	mysqlep := state.RelationEndpoint{"mysqldb", "blog", "db", state.RoleProvider, state.ScopeGlobal}
+	blogep := state.RelationEndpoint{"wordpress", "blog", "db", state.RoleRequirer, state.ScopeGlobal}
+	relation, serviceRelations, err := s.st.AddRelation(blogep, mysqlep)
+	c.Assert(err, IsNil)
+	c.Assert(relation, NotNil)
+	c.Assert(serviceRelations, HasLen, 2)
+	c.Assert(serviceRelations[0].RelationScope(), Equals, state.ScopeGlobal)
+	c.Assert(serviceRelations[0].RelationRole(), Equals, state.RoleRequirer)
+	c.Assert(serviceRelations[1].RelationScope(), Equals, state.ScopeGlobal)
+	c.Assert(serviceRelations[1].RelationRole(), Equals, state.RoleProvider)
+	c.Assert(serviceRelations[0].RelationName(), Equals, serviceRelations[1].RelationName())
+	// Peer.
+	s.st.AddService("riak", dummy)
+	riakep := state.RelationEndpoint{"riak", "ring", "cache", state.RolePeer, state.ScopeGlobal}
+	relation, serviceRelations, err = s.st.AddRelation(riakep)
+	c.Assert(err, IsNil)
+	c.Assert(relation, NotNil)
+	c.Assert(serviceRelations, HasLen, 1)
+	c.Assert(serviceRelations[0].RelationScope(), Equals, state.ScopeGlobal)
+	c.Assert(serviceRelations[0].RelationRole(), Equals, state.RolePeer)
+	c.Assert(serviceRelations[0].RelationName(), Equals, "cache")
+
+}
+
+func (s *StateSuite) TestAddRelationMissingService(c *C) {
+	dummy := s.addDummyCharm(c)
+	s.st.AddService("mysqldb", dummy)
+	mysqlep := state.RelationEndpoint{"mysqldb", "blog", "db", state.RoleProvider, state.ScopeGlobal}
+	blogep := state.RelationEndpoint{"wordpress", "blog", "db", state.RoleRequirer, state.ScopeGlobal}
+	_, _, err := s.st.AddRelation(blogep, mysqlep)
+	c.Assert(err, ErrorMatches, `service with name "wordpress" not found`)
+}
+
+func (s *StateSuite) TestAddRelationMissingEndpoint(c *C) {
+	dummy := s.addDummyCharm(c)
+	s.st.AddService("mysqldb", dummy)
+	mysqlep := state.RelationEndpoint{"mysqldb", "blog", "db", state.RoleProvider, state.ScopeGlobal}
+	_, _, err := s.st.AddRelation(mysqlep)
+	c.Assert(err, ErrorMatches, `can't add non-peer relation with a single service`)
+}
+
+func (s *StateSuite) TestAddClientServerDifferentRoles(c *C) {
+	dummy := s.addDummyCharm(c)
+	s.st.AddService("mysqldb", dummy)
+	s.st.AddService("riak", dummy)
+	mysqlep := state.RelationEndpoint{"mysqldb", "ifce", "db", state.RoleRequirer, state.ScopeGlobal}
+	riakep := state.RelationEndpoint{"riak", "ring", "cache", state.RolePeer, state.ScopeGlobal}
+	_, _, err := s.st.AddRelation(mysqlep, riakep)
+	c.Assert(err, ErrorMatches, `can't add relation between mysqldb:db and riak:cache`)
+}
+
+func (s *StateSuite) TestAddRelationDifferentInterfaces(c *C) {
+	dummy := s.addDummyCharm(c)
+	s.st.AddService("mysqldb", dummy)
+	s.st.AddService("wordpress", dummy)
+	mysqlep := state.RelationEndpoint{"mysqldb", "ifce-a", "db", state.RoleProvider, state.ScopeGlobal}
+	blogep := state.RelationEndpoint{"wordpress", "ifce-b", "db", state.RoleRequirer, state.ScopeGlobal}
+	_, _, err := s.st.AddRelation(blogep, mysqlep)
+	c.Assert(err, ErrorMatches, `can't add relation between wordpress:db and mysqldb:db`)
+}
+
+func (s *StateSuite) TestAddClientServerRelationTwice(c *C) {
+	dummy := s.addDummyCharm(c)
+	// Provider and requirer.
+	s.st.AddService("mysqldb", dummy)
+	s.st.AddService("wordpress", dummy)
+	mysqlep := state.RelationEndpoint{"mysqldb", "blog", "db", state.RoleProvider, state.ScopeGlobal}
+	blogep := state.RelationEndpoint{"wordpress", "blog", "db", state.RoleRequirer, state.ScopeGlobal}
+	_, _, err := s.st.AddRelation(blogep, mysqlep)
+	c.Assert(err, IsNil)
+	_, _, err = s.st.AddRelation(blogep, mysqlep)
+	c.Assert(err, ErrorMatches, `relation already exists`)
+	// Peer.
+	s.st.AddService("riak", dummy)
+	riakep := state.RelationEndpoint{"riak", "ring", "cache", state.RolePeer, state.ScopeGlobal}
+	_, _, err = s.st.AddRelation(riakep)
+	c.Assert(err, IsNil)
+	_, _, err = s.st.AddRelation(riakep)
+	c.Assert(err, ErrorMatches, `relation already exists`)
+}
+
+func (s *StateSuite) TestAddPeerRelationIllegalEndpointNumber(c *C) {
+	dummy := s.addDummyCharm(c)
+	s.st.AddService("mysqldb", dummy)
+	s.st.AddService("wordpress", dummy)
+	s.st.AddService("riak", dummy)
+	mysqlep := state.RelationEndpoint{"mysqldb", "ifce", "cache", state.RoleProvider, state.ScopeGlobal}
+	blogep := state.RelationEndpoint{"wordpress", "ifce", "cache", state.RoleRequirer, state.ScopeGlobal}
+	riakep := state.RelationEndpoint{"riak", "blog", "cache", state.RolePeer, state.ScopeGlobal}
+	_, _, err := s.st.AddRelation()
+	c.Assert(err, ErrorMatches, `can't add relations between 0 services`)
+	_, _, err = s.st.AddRelation(mysqlep, blogep, riakep)
+	c.Assert(err, ErrorMatches, `can't add relations between 3 services`)
+}