From 637e89e075c9724c7bb7e446014c991c5f2f498d Mon Sep 17 00:00:00 2001
From: Joseph Phillips <joe.phillips@canonical.com>
Date: Tue, 19 May 2020 11:31:02 +0200
Subject: [PATCH] Hits the DB exacly once for subnets when calling
 Space.NetworkSpace or Space.AllSpaceInfos.

This will be faster than using Space.Subnets, which hits the DB multiple
times when there are any subnets in the space.
---
 state/spaces.go      | 45 +++++++++++++++++++++++++++++---------------
 state/spaces_test.go | 33 ++++++++++----------------------
 2 files changed, 40 insertions(+), 38 deletions(-)

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)