feat: metadata backup returns bucket manifests #21601
Conversation
backup/backup.go
Outdated
| return err | ||
| } | ||
|
|
||
| l := []influxdb.BucketMetadataManifest{} |
There was a problem hiding this comment.
Could do this as a small optimization:
l := make([]influxdb.BucketMetadataManifest, 0, len(bkts))
There was a problem hiding this comment.
Nice! I did that for this and the other similar functions.
backup/backup.go
Outdated
| // retentionPolicyToManifest and the various similar functions that follow are for converting | ||
| // from the structs in the meta package to the manifest structs | ||
| func retentionPolicyToManifest(meta []meta.RetentionPolicyInfo) []influxdb.RetentionPolicyManifest { | ||
| r := []influxdb.RetentionPolicyManifest{} |
There was a problem hiding this comment.
Could pre-set the capacity here via make but it's probably less important since buckets in 2.x only have 1 retention policy
| // It is intended to be used to write to an HTTP response after appropriate measures have been taken | ||
| // to ensure that the request is authorized. | ||
| func (b BucketManifestWriter) WriteManifest(ctx context.Context, w io.Writer) error { | ||
| bkts, _, err := b.ts.FindBuckets(ctx, influxdb.BucketFilter{}) |
There was a problem hiding this comment.
Making sure: in a "real" DB this call (and the calls to OrganizationService) don't try to take a lock on the KV store, right? i.e. we don't deadlock here
There was a problem hiding this comment.
They both need read locks, for example the FindBuckets goes through here & FindOrganizationByID go through here, both of which end up getting the lock here.
The handleBackupMetadata gets a read lock on the KV at the start of the process and holds it until the end. So once the handler has the lock, the subsequent calls in the process can also get a read lock.
That all sounds great of course, but one other thing I forgot to mention in the initial comment is that I am actually able to successfully hit the api/v2/backup/metadata endpoint and everything works fine, so that's some verification that it doesn't deadlock 😄
Closes #21567
This PR finishes the implementation of the
backup/metadataendpoint with the full array of bucket data manifests as described in https://github.com/influxdata/openapi/blob/master/src/oss/schemas/BucketMetadataManifests.yml etc.I also modified the logic for
handleBackupMetadatato useCreatePartinstead ofCreateFormFile. This seems more appropriate for themultipart/mixedresponse type, although it does require manually setting more of the headers.I haven't come up with an easy way to unit test the
WriteManifestfunction. Ideally we would have an end-to-end test of some kind that would cover it as well as the entirebackup/metadataendpoint to make sure the response can actually be parsed, processed, and successfully restored eventually.