Make conditional vs unconditional updates clearer

gravitational · Nov 4, 2024 · 2e081a1 · 2e081a1
1 parent 15bc915
commit 2e081a1
Show file tree

Hide file tree

Showing 6 changed files with 12 additions and 10 deletions.
diff --git a/lib/services/local/access_monitoring_rules.go b/lib/services/local/access_monitoring_rules.go
@@ -85,7 +85,7 @@ func (s *AccessMonitoringRulesService) CreateAccessMonitoringRule(ctx context.Co
 
 // UpdateAccessMonitoringRule updates an existing AccessMonitoringRule resource.
 func (s *AccessMonitoringRulesService) UpdateAccessMonitoringRule(ctx context.Context, amr *accessmonitoringrulesv1.AccessMonitoringRule) (*accessmonitoringrulesv1.AccessMonitoringRule, error) {
-	updated, err := s.svc.UpdateResource(ctx, amr)
+	updated, err := s.svc.UnconditionalUpdateResource(ctx, amr)
 	return updated, trace.Wrap(err)
 }
 

diff --git a/lib/services/local/autoupdate.go b/lib/services/local/autoupdate.go
@@ -113,7 +113,7 @@ func (s *AutoUpdateService) UpdateAutoUpdateConfig(
 	ctx context.Context,
 	c *autoupdate.AutoUpdateConfig,
 ) (*autoupdate.AutoUpdateConfig, error) {
-	config, err := s.config.UpdateResource(ctx, c)
+	config, err := s.config.UnconditionalUpdateResource(ctx, c)
 	return config, trace.Wrap(err)
 }
 
@@ -151,7 +151,7 @@ func (s *AutoUpdateService) UpdateAutoUpdateVersion(
 	ctx context.Context,
 	v *autoupdate.AutoUpdateVersion,
 ) (*autoupdate.AutoUpdateVersion, error) {
-	version, err := s.version.UpdateResource(ctx, v)
+	version, err := s.version.UnconditionalUpdateResource(ctx, v)
 	return version, trace.Wrap(err)
 }
 
@@ -189,7 +189,7 @@ func (s *AutoUpdateService) UpdateAutoUpdateAgentRollout(
 	ctx context.Context,
 	v *autoupdate.AutoUpdateAgentRollout,
 ) (*autoupdate.AutoUpdateAgentRollout, error) {
-	rollout, err := s.rollout.UpdateResource(ctx, v)
+	rollout, err := s.rollout.UnconditionalUpdateResource(ctx, v)
 	return rollout, trace.Wrap(err)
 }
 

diff --git a/lib/services/local/databaseobject.go b/lib/services/local/databaseobject.go
@@ -45,7 +45,7 @@ func (s *DatabaseObjectService) UpsertDatabaseObject(ctx context.Context, object
 }
 
 func (s *DatabaseObjectService) UpdateDatabaseObject(ctx context.Context, object *dbobjectv1.DatabaseObject) (*dbobjectv1.DatabaseObject, error) {
-	out, err := s.service.UpdateResource(ctx, object)
+	out, err := s.service.UnconditionalUpdateResource(ctx, object)
 	return out, trace.Wrap(err)
 }
 

diff --git a/lib/services/local/databaseobjectimportrule.go b/lib/services/local/databaseobjectimportrule.go
@@ -43,7 +43,7 @@ func (s *databaseObjectImportRuleService) UpsertDatabaseObjectImportRule(ctx con
 }
 
 func (s *databaseObjectImportRuleService) UpdateDatabaseObjectImportRule(ctx context.Context, rule *databaseobjectimportrulev1.DatabaseObjectImportRule) (*databaseobjectimportrulev1.DatabaseObjectImportRule, error) {
-	out, err := s.service.UpdateResource(ctx, rule)
+	out, err := s.service.UnconditionalUpdateResource(ctx, rule)
 	return out, trace.Wrap(err)
 }
 

diff --git a/lib/services/local/generic/generic_wrapper.go b/lib/services/local/generic/generic_wrapper.go
@@ -125,14 +125,16 @@ func (s ServiceWrapper[T]) UpsertResource(ctx context.Context, resource T) (T, e
 	return adapter.resource, trace.Wrap(err)
 }
 
-// UpdateResource updates an existing resource.
-func (s ServiceWrapper[T]) UpdateResource(ctx context.Context, resource T) (T, error) {
+// UnconditionalUpdateResource updates an existing resource without checking the provided resource revision.
+// This is faster and cheaper than ConditionalUpdateResource but can cause data loss on concurrent writes.
+func (s ServiceWrapper[T]) UnconditionalUpdateResource(ctx context.Context, resource T) (T, error) {
 	adapter, err := s.service.UpdateResource(ctx, newResourceMetadataAdapter(resource))
 	return adapter.resource, trace.Wrap(err)
 }
 
 // ConditionalUpdateResource updates an existing resource if the provided
 // resource and the existing resource have matching revisions.
+// This is safer than UnconditionalUpdateResource but more expensive and might not suit high-load scenarios.
 func (s ServiceWrapper[T]) ConditionalUpdateResource(ctx context.Context, resource T) (T, error) {
 	adapter, err := s.service.ConditionalUpdateResource(ctx, newResourceMetadataAdapter(resource))
 	return adapter.resource, trace.Wrap(err)

diff --git a/lib/services/local/generic/generic_wrapper_test.go b/lib/services/local/generic/generic_wrapper_test.go
@@ -180,7 +180,7 @@ func TestGenericWrapperCRUD(t *testing.T) {
 
 	// Update a resource.
 	r1.Metadata.Labels = map[string]string{"newlabel": "newvalue"}
-	r1, err = service.UpdateResource(ctx, r1)
+	r1, err = service.UnconditionalUpdateResource(ctx, r1)
 	require.NoError(t, err)
 	r, err = service.GetResource(ctx, r1.GetMetadata().GetName())
 	require.NoError(t, err)
@@ -198,7 +198,7 @@ func TestGenericWrapperCRUD(t *testing.T) {
 
 	// Update a resource that doesn't exist.
 	doesNotExist := newTestResource153("doesnotexist")
-	_, err = service.UpdateResource(ctx, doesNotExist)
+	_, err = service.UnconditionalUpdateResource(ctx, doesNotExist)
 	require.True(t, trace.IsNotFound(err))
 
 	// Delete a resource.