pyjuju

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

  1. go-state-topology-relation-endpoints
  2. Merge into go
Proposed by Frank Mueller
Status: Merged
Approved by: Gustavo Niemeyer
Approved revision: 162
Merged at revision: 190
Proposed branch: lp:~themue/pyjuju/go-state-topology-relation-endpoints
Merge into: lp:pyjuju/go
Prerequisite: lp:~themue/pyjuju/go-state-topology-relations-without-endpoints
Diff against target: 487 lines (+267/-37)
3 files modified
state/internal_test.go (+188/-24)
state/relation.go (+5/-0)
state/topology.go (+74/-13)
To merge this branch: bzr merge lp:~themue/pyjuju/go-state-topology-relation-endpoints
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+105156@code.launchpad.net

Description of the change

Added methods for relation endpoints to topology.

Relation endpoints needed two methods for testing and
key retrieving in topology.

https://codereview.appspot.com/6198055/

To post a comment you must log in.
Revision history for this message
William Reade (fwereade) wrote :

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

https://codereview.appspot.com/6198055/diff/1/state/topology.go#newcode550
state/topology.go:550: if service == nil || service.Name !=
e.RelationName || scope != e.RelationScope {
I'm pretty sure that "service.Name != e.RelationName" is either insane,
or it betrays a serious misnaming issue somewhere.

That said, per prereq review, I think we should drop RelationName
anyway.

https://codereview.appspot.com/6198055/diff/1/state/topology.go#newcode563
state/topology.go:563: // between "endpoints" or "found" will be false.
I'm -1 on the ... here, I think it's a wart in the python version and
I'd prefer not to duplicate it in Go. Also, it feels like "found" is
redundant -- can't we just return a key of ""? How about:

func (t *topology) RelationKey(ep1, ep2 RelationEndpoint) (string,
error)

func (t *topology) PeerRelationKey(ep RelationEndpoint) (string, error)

...and, given that, I think that HasRelationBetweenEndpoints is actually
completely redundant.

https://codereview.appspot.com/6198055/diff/1/state/topology.go#newcode576
state/topology.go:576: for key, relation := range t.topology.Relations {
Please double-check why the implementation here is different to the one
in HasRelationBetweenEndpoints... by gut says that this is evidence of a
bug somewhere.

https://codereview.appspot.com/6198055/

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

https://codereview.appspot.com/6198055/diff/5001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/5001/state/topology.go#newcode545
state/topology.go:545: return "", nil
If the developer asks for a RelationKey, and it doesn't exist, an error
should be returned. Silently returning nothing and hoping that the
developer remembers to check for the case where it doesn't exist every
time is extremely error prone.

Imagine if you had to do this:

     f, err := os.Open(filename)
     if err != nil {
          ...
     }
     if f != nil {
          ...
     }
     ...

Every time!

The documentation should reflect the change too.

https://codereview.appspot.com/6198055/diff/5001/state/topology.go#newcode571
state/topology.go:571: return "", nil
Ditto.

https://codereview.appspot.com/6198055/

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

https://codereview.appspot.com/6198055/diff/8002/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/8002/state/topology.go#newcode20
state/topology.go:20: errRelationDoesNotExist = errors.New("relation
does not exist")
If we define that error more nicely, we can allow it to pop out to of
the state package when using it. For example:

type noRelationError struct {
     endpoint1, endpoint2 RelationEndpoint
}

func (e *noRelationError) Error() string {
     if e.endpoint2 != nil {
         return fmt.Errorf("state: no peer relation for %s", (what
here?))
     } else {
         return fmt.Errorf("state: no relation between %s and %s", (what
here?))
     }
}

Not sure about the details of the error messages, but it should be
something nice that defines the endpoints.

What do you think?

https://codereview.appspot.com/6198055/

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

https://codereview.appspot.com/6198055/

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

Also in a good direction, thanks Frank.

https://codereview.appspot.com/6198055/diff/9002/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6198055/diff/9002/state/relation.go#newcode55
state/relation.go:55: func (e *RelationEndpoint) String() string {
That's somewhat unexpected. What's the goal here?

I think I'd prefer to leave this out. %#v and %v are simpler and produce
better results than this as it is.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode17
state/topology.go:17: // consumer endpoints of for one peer endpoint
does not exist.
// NoRelationError represents a relation not found for one or more
endpoints.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode19
state/topology.go:19: Endpoint1 *RelationEndpoint
I suggest using this instead:

     Endpoints []RelationEndpoint

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode474
state/topology.go:474: // interface between two endpoints. If it doesn't
exist an error is returned.
// RelationKey returns the key for the relation established between the
// provided endpoints. If no matching relation is found, error will be
// of type *NoRelationError.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode475
state/topology.go:475: func (t *topology) RelationKey(ep1, ep2
RelationEndpoint) (string, error) {
I think the design we had is better one here:

RelationKey(endpoints ...RelationEndpoint)

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode494
state/topology.go:494: if ep1.Interface != relation.Interface ||
ep2.Interface != relation.Interface {
If ep1 and ep2 have diverging interfaces, we can return an error before
even starting to search.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode499
state/topology.go:499: }
Relation names matter as well.

https://codereview.appspot.com/6198055/diff/9002/state/topology.go#newcode506
state/topology.go:506: func (t *topology) PeerRelationKey(ep
RelationEndpoint) (string, error) {
We can subsume that into RelationKey as well.

https://codereview.appspot.com/6198055/

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

Moving it back onto WIP.

https://codereview.appspot.com/6198055/

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

https://codereview.appspot.com/6198055/diff/27001/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/27001/state/topology.go#newcode471
state/topology.go:471: return "", fmt.Errorf("state: differing
interfaces %q and %q", endpoints[0].Interface, endpoints[1].Interface)
What about just returning NoRelationError?

https://codereview.appspot.com/6198055/diff/27001/state/topology.go#newcode474
state/topology.go:474: return "", fmt.Errorf("state: illegal number of
endpoints passed")
s/passed/provided/

https://codereview.appspot.com/6198055/diff/27001/state/topology.go#newcode491
state/topology.go:491: if serviceName != endpoint.RelationName {
Huh? That's quite wrong.

https://codereview.appspot.com/6198055/

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

LGTM, with a few trivials:

https://codereview.appspot.com/6198055/diff/25003/state/topology.go
File state/topology.go (right):

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode29
state/topology.go:29: panic("state: illegal relation error")
s/ error//

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode73
state/topology.go:73: ServiceKey string
s/ServiceKey/Service/; see the convention in the other types.

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode74
state/topology.go:74: RelationName string
Please add a tag "relation-name" so we follow the naming convention too.

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode430
state/topology.go:430: return fmt.Errorf("provider and consumer keys
must not be the same")
s/consumer/requirer/

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode474
state/topology.go:474: if t.topology.Relations == nil {
Shouldn't the logic below work fine if this is dropped?

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode485
state/topology.go:485: return "", fmt.Errorf("state: illegal number of
endpoints provided")
s/endpoints/relation endpoints/

https://codereview.appspot.com/6198055/diff/25003/state/topology.go#newcode498
state/topology.go:498: if service.RelationName != endpoint.RelationName
{
Both of these if blocks can be made into one:

if !ok || ... {

https://codereview.appspot.com/6198055/

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

LGTM

https://codereview.appspot.com/6198055/

lp:~themue/pyjuju/go-state-topology-relation-endpoints updated
163. By Frank Mueller

Merged trunk.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'state/internal_test.go'
--- state/internal_test.go 2012-05-25 14:10:49 +0000
+++ state/internal_test.go 2012-05-29 18:02:21 +0000
@@ -1,6 +1,3 @@
1// launchpad.net/juju/go/state
2//
3// Copyright (c) 2011-2012 Canonical Ltd.
4package state1package state
52
6import (3import (
@@ -461,12 +458,12 @@
461 s.t.AddRelation("r-1", &zkRelation{458 s.t.AddRelation("r-1", &zkRelation{
462 Interface: "ifce",459 Interface: "ifce",
463 Scope: ScopeGlobal,460 Scope: ScopeGlobal,
464 Services: map[RelationRole]string{RolePeer: "s-p"},461 Services: map[RelationRole]*zkRelationService{RolePeer: &zkRelationService{"s-p", "cache"}},
465 })462 })
466 relation, err = s.t.Relation("r-1")463 relation, err = s.t.Relation("r-1")
467 c.Assert(err, IsNil)464 c.Assert(err, IsNil)
468 c.Assert(relation, NotNil)465 c.Assert(relation, NotNil)
469 c.Assert(relation.Services[RolePeer], Equals, "s-p")466 c.Assert(relation.Services[RolePeer].Service, Equals, "s-p")
470}467}
471468
472func (s *TopologySuite) TestAddRelation(c *C) {469func (s *TopologySuite) TestAddRelation(c *C) {
@@ -480,61 +477,82 @@
480 err = s.t.AddRelation("r-1", &zkRelation{477 err = s.t.AddRelation("r-1", &zkRelation{
481 Interface: "ifce",478 Interface: "ifce",
482 Scope: ScopeGlobal,479 Scope: ScopeGlobal,
483 Services: map[RelationRole]string{RoleProvider: "s-p", RoleRequirer: "s-r"},480 Services: map[RelationRole]*zkRelationService{
481 RoleProvider: &zkRelationService{"s-p", "db"},
482 RoleRequirer: &zkRelationService{"s-r", "db"},
483 },
484 })484 })
485 c.Assert(err, IsNil)485 c.Assert(err, IsNil)
486 relation, err = s.t.Relation("r-1")486 relation, err = s.t.Relation("r-1")
487 c.Assert(err, IsNil)487 c.Assert(err, IsNil)
488 c.Assert(relation, NotNil)488 c.Assert(relation, NotNil)
489 c.Assert(relation.Services[RoleProvider], Equals, "s-p")489 c.Assert(relation.Services[RoleProvider].Service, Equals, "s-p")
490 c.Assert(relation.Services[RoleRequirer], Equals, "s-r")490 c.Assert(relation.Services[RoleRequirer].Service, Equals, "s-r")
491491
492 err = s.t.AddRelation("r-2", &zkRelation{492 err = s.t.AddRelation("r-2", &zkRelation{
493 Interface: "",493 Interface: "",
494 Scope: ScopeGlobal,494 Scope: ScopeGlobal,
495 Services: map[RelationRole]string{RoleProvider: "s-p", RoleRequirer: "s-r"},495 Services: map[RelationRole]*zkRelationService{
496 RoleProvider: &zkRelationService{"s-p", "db"},
497 RoleRequirer: &zkRelationService{"s-r", "db"},
498 },
496 })499 })
497 c.Assert(err, ErrorMatches, `relation interface is empty`)500 c.Assert(err, ErrorMatches, `relation interface is empty`)
498501
499 err = s.t.AddRelation("r-3", &zkRelation{502 err = s.t.AddRelation("r-3", &zkRelation{
500 Interface: "ifce",503 Interface: "ifce",
501 Scope: ScopeGlobal,504 Scope: ScopeGlobal,
502 Services: map[RelationRole]string{},505 Services: map[RelationRole]*zkRelationService{},
503 })506 })
504 c.Assert(err, ErrorMatches, `relation has no services`)507 c.Assert(err, ErrorMatches, `relation has no services`)
505508
506 err = s.t.AddRelation("r-4", &zkRelation{509 err = s.t.AddRelation("r-4", &zkRelation{
507 Interface: "ifce",510 Interface: "ifce",
508 Scope: ScopeGlobal,511 Scope: ScopeGlobal,
509 Services: map[RelationRole]string{RoleProvider: "s-p"},512 Services: map[RelationRole]*zkRelationService{
513 RoleProvider: &zkRelationService{"s-p", "db"},
514 },
510 })515 })
511 c.Assert(err, ErrorMatches, `relation has provider but no requirer`)516 c.Assert(err, ErrorMatches, `relation has provider but no requirer`)
512517
513 err = s.t.AddRelation("r-5", &zkRelation{518 err = s.t.AddRelation("r-5", &zkRelation{
514 Interface: "ifce",519 Interface: "ifce",
515 Scope: ScopeGlobal,520 Scope: ScopeGlobal,
516 Services: map[RelationRole]string{RoleProvider: "s-p", RolePeer: "s-r"},521 Services: map[RelationRole]*zkRelationService{
522 RoleProvider: &zkRelationService{"s-p", "db"},
523 RolePeer: &zkRelationService{"s-r", "db"},
524 },
517 })525 })
518 c.Assert(err, ErrorMatches, `relation has provider but no requirer`)526 c.Assert(err, ErrorMatches, `relation has provider but no requirer`)
519527
520 err = s.t.AddRelation("r-6", &zkRelation{528 err = s.t.AddRelation("r-6", &zkRelation{
521 Interface: "ifce",529 Interface: "ifce",
522 Scope: ScopeGlobal,530 Scope: ScopeGlobal,
523 Services: map[RelationRole]string{RoleProvider: "s-p", RoleRequirer: "s-r", RolePeer: "s-r"},531 Services: map[RelationRole]*zkRelationService{
532 RoleProvider: &zkRelationService{"s-p", "db"},
533 RoleRequirer: &zkRelationService{"s-r", "db"},
534 RolePeer: &zkRelationService{"s-r", "db"},
535 },
524 })536 })
525 c.Assert(err, ErrorMatches, `relation with mixed peer, provider, and requirer roles`)537 c.Assert(err, ErrorMatches, `relation with mixed peer, provider, and requirer roles`)
526538
527 err = s.t.AddRelation("r-7", &zkRelation{539 err = s.t.AddRelation("r-7", &zkRelation{
528 Interface: "ifce",540 Interface: "ifce",
529 Scope: ScopeGlobal,541 Scope: ScopeGlobal,
530 Services: map[RelationRole]string{RoleProvider: "s-p", RoleRequirer: "illegal"},542 Services: map[RelationRole]*zkRelationService{
543 RoleProvider: &zkRelationService{"s-p", "db"},
544 RoleRequirer: &zkRelationService{"illegal", "db"},
545 },
531 })546 })
532 c.Assert(err, ErrorMatches, `service with key "illegal" not found`)547 c.Assert(err, ErrorMatches, `service with key "illegal" not found`)
533548
534 err = s.t.AddRelation("r-1", &zkRelation{549 err = s.t.AddRelation("r-1", &zkRelation{
535 Interface: "ifce",550 Interface: "ifce",
536 Scope: ScopeGlobal,551 Scope: ScopeGlobal,
537 Services: map[RelationRole]string{RoleProvider: "s-p", RoleRequirer: "s-r"},552 Services: map[RelationRole]*zkRelationService{
553 RoleProvider: &zkRelationService{"s-p", "db"},
554 RoleRequirer: &zkRelationService{"s-r", "db"},
555 },
538 })556 })
539 c.Assert(err, ErrorMatches, `relation key "r-1" already in use`)557 c.Assert(err, ErrorMatches, `relation key "r-1" already in use`)
540}558}
@@ -548,7 +566,9 @@
548 s.t.AddRelation("r-1", &zkRelation{566 s.t.AddRelation("r-1", &zkRelation{
549 Interface: "ifce",567 Interface: "ifce",
550 Scope: ScopeGlobal,568 Scope: ScopeGlobal,
551 Services: map[RelationRole]string{RolePeer: "s-p"},569 Services: map[RelationRole]*zkRelationService{
570 RolePeer: &zkRelationService{"s-p", "cache"},
571 },
552 })572 })
553 keys = s.t.RelationKeys()573 keys = s.t.RelationKeys()
554 c.Assert(keys, DeepEquals, []string{"r-1"})574 c.Assert(keys, DeepEquals, []string{"r-1"})
@@ -556,7 +576,9 @@
556 s.t.AddRelation("r-2", &zkRelation{576 s.t.AddRelation("r-2", &zkRelation{
557 Interface: "ifce",577 Interface: "ifce",
558 Scope: ScopeGlobal,578 Scope: ScopeGlobal,
559 Services: map[RelationRole]string{RolePeer: "s-p"},579 Services: map[RelationRole]*zkRelationService{
580 RolePeer: &zkRelationService{"s-p", "cache"},
581 },
560 })582 })
561 keys = s.t.RelationKeys()583 keys = s.t.RelationKeys()
562 c.Assert(keys, DeepEquals, []string{"r-1", "r-2"})584 c.Assert(keys, DeepEquals, []string{"r-1", "r-2"})
@@ -572,12 +594,16 @@
572 s.t.AddRelation("r-0", &zkRelation{594 s.t.AddRelation("r-0", &zkRelation{
573 Interface: "ifce0",595 Interface: "ifce0",
574 Scope: ScopeGlobal,596 Scope: ScopeGlobal,
575 Services: map[RelationRole]string{RolePeer: "s-p"},597 Services: map[RelationRole]*zkRelationService{
598 RolePeer: &zkRelationService{"s-p", "cache"},
599 },
576 })600 })
577 s.t.AddRelation("r-1", &zkRelation{601 s.t.AddRelation("r-1", &zkRelation{
578 Interface: "ifce1",602 Interface: "ifce1",
579 Scope: ScopeGlobal,603 Scope: ScopeGlobal,
580 Services: map[RelationRole]string{RolePeer: "s-p"},604 Services: map[RelationRole]*zkRelationService{
605 RolePeer: &zkRelationService{"s-p", "cache"},
606 },
581 })607 })
582 relations, err = s.t.RelationsForService("s-p")608 relations, err = s.t.RelationsForService("s-p")
583 c.Assert(err, IsNil)609 c.Assert(err, IsNil)
@@ -594,21 +620,24 @@
594620
595func (s *TopologySuite) TestRemoveRelation(c *C) {621func (s *TopologySuite) TestRemoveRelation(c *C) {
596 // Check that removing of a relation works.622 // Check that removing of a relation works.
597 s.t.AddService("s-c", "wordpress")623 s.t.AddService("s-r", "wordpress")
598 s.t.AddService("s-p", "mysql")624 s.t.AddService("s-p", "mysql")
599625
600 err := s.t.AddRelation("r-1", &zkRelation{626 err := s.t.AddRelation("r-1", &zkRelation{
601 Interface: "ifce",627 Interface: "ifce",
602 Scope: ScopeGlobal,628 Scope: ScopeGlobal,
603 Services: map[RelationRole]string{RoleProvider: "s-p", RoleRequirer: "s-c"},629 Services: map[RelationRole]*zkRelationService{
630 RoleProvider: &zkRelationService{"s-p", "db"},
631 RoleRequirer: &zkRelationService{"s-r", "db"},
632 },
604 })633 })
605 c.Assert(err, IsNil)634 c.Assert(err, IsNil)
606635
607 relation, err := s.t.Relation("r-1")636 relation, err := s.t.Relation("r-1")
608 c.Assert(err, IsNil)637 c.Assert(err, IsNil)
609 c.Assert(relation, NotNil)638 c.Assert(relation, NotNil)
610 c.Assert(relation.Services[RoleProvider], Equals, "s-p")639 c.Assert(relation.Services[RoleProvider].Service, Equals, "s-p")
611 c.Assert(relation.Services[RoleRequirer], Equals, "s-c")640 c.Assert(relation.Services[RoleRequirer].Service, Equals, "s-r")
612641
613 s.t.RemoveRelation("r-1")642 s.t.RemoveRelation("r-1")
614643
@@ -624,13 +653,148 @@
624 s.t.AddRelation("r-1", &zkRelation{653 s.t.AddRelation("r-1", &zkRelation{
625 Interface: "ifce",654 Interface: "ifce",
626 Scope: ScopeGlobal,655 Scope: ScopeGlobal,
627 Services: map[RelationRole]string{RolePeer: "s-p"},656 Services: map[RelationRole]*zkRelationService{
657 RolePeer: &zkRelationService{"s-p", "cache"},
658 },
628 })659 })
629660
630 err := s.t.RemoveService("s-p")661 err := s.t.RemoveService("s-p")
631 c.Assert(err, ErrorMatches, `cannot remove service "s-p" with active relations`)662 c.Assert(err, ErrorMatches, `cannot remove service "s-p" with active relations`)
632}663}
633664
665func (s *TopologySuite) TestRelationKeyEndpoints(c *C) {
666 mysqlep1 := RelationEndpoint{"mysql", "ifce1", "db", RoleProvider, ScopeGlobal}
667 blogep1 := RelationEndpoint{"wordpress", "ifce1", "db", RoleRequirer, ScopeGlobal}
668 mysqlep2 := RelationEndpoint{"mysql", "ifce2", "db", RoleProvider, ScopeGlobal}
669 blogep2 := RelationEndpoint{"wordpress", "ifce2", "db", RoleRequirer, ScopeGlobal}
670 mysqlep3 := RelationEndpoint{"mysql", "ifce3", "db", RoleProvider, ScopeGlobal}
671 blogep3 := RelationEndpoint{"wordpress", "ifce3", "db", RoleRequirer, ScopeGlobal}
672 s.t.AddService("s-r", "wordpress")
673 s.t.AddService("s-p", "mysql")
674 s.t.AddRelation("r-0", &zkRelation{
675 Interface: "ifce1",
676 Scope: ScopeGlobal,
677 Services: map[RelationRole]*zkRelationService{
678 RoleProvider: &zkRelationService{"s-p", "db"},
679 RoleRequirer: &zkRelationService{"s-r", "db"},
680 },
681 })
682 s.t.AddRelation("r-1", &zkRelation{
683 Interface: "ifce2",
684 Scope: ScopeGlobal,
685 Services: map[RelationRole]*zkRelationService{
686 RoleProvider: &zkRelationService{"s-p", "db"},
687 RoleRequirer: &zkRelationService{"s-r", "db"},
688 },
689 })
690
691 // Valid relations.
692 key, err := s.t.RelationKey(mysqlep1, blogep1)
693 c.Assert(err, IsNil)
694 c.Assert(key, Equals, "r-0")
695 key, err = s.t.RelationKey(blogep1, mysqlep1)
696 c.Assert(err, IsNil)
697 c.Assert(key, Equals, "r-0")
698 key, err = s.t.RelationKey(mysqlep2, blogep2)
699 c.Assert(err, IsNil)
700 c.Assert(key, Equals, "r-1")
701 key, err = s.t.RelationKey(blogep2, mysqlep2)
702 c.Assert(err, IsNil)
703 c.Assert(key, Equals, "r-1")
704
705 // Endpoints without relation.
706 _, err = s.t.RelationKey(mysqlep3, blogep3)
707 c.Assert(err, ErrorMatches, `state: no relation between "mysql:db" and "wordpress:db"`)
708
709 // Mix of endpoints of two relations.
710 _, err = s.t.RelationKey(mysqlep1, blogep2)
711 c.Assert(err, ErrorMatches, `state: no relation between "mysql:db" and "wordpress:db"`)
712
713 // Illegal number of endpoints.
714 _, err = s.t.RelationKey()
715 c.Assert(err, ErrorMatches, `state: illegal number of relation endpoints provided`)
716 _, err = s.t.RelationKey(mysqlep1, mysqlep2, blogep1)
717 c.Assert(err, ErrorMatches, `state: illegal number of relation endpoints provided`)
718}
719
720func (s *TopologySuite) TestRelationKeyIllegalEndpoints(c *C) {
721 mysqlep1 := RelationEndpoint{"mysql", "ifce", "db", RoleProvider, ScopeGlobal}
722 blogep1 := RelationEndpoint{"wordpress", "ifce", "db", RoleRequirer, ScopeGlobal}
723 mysqlep2 := RelationEndpoint{"illegal-mysql", "ifce", "db", RoleProvider, ScopeGlobal}
724 blogep2 := RelationEndpoint{"illegal-wordpress", "ifce", "db", RoleRequirer, ScopeGlobal}
725 riakep3 := RelationEndpoint{"riak", "ifce", "ring", RolePeer, ScopeGlobal}
726 s.t.AddService("s-r", "wordpress")
727 s.t.AddService("s-p1", "mysql")
728 s.t.AddService("s-p2", "riak")
729 s.t.AddRelation("r-0", &zkRelation{
730 Interface: "ifce1",
731 Scope: ScopeGlobal,
732 Services: map[RelationRole]*zkRelationService{
733 RoleProvider: &zkRelationService{"s-p", "db"},
734 RoleRequirer: &zkRelationService{"s-r", "db"},
735 },
736 })
737
738 key, err := s.t.RelationKey(mysqlep1, blogep2)
739 c.Assert(key, Equals, "")
740 c.Assert(err, ErrorMatches, `state: no relation between "mysql:db" and "illegal-wordpress:db"`)
741 key, err = s.t.RelationKey(mysqlep2, blogep1)
742 c.Assert(key, Equals, "")
743 c.Assert(err, ErrorMatches, `state: no relation between "illegal-mysql:db" and "wordpress:db"`)
744 key, err = s.t.RelationKey(mysqlep1, riakep3)
745 c.Assert(key, Equals, "")
746 c.Assert(err, ErrorMatches, `state: no relation between "mysql:db" and "riak:ring"`)
747}
748
749func (s *TopologySuite) TestPeerRelationKeyEndpoints(c *C) {
750 riakep1 := RelationEndpoint{"riak", "ifce1", "ring", RolePeer, ScopeGlobal}
751 riakep2 := RelationEndpoint{"riak", "ifce2", "ring", RolePeer, ScopeGlobal}
752 riakep3 := RelationEndpoint{"riak", "ifce3", "ring", RolePeer, ScopeGlobal}
753 s.t.AddService("s-p", "ring")
754 s.t.AddRelation("r-0", &zkRelation{
755 Interface: "ifce1",
756 Scope: ScopeGlobal,
757 Services: map[RelationRole]*zkRelationService{
758 RolePeer: &zkRelationService{"s-p", "ring"},
759 },
760 })
761 s.t.AddRelation("r-1", &zkRelation{
762 Interface: "ifce2",
763 Scope: ScopeGlobal,
764 Services: map[RelationRole]*zkRelationService{
765 RolePeer: &zkRelationService{"s-p", "ring"},
766 },
767 })
768
769 // Valid relations.
770 key, err := s.t.RelationKey(riakep1)
771 c.Assert(err, IsNil)
772 c.Assert(key, Equals, "r-0")
773 key, err = s.t.RelationKey(riakep2)
774 c.Assert(err, IsNil)
775 c.Assert(key, Equals, "r-1")
776
777 // Endpoint without relation.
778 key, err = s.t.RelationKey(riakep3)
779 c.Assert(err, ErrorMatches, `state: no peer relation for "riak:ring"`)
780}
781
782func (s *TopologySuite) TestPeerRelationKeyIllegalEndpoints(c *C) {
783 riakep1 := RelationEndpoint{"riak", "ifce", "illegal-ring", RolePeer, ScopeGlobal}
784 s.t.AddService("s-p", "riak")
785 s.t.AddRelation("r-0", &zkRelation{
786 Interface: "ifce",
787 Scope: ScopeGlobal,
788 Services: map[RelationRole]*zkRelationService{
789 RolePeer: &zkRelationService{"s-p", "ring"},
790 },
791 })
792
793 key, err := s.t.RelationKey(riakep1)
794 c.Assert(key, Equals, "")
795 c.Assert(err, ErrorMatches, `state: no peer relation for "riak:illegal-ring"`)
796}
797
634type ConfigNodeSuite struct {798type ConfigNodeSuite struct {
635 zkServer *zookeeper.Server799 zkServer *zookeeper.Server
636 zkTestRoot string800 zkTestRoot string
637801
=== modified file 'state/relation.go'
--- state/relation.go 2012-05-24 18:57:02 +0000
+++ state/relation.go 2012-05-29 18:02:21 +0000
@@ -41,3 +41,8 @@
41 }41 }
42 panic("endpoint role is undefined")42 panic("endpoint role is undefined")
43}43}
44
45// String returns the unique identifier of the relation endpoint.
46func (e RelationEndpoint) String() string {
47 return e.ServiceName + ":" + e.RelationName
48}
4449
=== modified file 'state/topology.go'
--- state/topology.go 2012-05-24 18:57:02 +0000
+++ state/topology.go 2012-05-29 18:02:21 +0000
@@ -13,6 +13,22 @@
13// when we know that a version is in fact actually incompatible.13// when we know that a version is in fact actually incompatible.
14const topologyVersion = 114const topologyVersion = 1
1515
16// NoRelationError represents a relation not found for one or more endpoints.
17type NoRelationError struct {
18 Endpoints []RelationEndpoint
19}
20
21// Error returns the string representation of the error.
22func (e NoRelationError) Error() string {
23 switch len(e.Endpoints) {
24 case 1:
25 return fmt.Sprintf("state: no peer relation for %q", e.Endpoints[0])
26 case 2:
27 return fmt.Sprintf("state: no relation between %q and %q", e.Endpoints[0], e.Endpoints[1])
28 }
29 panic("state: illegal relation")
30}
31
16// zkTopology is used to marshal and unmarshal the content32// zkTopology is used to marshal and unmarshal the content
17// of the /topology node in ZooKeeper.33// of the /topology node in ZooKeeper.
18type zkTopology struct {34type zkTopology struct {
@@ -47,7 +63,15 @@
47type zkRelation struct {63type zkRelation struct {
48 Interface string64 Interface string
49 Scope RelationScope65 Scope RelationScope
50 Services map[RelationRole]string66 Services map[RelationRole]*zkRelationService
67}
68
69// zkRelationService represents the data of one
70// service of a relation within the /topology
71// node in ZooKeeper.
72type zkRelationService struct {
73 Service string
74 RelationName string "relation-name"
51}75}
5276
53// check verifies that r is a proper relation.77// check verifies that r is a proper relation.
@@ -63,9 +87,12 @@
63 RoleProvider: RoleRequirer,87 RoleProvider: RoleRequirer,
64 RolePeer: RolePeer,88 RolePeer: RolePeer,
65 }89 }
66 for serviceRole, serviceKey := range r.Services {90 for serviceRole, service := range r.Services {
67 if serviceKey == "" {91 if service.Service == "" {
68 return fmt.Errorf("relation has %s service with empty key", serviceRole)92 return fmt.Errorf("relation has %s service with empty service key", serviceRole)
93 }
94 if service.RelationName == "" {
95 return fmt.Errorf("relation has %s service with empty relation name", serviceRole)
69 }96 }
70 counterRole, ok := counterpart[serviceRole]97 counterRole, ok := counterpart[serviceRole]
71 if !ok {98 if !ok {
@@ -391,16 +418,16 @@
391 if err := relation.check(); err != nil {418 if err := relation.check(); err != nil {
392 return err419 return err
393 }420 }
394 for _, serviceKey := range relation.Services {421 for _, service := range relation.Services {
395 if err := t.assertService(serviceKey); err != nil {422 if err := t.assertService(service.Service); err != nil {
396 return err423 return err
397 }424 }
398 }425 }
399 if relation.Services[RolePeer] == "" {426 if relation.Services[RolePeer] == nil {
400 providerKey := relation.Services[RoleProvider]427 providerKey := relation.Services[RoleProvider].Service
401 consumerKey := relation.Services[RoleRequirer]428 requirerKey := relation.Services[RoleRequirer].Service
402 if providerKey == consumerKey {429 if providerKey == requirerKey {
403 return fmt.Errorf("provider and consumer keys must not be the same")430 return fmt.Errorf("provider and requirer keys must not be the same")
404 }431 }
405 }432 }
406 t.topology.Relations[relationKey] = relation433 t.topology.Relations[relationKey] = relation
@@ -430,8 +457,8 @@
430 }457 }
431 relations := make(map[string]*zkRelation)458 relations := make(map[string]*zkRelation)
432 for relationKey, relation := range t.topology.Relations {459 for relationKey, relation := range t.topology.Relations {
433 for _, roleServiceKey := range relation.Services {460 for _, service := range relation.Services {
434 if roleServiceKey == serviceKey {461 if service.Service == serviceKey {
435 relations[relationKey] = relation462 relations[relationKey] = relation
436 break463 break
437 }464 }
@@ -440,6 +467,40 @@
440 return relations, nil467 return relations, nil
441}468}
442469
470// RelationKey returns the key for the relation established between the
471// provided endpoints. If no matching relation is found, error will be
472// of type *NoRelationError.
473func (t *topology) RelationKey(endpoints ...RelationEndpoint) (string, error) {
474 switch len(endpoints) {
475 case 1:
476 // Just pass.
477 case 2:
478 if endpoints[0].Interface != endpoints[1].Interface {
479 return "", &NoRelationError{endpoints}
480 }
481 default:
482 return "", fmt.Errorf("state: illegal number of relation endpoints provided")
483 }
484 for relationKey, relation := range t.topology.Relations {
485 if relation.Interface != endpoints[0].Interface {
486 continue
487 }
488 found := true
489 for _, endpoint := range endpoints {
490 service, ok := relation.Services[endpoint.RelationRole]
491 if !ok || service.RelationName != endpoint.RelationName {
492 found = false
493 break
494 }
495 }
496 if found {
497 // All endpoints tested positive.
498 return relationKey, nil
499 }
500 }
501 return "", &NoRelationError{endpoints}
502}
503
443// assertMachine checks if a machine exists.504// assertMachine checks if a machine exists.
444func (t *topology) assertMachine(machineKey string) error {505func (t *topology) assertMachine(machineKey string) error {
445 if _, ok := t.topology.Machines[machineKey]; !ok {506 if _, ok := t.topology.Machines[machineKey]; !ok {

Subscribers

People subscribed via source and target branches