Skip to content

ROX-13261: DryRunUpdate on collection datastore #3687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
keyallis merged 3 commits into from
Nov 8, 2022
Merged

ROX-13261: DryRunUpdate on collection datastore #3687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f0c01d
ROX-13261: DryRunUpdate on collection datastore
keyallis Nov 3, 2022
2791d83
style check fixes
keyallis Nov 3, 2022
a364cbb
static check fix
keyallis Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions central/resourcecollection/datastore/datastore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ type DataStore interface {
DeleteCollection(ctx context.Context, id string) error
DryRunAddCollection(ctx context.Context, collection *storage.ResourceCollection) error
UpdateCollection(ctx context.Context, collection *storage.ResourceCollection) error
DryRunUpdateCollection(ctx context.Context, collection *storage.ResourceCollection) error
// autocomplete workflow, maybe SearchResults? TODO ROX-12616
}

Expand Down
14 changes: 12 additions & 2 deletions central/resourcecollection/datastore/datastore_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,8 +348,14 @@ func (ds *datastoreImpl) updateCollectionWorkflow(ctx context.Context, collectio
return err
}

ds.lock.Lock()
defer ds.lock.Unlock()
// if this a dryrun we don't ever end up calling upsert so we only need to get a read lock
if dryrun {
ds.lock.RLock()
defer ds.lock.RUnlock()
} else {
ds.lock.Lock()
defer ds.lock.Unlock()
}
Comment on lines +351 to +358
Copy link
Copy Markdown
Contributor

@md2119 md2119 Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although the dry run is not making any changes to persistence, the state that it reads at the start of the operation is important otherwise dry run could be acting on an intermediate state. For example, a conflicting name could be added to the set after the dry run obtains the read lock above. Therefore, locking write ops is essential.

Copy link
Copy Markdown
Contributor Author

@keyallis keyallis Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I understand what you mean by this. From what I can logically reason it seems like that would not be an issue and we only need to acquire a read lock for the update dryrun operation

Copy link
Copy Markdown
Contributor

@md2119 md2119 Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I expanded the diff to check how the names is used.


// resolve object to check if the name was changed
storedCollection, ok, err := ds.storage.Get(ctx, collection.GetId())
Expand Down Expand Up @@ -389,6 +395,10 @@ func (ds *datastoreImpl) UpdateCollection(ctx context.Context, collection *stora
return ds.updateCollectionWorkflow(ctx, collection, false)
}

func (ds *datastoreImpl) DryRunUpdateCollection(ctx context.Context, collection *storage.ResourceCollection) error {
return ds.updateCollectionWorkflow(ctx, collection, true)
}

func (ds *datastoreImpl) DeleteCollection(ctx context.Context, id string) error {

// check for access so we can fast fail before locking
Expand Down
54 changes: 49 additions & 5 deletions central/resourcecollection/datastore/datastore_impl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,29 +211,54 @@ func (s *CollectionPostgresDataStoreTestSuite) TestCollectionWorkflows() {
err = s.datastore.DeleteCollection(ctx, objA.GetId())
assert.Error(s.T(), err)

// try to update 'a' to point to 'b' which creates a cycle
// dryrun update 'a' to point to 'b' which creates a cycle
objACycle := getTestCollection("a", []string{objB.GetId()})
objACycle.Id = objA.GetId()
err = s.datastore.DryRunUpdateCollection(ctx, objACycle)
assert.Error(s.T(), err)
_, ok = err.(dag.EdgeLoopError)
assert.True(s.T(), ok)

// update 'a' to point to 'b' which creates a cycle
err = s.datastore.UpdateCollection(ctx, objACycle)
assert.Error(s.T(), err)
_, ok = err.(dag.EdgeLoopError)
assert.True(s.T(), ok)

// try to update 'a' to point to itself which creates a self cycle
// dryrun update 'a' to point to itself which creates a self cycle
updateTestCollection(objACycle, []string{objA.GetId()})
err = s.datastore.DryRunUpdateCollection(ctx, objACycle)
assert.Error(s.T(), err)
_, ok = err.(dag.SrcDstEqualError)
assert.True(s.T(), ok)

// update 'a' to point to itself which creates a self cycle
err = s.datastore.UpdateCollection(ctx, objACycle)
assert.Error(s.T(), err)
_, ok = err.(dag.SrcDstEqualError)
assert.True(s.T(), ok)

// try to update 'a' with a duplicate name
// dryrun update 'a' with a duplicate name
objADup.Id = objA.GetId()
objADup.Name = objB.GetName()
err = s.datastore.DryRunUpdateCollection(ctx, objADup)
assert.Error(s.T(), err)

// update 'a' with a duplicate name
err = s.datastore.UpdateCollection(ctx, objADup)
assert.Error(s.T(), err)

// try to update 'a' with a new name
// dryrun update 'a' with a new name
objADup = objA.Clone()
objA.Name = "A"
err = s.datastore.DryRunUpdateCollection(ctx, objA)
assert.NoError(s.T(), err)
obj, ok, err = s.datastore.Get(ctx, objA.GetId())
assert.NoError(s.T(), err)
assert.True(s.T(), ok)
assert.Equal(s.T(), objADup, obj)

// update 'a' with a new name
err = s.datastore.UpdateCollection(ctx, objA)
assert.NoError(s.T(), err)
obj, ok, err = s.datastore.Get(ctx, objA.GetId())
Expand All @@ -251,6 +276,16 @@ func (s *CollectionPostgresDataStoreTestSuite) TestCollectionWorkflows() {
assert.True(s.T(), ok)
assert.Equal(s.T(), objE, obj)

// dryrun update 'e' to point to only 'a'
objEDup := objE.Clone()
updateTestCollection(objE, []string{objA.GetId()})
err = s.datastore.DryRunUpdateCollection(ctx, objE)
assert.NoError(s.T(), err)
obj, ok, err = s.datastore.Get(ctx, objE.GetId())
assert.NoError(s.T(), err)
assert.True(s.T(), ok)
assert.Equal(s.T(), objEDup, obj)

// update 'e' to point to only 'a', this tests addition and removal of edges
updateTestCollection(objE, []string{objA.GetId()})
err = s.datastore.UpdateCollection(ctx, objE)
Expand All @@ -260,8 +295,17 @@ func (s *CollectionPostgresDataStoreTestSuite) TestCollectionWorkflows() {
assert.True(s.T(), ok)
assert.Equal(s.T(), objE, obj)

// update 'b' to point to only 'e', making sure the original 'e' -> 'b' edge was removed
// dryrun update 'b' to point to only 'e'
objBDup := objB.Clone()
updateTestCollection(objB, []string{objE.GetId()})
err = s.datastore.DryRunUpdateCollection(ctx, objB)
assert.NoError(s.T(), err)
obj, ok, err = s.datastore.Get(ctx, objB.GetId())
assert.NoError(s.T(), err)
assert.True(s.T(), ok)
assert.Equal(s.T(), objBDup, obj)

// update 'b' to point to only 'e', making sure the original 'e' -> 'b' edge was removed
err = s.datastore.UpdateCollection(ctx, objB)
assert.NoError(s.T(), err)
obj, ok, err = s.datastore.Get(ctx, objB.GetId())
Expand Down
14 changes: 14 additions & 0 deletions central/resourcecollection/datastore/mocks/datastore.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.