diff --git a/state/spaces.go b/state/spaces.go index 959fb371483e..d800bcc94bc4 100644 --- a/state/spaces.go +++ b/state/spaces.go @@ -67,6 +67,8 @@ func (s *Space) ProviderId() network.Id { } // Subnets returns all the subnets associated with the Space. +// TODO (manadart 2020-05-19): Phase out usage of this method. +// Prefer NetworkSpace for retrieving space subnet data. func (s *Space) Subnets() ([]*Subnet, error) { id := s.Id() @@ -96,26 +98,38 @@ func (s *Space) Subnets() ([]*Subnet, error) { } // NetworkSpace maps the space fields into a network.SpaceInfo. +// This method materialises subnets for each call. +// If calling multiple times, consider using AllSpaceInfos and filtering +// in-place. func (s *Space) NetworkSpace() (network.SpaceInfo, error) { - subnets, err := s.Subnets() + subs, err := s.st.AllSubnetInfos() if err != nil { return network.SpaceInfo{}, errors.Trace(err) } - mappedSubnets := make([]network.SubnetInfo, len(subnets)) - for i, subnet := range subnets { - mappedSubnet := subnet.NetworkSubnet() - mappedSubnet.SpaceID = s.Id() - mappedSubnet.SpaceName = s.Name() - mappedSubnet.ProviderSpaceId = s.ProviderId() - mappedSubnets[i] = mappedSubnet + space, err := s.networkSpace(subs) + return space, errors.Trace(err) +} + +// networkSpace transforms a Space into a network.SpaceInfo using the +// materialised subnet information. +func (s *Space) networkSpace(subnets network.SubnetInfos) (network.SpaceInfo, error) { + spaceSubs, err := subnets.GetBySpaceID(s.Id()) + if err != nil { + return network.SpaceInfo{}, errors.Trace(err) + } + + for i := range spaceSubs { + spaceSubs[i].SpaceID = s.Id() + spaceSubs[i].SpaceName = s.Name() + spaceSubs[i].ProviderSpaceId = s.ProviderId() } return network.SpaceInfo{ ID: s.Id(), Name: network.SpaceName(s.Name()), ProviderId: s.ProviderId(), - Subnets: mappedSubnets, + Subnets: spaceSubs, }, nil } @@ -286,19 +300,20 @@ func (st *State) SpaceByName(name string) (*Space, error) { } // AllSpaceInfos returns SpaceInfos for all spaces in the model. -// TODO (manadart 2020-04-09): Instead of calling NetworkSpace here, -// load all the SubnetInfos once and retrieve the space subnets and -// overlays from there. -// This method is inefficient in that it hits the DB 1-2 times for -// each space to retrieve subnets. func (st *State) AllSpaceInfos() (network.SpaceInfos, error) { spaces, err := st.AllSpaces() if err != nil { return nil, errors.Trace(err) } + + subs, err := st.AllSubnetInfos() + if err != nil { + return nil, errors.Trace(err) + } + result := make(network.SpaceInfos, len(spaces)) for i, space := range spaces { - if result[i], err = space.NetworkSpace(); err != nil { + if result[i], err = space.networkSpace(subs); err != nil { return nil, err } } diff --git a/state/spaces_test.go b/state/spaces_test.go index c764382a42b3..0344c1d03049 100644 --- a/state/spaces_test.go +++ b/state/spaces_test.go @@ -6,7 +6,6 @@ package state_test import ( "fmt" "net" - "sort" "github.com/juju/errors" jc "github.com/juju/testing/checkers" @@ -683,10 +682,18 @@ func (s *SpacesSuite) TestSpaceToNetworkSpace(c *gc.C) { } // Sort subnets by CIDR to avoid flaky tests - sort.Slice(spaceInfo.Subnets, func(l, r int) bool { return spaceInfo.Subnets[l].CIDR < spaceInfo.Subnets[r].CIDR }) - sort.Slice(expSpaceInfo.Subnets, func(l, r int) bool { return expSpaceInfo.Subnets[l].CIDR < expSpaceInfo.Subnets[r].CIDR }) + corenetwork.SortSubnetInfos(spaceInfo.Subnets) + corenetwork.SortSubnetInfos(expSpaceInfo.Subnets) c.Assert(spaceInfo, gc.DeepEquals, expSpaceInfo) + + // Test that AllSpaceInfos works the same way. + allSpaceInfos, err := s.State.AllSpaceInfos() + c.Assert(err, jc.ErrorIsNil) + + space1 := allSpaceInfos.GetByName("space1") + c.Assert(space1, gc.NotNil) + c.Assert(*space1, gc.DeepEquals, expSpaceInfo) } type SpacesDiscoverySuite struct { @@ -851,26 +858,6 @@ func checkSubnetsEqual(c *gc.C, subnets []*state.Subnet, subnetInfos []network.S } } -func checkSpacesEqual(c *gc.C, actual []*state.Space, expected []network.SpaceInfo) { - // Filter out the default space for comparisons. - filtered := actual[:0] - for _, s := range actual { - if s.Name() != network.AlphaSpaceName { - filtered = append(filtered, s) - } - } - - c.Assert(len(filtered), gc.Equals, len(expected)) - for i, spaceInfo := range expected { - space := filtered[i] - c.Check(string(spaceInfo.Name), gc.Equals, space.Name()) - c.Check(spaceInfo.ProviderId, gc.Equals, space.ProviderId()) - subnets, err := space.Subnets() - c.Assert(err, jc.ErrorIsNil) - checkSubnetsEqual(c, subnets, spaceInfo.Subnets) - } -} - func (s *SpacesDiscoverySuite) TestSaveProviderSubnets(c *gc.C) { err := s.State.SaveProviderSubnets(twoSubnets, "") c.Check(err, jc.ErrorIsNil)