From 6c4b83c119d8d9bbebeb460aaacaa361975f1fe7 Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Fri, 9 Feb 2024 13:00:21 -0500 Subject: [PATCH] Allow reuse of cache indexes (#20562) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously calling `index.New` would return an object with the index information such as the Indexer, whether it was required, and the name of the index as well as a radix tree to store indexed data. Now the main `Index` type doesn’t contain the radix tree for indexed data. Instead the `IndexedData` method can be used to combine the main `Index` with a radix tree in the `IndexedData` structure. The cache still only allows configuring the `Index` type and will invoke the `IndexedData` method on the provided indexes to get the structure that the cache can use for actual data management. All of this makes it now safe to reuse the `index.Index` types. --- .../cache/index/convenience_test.go | 49 +++++++++++++++ internal/controller/cache/index/index.go | 18 +++++- internal/controller/cache/index/index_test.go | 60 +++++++++++++++++-- internal/controller/cache/index/txn.go | 2 +- internal/controller/cache/index/txn_test.go | 4 +- .../cache/indexers/decoded_indexer_test.go | 4 +- .../cache/indexers/id_indexer_test.go | 6 +- .../cache/indexers/ref_indexer_test.go | 4 +- internal/controller/cache/kind.go | 10 ++-- 9 files changed, 135 insertions(+), 22 deletions(-) diff --git a/internal/controller/cache/index/convenience_test.go b/internal/controller/cache/index/convenience_test.go index adb0f3a8e2..f5fe5cae45 100644 --- a/internal/controller/cache/index/convenience_test.go +++ b/internal/controller/cache/index/convenience_test.go @@ -243,6 +243,55 @@ func TestPrefixReferenceOrIDFromArgs(t *testing.T) { }) } +func TestMaybePrefixReferenceOrIDFromArgs(t *testing.T) { + ref := &pbresource.Reference{ + Type: &pbresource.Type{ + Group: "test", + GroupVersion: "v1", + Kind: "fake", + }, + Tenancy: &pbresource.Tenancy{ + Partition: "default", + }, + } + t.Run("invalid length", func(t *testing.T) { + // the second arg will cause a validation error + val, err := MaybePrefixReferenceOrIDFromArgs(ref, IndexQueryOptions{Prefix: false}, 3) + require.Nil(t, val) + require.Error(t, err) + }) + + t.Run("invalid type arg 1", func(t *testing.T) { + val, err := MaybePrefixReferenceOrIDFromArgs("string type unexpected", IndexQueryOptions{Prefix: true}) + require.Nil(t, val) + require.Error(t, err) + }) + + t.Run("invalid type arg 2", func(t *testing.T) { + val, err := MaybePrefixReferenceOrIDFromArgs(ref, "string type unexpected") + require.Nil(t, val) + require.Error(t, err) + }) + + t.Run("ok single arg", func(t *testing.T) { + val, err := MaybePrefixReferenceOrIDFromArgs(ref) + require.NoError(t, err) + require.Equal(t, IndexFromRefOrID(ref), val) + }) + + t.Run("ok two args no prefix", func(t *testing.T) { + val, err := MaybePrefixReferenceOrIDFromArgs(ref, IndexQueryOptions{Prefix: false}) + require.NoError(t, err) + require.Equal(t, IndexFromRefOrID(ref), val) + }) + + t.Run("ok two args with prefix", func(t *testing.T) { + val, err := MaybePrefixReferenceOrIDFromArgs(ref, IndexQueryOptions{Prefix: true}) + require.NoError(t, err) + require.Equal(t, PrefixIndexFromRefOrID(ref), val) + }) +} + func TestSingleValueFromArgs(t *testing.T) { injectedError := errors.New("injected test error") diff --git a/internal/controller/cache/index/index.go b/internal/controller/cache/index/index.go index 66f39ad024..aa3ba047f5 100644 --- a/internal/controller/cache/index/index.go +++ b/internal/controller/cache/index/index.go @@ -12,7 +12,6 @@ type Index struct { name string required bool indexer MultiIndexer - tree *iradix.Tree[[]*pbresource.Resource] } func New(name string, i Indexer, opts ...IndexOption) *Index { @@ -36,7 +35,6 @@ func New(name string, i Indexer, opts ...IndexOption) *Index { idx := &Index{ name: name, indexer: multiIndexer, - tree: iradix.New[[]*pbresource.Resource](), } for _, opt := range opts { @@ -50,7 +48,21 @@ func (i *Index) Name() string { return i.name } -func (i *Index) Txn() Txn { +// IndexedData combines the Index with an radix tree for index and resource storage. +func (i *Index) IndexedData() *IndexedData { + return &IndexedData{ + Index: i, + tree: iradix.New[[]*pbresource.Resource](), + } +} + +// IndexedData is a wrapper around an Index and an radix tree for index and resource storage. +type IndexedData struct { + *Index + tree *iradix.Tree[[]*pbresource.Resource] +} + +func (i *IndexedData) Txn() Txn { return &txn{ inner: i.tree.Txn(), index: i, diff --git a/internal/controller/cache/index/index_test.go b/internal/controller/cache/index/index_test.go index 1f65e6d3ab..d1c492ff0e 100644 --- a/internal/controller/cache/index/index_test.go +++ b/internal/controller/cache/index/index_test.go @@ -10,23 +10,24 @@ import ( "github.com/hashicorp/consul/internal/controller/cache/index/indexmock" "github.com/hashicorp/consul/internal/resource/resourcetest" "github.com/hashicorp/consul/proto-public/pbresource" + "github.com/hashicorp/consul/proto/private/prototest" "github.com/stretchr/testify/require" ) type testSingleIndexer struct{} func (testSingleIndexer) FromArgs(args ...any) ([]byte, error) { - return ReferenceOrIDFromArgs(args) + return ReferenceOrIDFromArgs(args...) } -func (testSingleIndexer) FromResource(*pbresource.Resource) (bool, []byte, error) { - return false, nil, nil +func (testSingleIndexer) FromResource(res *pbresource.Resource) (bool, []byte, error) { + return true, IndexFromRefOrID(res.Id), nil } type testMultiIndexer struct{} func (testMultiIndexer) FromArgs(args ...any) ([]byte, error) { - return ReferenceOrIDFromArgs(args) + return ReferenceOrIDFromArgs(args...) } func (testMultiIndexer) FromResource(*pbresource.Resource) (bool, [][]byte, error) { @@ -135,3 +136,54 @@ func TestSingleIndexWrapper(t *testing.T) { require.Equal(t, []byte{1, 2, 3}, vals[0]) }) } + +func TestIndexReuse(t *testing.T) { + rtype := &pbresource.Type{ + Group: "test", + GroupVersion: "v1", + Kind: "fake", + } + id := resourcetest.Resource(rtype, "foo").ID() + + res1 := resourcetest.ResourceID(id).Build() + res2 := resourcetest.ResourceID(id).WithStatus("foo", &pbresource.Status{ + ObservedGeneration: "woo", + }).Build() + + indexer := testSingleIndexer{} + + // Verify that the indexer produces an identical index for both resources. If this + // isn't true then the rest of the checks we do don't actually prove that the + // two IndexedData objects have independent resource storage. + _, idx1, _ := indexer.FromResource(res1) + _, idx2, _ := indexer.FromResource(res2) + require.Equal(t, idx1, idx2) + + // Create the index and two indepent indexed data storage objects + idx := New("test", indexer) + data1 := idx.IndexedData() + data2 := idx.IndexedData() + + // Push 1 resource into each + txn := data1.Txn() + txn.Insert(res1) + txn.Commit() + + txn = data2.Txn() + txn.Insert(res2) + txn.Commit() + + // Verify that querying the first indexed data can only return the first resource + iter, err := data1.Txn().ListIterator(id) + require.NoError(t, err) + res := iter.Next() + prototest.AssertDeepEqual(t, res1, res) + require.Nil(t, iter.Next()) + + // Verify that querying the second indexed data can only return the second resource + iter, err = data2.Txn().ListIterator(id) + require.NoError(t, err) + res = iter.Next() + prototest.AssertDeepEqual(t, res2, res) + require.Nil(t, iter.Next()) +} diff --git a/internal/controller/cache/index/txn.go b/internal/controller/cache/index/txn.go index 6952c3cb63..2a932e6a20 100644 --- a/internal/controller/cache/index/txn.go +++ b/internal/controller/cache/index/txn.go @@ -12,7 +12,7 @@ import ( type txn struct { inner *iradix.Txn[[]*pbresource.Resource] - index *Index + index *IndexedData dirty bool } diff --git a/internal/controller/cache/index/txn_test.go b/internal/controller/cache/index/txn_test.go index ee8815c4d1..5dfc248137 100644 --- a/internal/controller/cache/index/txn_test.go +++ b/internal/controller/cache/index/txn_test.go @@ -24,7 +24,7 @@ type txnSuite struct { suite.Suite indexer *indexmock.SingleIndexer - index *Index + index *IndexedData r1 *pbresource.Resource r2 *pbresource.Resource @@ -34,7 +34,7 @@ type txnSuite struct { func (suite *txnSuite) SetupTest() { suite.indexer = indexmock.NewSingleIndexer(suite.T()) - suite.index = New("test", suite.indexer, IndexRequired) + suite.index = New("test", suite.indexer, IndexRequired).IndexedData() suite.r1 = testResource("r1") suite.r2 = testResource("r2") diff --git a/internal/controller/cache/indexers/decoded_indexer_test.go b/internal/controller/cache/indexers/decoded_indexer_test.go index c37f0d965b..0fa876c193 100644 --- a/internal/controller/cache/indexers/decoded_indexer_test.go +++ b/internal/controller/cache/indexers/decoded_indexer_test.go @@ -131,7 +131,7 @@ func (suite *decodedSingleIndexerSuite) TestIntegration() { dec := resourcetest.MustDecode[*pbdemo.Album](suite.T(), res) - idx := DecodedSingleIndexer("test", suite.args.Execute, suite.indexer.Execute) + idx := DecodedSingleIndexer("test", suite.args.Execute, suite.indexer.Execute).IndexedData() suite.indexer.EXPECT(). Execute(dec). @@ -263,7 +263,7 @@ func (suite *decodedMultiIndexerSuite) TestIntegration() { dec := resourcetest.MustDecode[*pbdemo.Album](suite.T(), res) - idx := DecodedMultiIndexer("test", suite.args.Execute, suite.indexer.Execute) + idx := DecodedMultiIndexer("test", suite.args.Execute, suite.indexer.Execute).IndexedData() suite.indexer.EXPECT(). Execute(dec). diff --git a/internal/controller/cache/indexers/id_indexer_test.go b/internal/controller/cache/indexers/id_indexer_test.go index a70bd349d1..a5b80e3fdb 100644 --- a/internal/controller/cache/indexers/id_indexer_test.go +++ b/internal/controller/cache/indexers/id_indexer_test.go @@ -17,7 +17,7 @@ import ( ) func TestIDIndex(t *testing.T) { - idx := IDIndex("test", index.IndexRequired) + idx := IDIndex("test", index.IndexRequired).IndexedData() r1 := resourcetest.Resource(pbdemo.AlbumType, "foo"). WithTenancy(&pbresource.Tenancy{ @@ -39,7 +39,7 @@ func TestIDIndex(t *testing.T) { } func TestOwnerIndex(t *testing.T) { - idx := OwnerIndex("test", index.IndexRequired) + idx := OwnerIndex("test", index.IndexRequired).IndexedData() r1 := resourcetest.Resource(pbdemo.AlbumType, "foo"). WithTenancy(&pbresource.Tenancy{ @@ -70,7 +70,7 @@ func TestOwnerIndex(t *testing.T) { func TestSingleIDOrRefIndex(t *testing.T) { getRef := indexersmock.NewGetSingleRefOrID(t) - idx := SingleIDOrRefIndex("test", getRef.Execute) + idx := SingleIDOrRefIndex("test", getRef.Execute).IndexedData() r1 := resourcetest.Resource(pbdemo.AlbumType, "foo").Build() ref := &pbresource.Reference{ diff --git a/internal/controller/cache/indexers/ref_indexer_test.go b/internal/controller/cache/indexers/ref_indexer_test.go index cfd875be42..55657806ec 100644 --- a/internal/controller/cache/indexers/ref_indexer_test.go +++ b/internal/controller/cache/indexers/ref_indexer_test.go @@ -43,7 +43,7 @@ func TestRefOrIDIndex(t *testing.T) { refs := indexersmock.NewRefOrIDFetcher[*pbdemo.Album, *pbresource.Reference](t) - idx := RefOrIDIndex("test", refs.Execute) + idx := RefOrIDIndex("test", refs.Execute).IndexedData() refs.EXPECT().Execute(dec). Return([]*pbresource.Reference{ref1, ref2}). @@ -90,7 +90,7 @@ func TestBoundRefsIndex(t *testing.T) { }). Build() - idx := BoundRefsIndex[*pbmesh.ComputedRoutes]("test") + idx := BoundRefsIndex[*pbmesh.ComputedRoutes]("test").IndexedData() txn := idx.Txn() require.NoError(t, txn.Insert(r1)) diff --git a/internal/controller/cache/kind.go b/internal/controller/cache/kind.go index 4d9aa4d4f4..1c4c4e3f1f 100644 --- a/internal/controller/cache/kind.go +++ b/internal/controller/cache/kind.go @@ -18,16 +18,16 @@ type kindIndices struct { it unversionedType - indices map[string]*index.Index + indices map[string]*index.IndexedData } func newKindIndices() *kindIndices { kind := &kindIndices{ - indices: make(map[string]*index.Index), + indices: make(map[string]*index.IndexedData), } // add the id index - kind.indices[IDIndex] = indexers.IDIndex(IDIndex, index.IndexRequired) + kind.indices[IDIndex] = indexers.IDIndex(IDIndex, index.IndexRequired).IndexedData() return kind } @@ -38,7 +38,7 @@ func (k *kindIndices) addIndex(i *index.Index) error { return DuplicateIndexError{name: i.Name()} } - k.indices[i.Name()] = i + k.indices[i.Name()] = i.IndexedData() return nil } @@ -178,7 +178,7 @@ func (k *kindIndices) delete(r *pbresource.Resource) error { return nil } -func (k *kindIndices) getIndex(name string) (*index.Index, error) { +func (k *kindIndices) getIndex(name string) (*index.IndexedData, error) { idx, ok := k.indices[name] if !ok { return nil, CacheTypeError{err: IndexNotFoundError{name: name}, it: k.it}