chore: backup support for new multitenant system #2549

BonapartePC · 2022-10-11T10:46:49Z

Description

Supporting backup upload for the multitenant system. The following are the changes:

We support using self-storage by the customer now
In a table with multiple customers, we might have to send different customers to different storage. With this PR, it should be possible.
Every gz file will now have a workspace id at the end attached to the file name.
Depending on the user preferences, proc_errors and gw dumps will be uploaded if they are set to true.

Notion Ticket

https://www.notion.so/rudderstacks/Allow-Customers-to-use-self-storage-26f45f75e38b4ef6bd872bea6187aa06
https://www.notion.so/rudderstacks/Divide-customers-events-in-jobs-table-and-upload-them-in-respective-storages-04dbf3dbd7bd435786e6a3808bed73ed

Security

The code changed/added as part of this pull request won't create any security issues with how the software is being used.

codecov · 2022-10-11T11:00:51Z

Codecov Report

Base: 43.74% // Head: 43.65% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (774836b) compared to base (00ba231).
Patch coverage: 77.36% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2549      +/-   ##
==========================================
- Coverage   43.74%   43.65%   -0.09%     
==========================================
  Files         187      191       +4     
  Lines       39993    40486     +493     
==========================================
+ Hits        17494    17675     +181     
- Misses      21403    21704     +301     
- Partials     1096     1107      +11

Impacted Files	Coverage Δ
config/backend-config/types.go	`0.00% <0.00%> (ø)`
jobsdb/jobsdb.go	`68.79% <20.00%> (-0.02%)`	⬇️
services/fileuploader/gzwriter.go	`64.00% <64.00%> (ø)`
processor/stash/stash.go	`21.39% <73.68%> (ø)`
jobsdb/backup.go	`76.30% <74.46%> (+1.39%)`	⬆️
services/fileuploader/fileuploader.go	`96.96% <96.96%> (ø)`
processor/manager.go	`86.04% <100.00%> (+0.33%)`	⬆️
processor/processor.go	`71.21% <100.00%> (-0.04%)`	⬇️
middleware/middleware.go	`86.20% <0.00%> (-8.63%)`	⬇️
services/rsources/handler.go	`69.72% <0.00%> (-1.39%)`	⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

config/backend-config/types.go

atzoum · 2022-10-24T13:54:33Z

@BonapartePC #2542 introduced a major refactoring on most backup functions. Furthermore, all backup-related functions are now moved in a separate file, backup.go. @saurav-malani may provide you with some more information regarding the scope of this aforementioned refactoring.
Please resolve all relevant conflicts, in order to proceed with the review.

services/backup/backup.go

atzoum

My only concern is test coverage.
Such large refactorings should increase overal code coverage instead of decreasing it.

…rver into chore.dataRetention

BonapartePC · 2022-11-04T05:59:04Z

My only concern is test coverage. Such large refactorings should increase overal code coverage instead of decreasing it.

The coverage is a little better now. Most of the impacted files are the ones where the fileuploader is just passed but there are no tests that result in decreasing coverage a bit.

raman2805 · 2022-11-04T03:05:46Z

jobsdb/backup.go

@@ -68,7 +70,6 @@ func (jd *HandleT) backupDSLoop(ctx context.Context) {
 		opID := jd.JournalMarkStart(backupDSOperation, opPayload)
 		err = jd.backupDS(ctx, backupDSRange)
 		if err != nil {
-			stats.Default.NewTaggedStat("backup_ds_failed", stats.CountType, stats.Tags{"customVal": jd.tablePrefix, "provider": config.GetString("JOBS_BACKUP_STORAGE_PROVIDER", "S3")}).Increment()


Does this not change the semantics of the backup_ds_failed metric which would count all failures (including issues while calling cleanStatusTable, which gets skipped if you remove this). I see this has been moved to the uploadDumps function with a different set of labels. Should we not keep this as is and rename the stat in uploadDumps to somethinglike upload_dump_failed.

I had to remove this here because at the point where we are making this stat we can't determine the provider tag. This will be different for each workspace which was previously taken from the env.

raman2805 · 2022-11-04T03:21:22Z

jobsdb/backup.go

+	g, _ := errgroup.WithContext(ctx)
+	g.SetLimit(config.GetInt("JobsDB.JobsBackupUploadWorkers", 100))


Question: Should this not be

g, ctx := errgroup.WithContext(ctx)

and we should be passing the derived ctx is passed to all the goroutines

Makes sense. added

raman2805 · 2022-11-04T03:58:37Z

jobsdb/backup.go

+	dumps, err := jd.createTableDumps(getFailedOnlyBackupQueryFn(backupDSRange), getFileName, totalCount)
 	if err != nil {
 		return fmt.Errorf("error while creating table dump: %w", err)
 	}
-	defer func() { _ = os.Remove(path) }()
-
-	err = jd.uploadTableDump(ctx, path)
+	defer func() {
+		for _, filePath := range dumps {
+			os.Remove(filePath)
+		}
+	}()
+	err = jd.uploadDumps(ctx, dumps)
 	if err != nil {
-		jd.logger.Errorf("[JobsDB] :: Failed to upload table %v", tableName)
-		return err
+		return fmt.Errorf("error while uploading dumps for table: %s: %w", tableName, err)
 	}
-
 	stats.Default.NewTaggedStat("total_TableDump_TimeStat", stats.TimerType, stats.Tags{"customVal": jd.tablePrefix}).Since(start)
 	return nil
 }


minor: This code is almost identical to the code in lines 241-256, 294-311 factorize it maybe?

We decided to leave it this way so it is clearly shown as different steps being taken

raman2805 · 2022-11-04T04:00:16Z

jobsdb/backup.go

-	err = jd.uploadTableDump(ctx, path)
+	defer func() {
+		for _, filePath := range dumps {
+			os.Remove(filePath)


_ = os.Remove(filePath)

Made the change

atzoum · 2022-11-04T06:28:58Z

services/fileuploader/fileuploader.go

+func (p *provider) GetFileManager(workspaceID string) (filemanager.FileManager, error) {
+	<-p.init
+	settings, ok := p.storageSettings[workspaceID]
+	if !ok {
+		return nil, fmt.Errorf("no storage settings found for workspace: %s", workspaceID)
+	}
+	return filemanager.DefaultFileManagerFactory.New(&filemanager.SettingsT{
+		Provider: settings.Bucket.Type,
+		Config:   settings.Bucket.Config,
+	})


please add a test scenario for this function

atzoum · 2022-11-04T06:29:14Z

services/fileuploader/fileuploader.go

+func NewStaticProvider(storageSettings map[string]StorageSettings) Provider {
+	s := &provider{
+		init:            make(chan struct{}),
+		storageSettings: storageSettings,
+	}
+	close(s.init)
+	return s


please add a test scenario for this function

atzoum · 2022-11-04T06:30:01Z

services/fileuploader/fileuploader.go

+	}
+
+	p.onceInit.Do(func() {
+		close(p.init)


why this appears to be not covered with tests?

Added a test for that

atzoum · 2022-11-04T06:30:12Z

services/fileuploader/fileuploader.go

+func (*defaultProvider) GetFileManager(_ string) (filemanager.FileManager, error) {
+	defaultConfig := getDefaultBucket(context.Background(), config.GetString("JOBS_BACKUP_STORAGE_PROVIDER", "S3"))
+	return filemanager.DefaultFileManagerFactory.New(&filemanager.SettingsT{
+		Provider: defaultConfig.Type,
+		Config:   defaultConfig.Config,
+	})


please add a test scenario for this function

saurav-malani · 2022-11-04T08:33:21Z

services/fileuploader/gzwriter.go

@@ -0,0 +1,62 @@
+package fileuploader


why is the rational behind creating this services/fileuploader/gzwriter? I mean it seems to be doing things very specific to our jobsDB backup (atleast as of now). Creating a gzFileHandler which has a map of path & gzipwriter. Where else are we going to use it? If no where, then won’t it be better to move it to jobsdb backup?

The idea is it can be used anywhere not just jobsdb so I kept it in services folder

BonapartePC and others added 2 commits October 8, 2022 08:50

chore: backup support for new multitenant system

7c98f6b

Merge branch 'master' into chore.dataRetention

fa21b13

github-actions bot added the server-team label Oct 11, 2022

BonapartePC and others added 3 commits October 13, 2022 22:00

Merge branch 'master' into chore.dataRetention

bbfb96d

add workers for creating and uploading backup files

a3cc925

Merge branch 'master' into chore.dataRetention

830da1b

BonapartePC force-pushed the chore.dataRetention branch from d108317 to c77cdd3 Compare October 18, 2022 10:50

check preferences before upload

2f896da

BonapartePC force-pushed the chore.dataRetention branch from c77cdd3 to 2f896da Compare October 18, 2022 11:43

BonapartePC added 2 commits October 19, 2022 13:21

Merge branch 'master' into chore.dataRetention

19962be

Merge branch 'master' into chore.dataRetention

105ddd6

github-actions bot added the with tests label Oct 20, 2022

overwrite storage setting config

ce530cf

BonapartePC force-pushed the chore.dataRetention branch from e2bf0a7 to ce530cf Compare October 21, 2022 03:50

BonapartePC and others added 2 commits October 21, 2022 09:55

Merge branch 'master' into chore.dataRetention

448b8b9

add max retries

b8a95f5

BonapartePC requested review from fracasula, chandumlg and atzoum October 21, 2022 09:19

$fracasula$

fracasula reviewed Oct 24, 2022

View reviewed changes

config/backend-config/types.go Outdated Show resolved Hide resolved

config/backend-config/types.go Outdated Show resolved Hide resolved

atzoum requested a review from saurav-malani October 24, 2022 07:58

atzoum reviewed Oct 24, 2022

View reviewed changes

services/backup/backup.go Outdated Show resolved Hide resolved

BonapartePC added 2 commits October 25, 2022 23:48

limit workers

9748693

fix comments and deepsource issues

15e452e

cisse21 assigned cisse21 and unassigned cisse21 Oct 26, 2022

cisse21 self-requested a review October 26, 2022 08:04

add backup_test.go in services

ae187df

fix tests

9cde50c

atzoum requested a review from saurav-malani November 3, 2022 17:53

atzoum reviewed Nov 3, 2022

View reviewed changes

add tests for gzwriter

ea720ae

BonapartePC force-pushed the chore.dataRetention branch from fcd29ea to ea720ae Compare November 3, 2022 19:33

BonapartePC and others added 3 commits November 4, 2022 01:12

Merge branch 'master' into chore.dataRetention

5fe9bc9

slight changes in gzwriter tests

ca36259

Merge branch 'chore.dataRetention' of github.com:rudderlabs/rudder-se…

04c269f

…rver into chore.dataRetention

raman2805 reviewed Nov 4, 2022

View reviewed changes

atzoum reviewed Nov 4, 2022

View reviewed changes

address review comments

774836b

BonapartePC force-pushed the chore.dataRetention branch from d9495e7 to 774836b Compare November 4, 2022 08:08

saurav-malani reviewed Nov 4, 2022

View reviewed changes

saurav-malani approved these changes Nov 4, 2022

View reviewed changes

atzoum requested a review from fracasula November 4, 2022 10:13

atzoum approved these changes Nov 4, 2022

View reviewed changes

$fracasula$

fracasula approved these changes Nov 4, 2022

View reviewed changes

chandumlg merged commit 4741989 into master Nov 4, 2022

chandumlg deleted the chore.dataRetention branch November 4, 2022 16:20

This was referenced Dec 1, 2022

chore: release 1.3.0 #2749

Closed

chore: prerelease 1.4.0-rc.1 #2750

Merged

chore: release 1.4.0 #2752

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: backup support for new multitenant system #2549

chore: backup support for new multitenant system #2549

BonapartePC commented Oct 11, 2022 •

edited

Loading

codecov bot commented Oct 11, 2022 •

edited

Loading

atzoum commented Oct 24, 2022

atzoum left a comment

BonapartePC commented Nov 4, 2022

raman2805 Nov 4, 2022

BonapartePC Nov 4, 2022

raman2805 Nov 4, 2022

BonapartePC Nov 4, 2022

raman2805 Nov 4, 2022

BonapartePC Nov 4, 2022

raman2805 Nov 4, 2022

BonapartePC Nov 4, 2022

atzoum Nov 4, 2022

BonapartePC Nov 4, 2022

atzoum Nov 4, 2022

BonapartePC Nov 4, 2022

atzoum Nov 4, 2022

BonapartePC Nov 4, 2022

atzoum Nov 4, 2022

BonapartePC Nov 4, 2022

saurav-malani Nov 4, 2022

BonapartePC Nov 4, 2022

		g, _ := errgroup.WithContext(ctx)
		g.SetLimit(config.GetInt("JobsDB.JobsBackupUploadWorkers", 100))

chore: backup support for new multitenant system #2549

chore: backup support for new multitenant system #2549

Conversation

BonapartePC commented Oct 11, 2022 • edited Loading

Description

Notion Ticket

Security

codecov bot commented Oct 11, 2022 • edited Loading

Codecov Report

atzoum commented Oct 24, 2022

atzoum left a comment

Choose a reason for hiding this comment

BonapartePC commented Nov 4, 2022

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

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

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

BonapartePC commented Oct 11, 2022 •

edited

Loading

codecov bot commented Oct 11, 2022 •

edited

Loading