CBG-3688 make sure kv_pool_size is overriden correctly for DCP #6680

torcolvin · 2024-02-12T15:53:39Z

create two functions for getting the connection string
- GetGoCBConnString
- GetGoCBConnStringForDCP

Simplifications to API:

Fix a confusing invariant where GetGoCBConnString would initialize KvPoolSize on BucketSpec by requiring a constructor that does string parsing of server value to pull out kv_pool_size. This will fail if an invalid kv_pool_size value is created.
Move the creation of modified connection string for the bootstrap connection to the caller (rest/main.go). This means that both serverless and regular callers will have values set on the connection string for bootstrap connection. Before these values were only set for serverless bootstrap:
- max_perhost_idle_http_connections
- max_idle_http_connections
- idle_http_connection_timeout

Questions:

We only set ca_cert_path for Bucket connections and not for the bootstrap connection? The only way that I imagine that this works is if the ca cert is in the system already or embedded in the pem file. I tried to use our x509 tests but they are broken because they don't run automatically. I tried to fix them but it is somewhat non trivial.

Pre-review checklist

Removed debug logging (fmt.Print, log.Print, ...)
Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGateway-Integration/2303/

- create two functions for getting the connection string - GetGoCBConnString - GetGoCBConnStringForDCP Simplifications to API: - Fix a confusing invariant where GetGoCBConnString would initialize KvPoolSize on BucketSpec by requiring a constructor that does string parsing of server value to pull out kv_pool_size. This will fail if an invalid kv_pool_size value is created. - Move the creation of modified connection string for the bootstrap connection to the caller (rest/main.go). This means that both serverless and regular callers will have values set on the connection string for bootstrap connection. Before these values were only set for serverless bootstrap: - max_perhost_idle_http_connections - max_idle_http_connections - idle_http_connection_timeout

adamcfraser

A few comments/questions.

base/gocb_connection_string.go

adamcfraser · 2024-02-12T21:25:47Z

base/bucket.go

@@ -119,6 +118,11 @@ type CouchbaseBucketType int

 // Full specification of how to connect to a bucket
 type BucketSpec struct {
+	BucketSpecOptions


This refactor feels arbitrary - I don't think KvPoolSize is any more or less of an 'option' than things like BucketOpTimeout. What's the problem that this refactor fixes?

KvPoolSize isn't an input parameter, it is only ever read from the connection string. It is parsed from the connection string and then stored when creating a bucket to determine how to throttle kvops in the below code.

numPools := 1 if spec.KvPoolSize > 0 { numPools = spec.KvPoolSize } gocbv2Bucket.kvOps = make(chan struct{}, MaxConcurrentSingleOps*nodeCount*numPools)

A different way of addressing the immutability of the struct would be to never set this value and avoid the use of a constructor for this struct. Then, inside GetGocbV2BucketFromCluster you could call getKvPoolSize(spec.Server) which would reparse the connection string. The reason I didn't do it like this was to only parse the connection string once but this is such a cheap operation (string parsing) compared to bucket opening that it would be fine.

Do you think this would be more readable?

I am not set on 2f038b9 but it simplifies the PR.

This will behave identically to the original PR but in order for serverless values to work, you have to modify the connection string on the BucketSpec to have kv_pool_size=1. This is done in GetBucketSpec anyway to add dcp_buffer_size and kv_buffer_size custom values so I think this is probably a reasonable refactor.

The downsides (which aren't bad):

the setting of an implicit kv_pool_size is set as DefaultGocbKvPoolSize inside some functions - BucketSpec.getGoCBConnString and BucketSpec.GetKvPoolSize and they have the potential to get out of sync.

the place that we will validate that we haven't passed kv_pool_size twice in a connection string will take place when opening a bucket and not loading a database. This is unlikely behavior we didn't test for anyway.

we parse the connection string twice when creating a Bucket, once to get the connection string for a cluster and another for figuring out the kv_pool_size. This is a cheap string operation vs a kv op for a rare condition.

…ion to GetKvPoolSize

adamcfraser

Overall this makes sense. Added a suggestion to go further with removing KvPoolSize from the bucket spec and just extract it from a computed connstring as needed.

base/bucket.go

adamcfraser · 2024-02-14T18:23:08Z

base/collection.go

-	if spec.KvPoolSize > 0 {
-		numPools = spec.KvPoolSize
+
+	numPools, err := spec.GetKvPoolSize()


It feels like GetKvPoolSize should be a helper function that takes a connString, and doesn't use the spec? That way we can be certain that the value here matches what's actually getting sent to gocb (on the conn string), and we don't need to have any logic inside GetKvPoolSize around default values, etc.

That would also let us get rid of the test-only getKVConnectionPol [sic], and just use the new connString based function for test verification.

I changed this to a generic helper function so I could use it for kv_pool_size and kv_buffer_size.

adamcfraser · 2024-02-14T18:45:32Z

base/dcp_sharded.go

@@ -296,7 +291,7 @@ func initCBGTManager(ctx context.Context, bucket Bucket, spec BucketSpec, cfgSG

 	// Since cbgt initializes a buffer per CBS node per partition in most cases (vbuckets in partitions can't be grouped by CBS node),
 	// setting the small buffer size used in cbgt 1.3.2.  (see CBG-3341 for potential optimization of this value)
-	options["kvConnectionBufferSize"] = "16384"
+	options["kvConnectionBufferSize"] = "16384" // This value will be overridden by the BucketSpec connection string.


I think it might be appropriate to log something at warn in the case where the connection string is setting a larger value for this, to help us triage possible memory issues that would result. The message would be along the lines of "dcp_buffer_size is set to %d on the connection string - this will result in increased memory usage"

For clarity, this is actually kv_buffer_size: https://sourcegraph.com/github.com/couchbase/gocbcore@8c8c79204421aa47127a042ac2ac560d87920645/-/blob/agent_config.go?L411

Implemented this idea.

adamcfraser

Generally looks good, wanted one bit of clarification.

adamcfraser · 2024-02-15T18:11:42Z

rest/server_context.go

@@ -455,9 +455,33 @@ func (sc *ServerContext) getOrAddDatabaseFromConfig(ctx context.Context, config
 	return sc._getOrAddDatabaseFromConfig(ctx, config, options)
 }

-func GetBucketSpec(ctx context.Context, config *DatabaseConfig, serverConfig *StartupConfig) (spec base.BucketSpec, err error) {
+// GetBucketSpec returns a BucketSpec from a given DatabaseConfig and StartupConfig.
+func GetBucketSpec(ctx context.Context, config *DatabaseConfig, serverConfig *StartupConfig) (*base.BucketSpec, error) {


What's the benefit in changing to a pointer return for BucketSpec here?

I had changed it to a pointer in a previous iteration because the value that it returns is effectively nil and you wouldn't want to use it. It doesn't matter in this case because we check the error value of the function and performing a construction/copy on BucketSpec is rare and cheap.

I restored the previous behavior.

* CBG-3688 make sure kv_pool_size is overriden correctly for DCP - create two functions for getting the connection string - GetGoCBConnString - GetGoCBConnStringForDCP Simplifications to API: - Fix a confusing invariant where GetGoCBConnString would initialize KvPoolSize on BucketSpec but making BucketSpec stateless and moving operations to be performed on an connection string - Remove redundant ca_cert_path, set by auth - Centralize modifying the connection string defaults for serverless on Bootstrap and Bucket creation.

torcolvin requested a review from adamcfraser February 12, 2024 16:44

torcolvin assigned adamcfraser Feb 12, 2024

adamcfraser requested changes Feb 12, 2024

View reviewed changes

Rename functions, remove redundant ca_cert_path

2095363

torcolvin mentioned this pull request Feb 13, 2024

[3.1.4 fix] CBG-3777 make sure cbgt always sets kv_pool_size=1 #6681

Merged

4 tasks

Remove cached KvPoolSize on BucketSpec and move possible error condit…

2f038b9

…ion to GetKvPoolSize

adamcfraser requested changes Feb 14, 2024

View reviewed changes

Make kv pool size a free function

ad85a58

adamcfraser assigned torcolvin and unassigned adamcfraser Feb 14, 2024

Warn if kv_buffer_size is oversize

d4af9f0

torcolvin assigned adamcfraser and unassigned torcolvin Feb 15, 2024

adamcfraser reviewed Feb 15, 2024

View reviewed changes

Return BucketSpec to a concrete type

4a65d0c

adamcfraser approved these changes Feb 15, 2024

View reviewed changes

torcolvin merged commit 6cd38f1 into master Feb 15, 2024
30 checks passed

torcolvin deleted the CBG-3688 branch February 15, 2024 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBG-3688 make sure kv_pool_size is overriden correctly for DCP #6680

CBG-3688 make sure kv_pool_size is overriden correctly for DCP #6680

torcolvin commented Feb 12, 2024 •

edited

Loading

adamcfraser left a comment

adamcfraser Feb 12, 2024

torcolvin Feb 12, 2024

torcolvin Feb 13, 2024

adamcfraser left a comment

adamcfraser Feb 14, 2024

torcolvin Feb 15, 2024

adamcfraser Feb 14, 2024

torcolvin Feb 15, 2024

torcolvin Feb 15, 2024

adamcfraser left a comment

adamcfraser Feb 15, 2024

torcolvin Feb 15, 2024

CBG-3688 make sure kv_pool_size is overriden correctly for DCP #6680

CBG-3688 make sure kv_pool_size is overriden correctly for DCP #6680

Conversation

torcolvin commented Feb 12, 2024 • edited Loading

Pre-review checklist

adamcfraser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamcfraser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamcfraser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torcolvin commented Feb 12, 2024 •

edited

Loading