[OADP-459] Caches getters, move MigrationRegistry annotation from common to imagestream plugin #133

kaovilai · 2022-04-26T01:04:12Z

See https://github.com/openshift/oadp-operator/blob/master/docs/config/custom_plugin_images.md on how to use custom images for testing

snippet of DPA with image built from this PR

apiVersion: oadp.openshift.io/v1alpha1
kind: DataProtectionApplication
metadata:
  name: dpa-sample
spec:
  unsupportedOverrides:
    openshiftPluginImageFqin: ghcr.io/kaovilai/openshift-velero-plugin:registrytmp

OADP-459

The following shared functions now remembers the value after making first api request.
getOADPRegistryRoute, GetRegistryInfo, GetServerVersion, GetBackup, getBackupStorageLocationForBackup, internalRegistrySystemContext
moved registry bits to imagestream

kaovilai · 2022-04-26T02:43:23Z

Initial testing indicates this PR works

$ cat /tmp/openshift.io/velero-plugin/<uid>/openshift-adp/velero-sample-1/oadp-registry-config/registry.txt 
<registryhostname>sh-4.4$

sseago · 2022-04-26T13:42:56Z

Looking at the different things being done in the common backup/restore plugins, the main thing that all pods, deployments, etc. need that we could cache is probably the result of the GetServerVersion and GetRegistryInfo calls. It looks like the OADP registry route (anything done with the MigrationRegistry annotation) is only used by the ImageStream plugin, so I'm thinking that we should probably move the part of common backup/restore plugins which set this to the imagestream backup/restore plugins, since there's no need to do that part at all for 10K deployments, etc.

…ocationForBackup

Cache GetRegistryInfo, GetServerVersion, GetBackup, getBackupStorageLocationForBackup, internalRegistrySystemContext

… condition for restore migLabel

velero-plugins/common/backup.go

kaovilai · 2022-04-28T05:41:38Z

velero-plugins/common/restore.go

 	if err != nil {
 		return nil, err
 	}
 	annotations[RestoreRegistryHostname] = registryHostname

-	if input.Restore.Labels[MigrationApplicationLabelKey] != MigrationApplicationLabelValue {


moved to imagestream, left migLabel alone

kaovilai · 2022-04-28T05:45:52Z

velero-plugins/imagestream/shared.go

@@ -36,3 +70,96 @@ func migrationRegistrySystemContext() (*types.SystemContext, error) {
 	return ctx, nil
 }

+// Takes Namesapce where the operator resides, name of the BackupStorageLocation and name of configMap as input and returns the Route of backup registry.


moved here so we know only imagestream depends on this.

kaovilai

notes

kaovilai · 2022-04-28T06:17:15Z

using plugin built from this PR latest commit backup of parks-app with restify DC scaled to 700 pods
❯ velero backup create test-parksapp-pods --include-namespaces=parks-app --include-resources=pod -n openshift-adp;

Started:    2022-04-28 02:09:29 -0400 EDT
Completed:  2022-04-28 02:10:00 -0400 EDT

Expiration:  2022-05-28 02:09:29 -0400 EDT

Total items to be backed up:  704
Items backed up:              704

about 30 seconds or so.

using instead quay.io/konveyor/openshift-velero-plugin:latest yields

Started:    2022-04-28 02:13:45 -0400 EDT
Completed:  2022-04-28 02:16:30 -0400 EDT

Expiration:  2022-05-28 02:13:45 -0400 EDT

Total items to be backed up:  704
Items backed up:              704

almost 3 minutes.

kaovilai · 2022-04-28T06:25:39Z

see log difference
https://gist.github.com/kaovilai/ecb7940bfe1ca10bd55b24620580d8b5

sseago

Looks good overall. Might be worth making that one trivial change I suggested, removing the switch statement. Won't change much in terms of performance, but it will make the code more readable.

velero-plugins/common/restore.go

kaovilai · 2022-04-28T14:30:21Z

This PR is being updated to use structs in memory instead of writing to disk.

kaovilai · 2022-04-28T16:10:58Z

As of 2b131ef
31 seconds. exactly the same as when writing to disk

Started:    2022-04-28 12:07:44 -0400 EDT
Completed:  2022-04-28 12:08:15 -0400 EDT

Expiration:  2022-05-28 12:07:37 -0400 EDT

Total items to be backed up:  704
Items backed up:              704

sseago

This approach makes sense overall, but lets cache these values in the already-existing plugin structs rather than creating global vars.

sseago · 2022-04-28T17:50:21Z

velero-plugins/common/shared.go

 	"k8s.io/client-go/rest"
 )
+var (
+	registryInfo *string
+	serverVersion *serverVersionStruct


Instead of saving these in global vars, why not add them to the BackupPlugin and RestorePlugin structs? Seems like that would be a much cleaner design.

The reason is, these can be reused for the next backup if my understanding is right. Having them in BackupPlugin and RestorePlugin may result in higher memory usage.

sseago · 2022-04-28T17:51:35Z

velero-plugins/imagestream/shared.go

 	"k8s.io/client-go/rest"
 )

+var (
+	internalRegistrySystemContextVar *types.SystemContext
+	oadpRegistryRoute *string


As with common, I'd rather see this added to the BackupPlugin and RestorePlugin structs than use a global var.

Got a "OK" :D

kaovilai · 2022-05-02T21:07:28Z

Results from Travis in slack

pre-fix: ~35 minutes was for 10k deployments. ~4 minutes for 1k deployments
post-fix: haven't tested 10k yet, but 1k took 30 seconds, which is about the same as velero on non-image resources (e.g. configmaps, secrets, etc.) that I've tested

…mon to imagestream plugin (#133) * registry tmp proto * use get for restore * add error text to log * reset err when reading route * Make dir before writing file * Cache GetRegistryInfo, GetServerVersion, GetBackup, getBackupStorageLocationForBackup * export WriteByteToDirPath * Cache internalRegistrySystemContext * move MigrationRegistry, SkipImageCopy to imagestream plugin, refactor condition for restore migLabel * remove redundant svcAccount switch/case * reduce params for GetRegistryInfo, use var instead of writing to disk (cherry picked from commit c231cbe)

kaovilai added 5 commits April 25, 2022 21:03

registry tmp proto

b909060

use get for restore

b246631

add error text to log

476fee6

reset err when reading route

83f643c

Make dir before writing file

f366538

kaovilai requested review from sseago and dymurray April 26, 2022 02:48

kaovilai and others added 4 commits April 27, 2022 23:09

Cache GetRegistryInfo, GetServerVersion, GetBackup, getBackupStorageL…

9fecc8b

…ocationForBackup

export WriteByteToDirPath

7c7e273

Cache internalRegistrySystemContext

6ca2e83

Merge pull request #7 from kaovilai/registrytmp+otherthings

8d3af3a

Cache GetRegistryInfo, GetServerVersion, GetBackup, getBackupStorageLocationForBackup, internalRegistrySystemContext

kaovilai changed the title ~~Saves found route to registry in tmp~~ Getters caches per backup static values in /tmp Apr 28, 2022

move MigrationRegistry, SkipImageCopy to imagestream plugin, refactor…

3750cad

… condition for restore migLabel

kaovilai changed the title ~~Getters caches per backup static values in /tmp~~ Caches getters, move MigrationRegistry annotation from common to imagestream plugin Apr 28, 2022

kaovilai commented Apr 28, 2022

View reviewed changes

velero-plugins/common/backup.go Show resolved Hide resolved

kaovilai commented Apr 28, 2022

View reviewed changes

sseago approved these changes Apr 28, 2022

View reviewed changes

velero-plugins/common/restore.go Outdated Show resolved Hide resolved

remove redundant svcAccount switch/case

94b47aa

kaovilai marked this pull request as draft April 28, 2022 14:30

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2022

reduce params for GetRegistryInfo, use var instead of writing to disk

2b131ef

kaovilai marked this pull request as ready for review April 28, 2022 16:01

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 28, 2022

kaovilai requested a review from sseago April 28, 2022 16:01

sseago previously requested changes Apr 28, 2022

View reviewed changes

kaovilai requested a review from sseago May 2, 2022 21:04

sseago approved these changes May 2, 2022

View reviewed changes

tjanssen3 mentioned this pull request May 7, 2022

Measure and graph velero backup and restore system performance impact varying API server object count, types, and sizes RamenDR/ramen#973

Open

2 tasks

dymurray approved these changes May 9, 2022

View reviewed changes

kaovilai changed the title ~~Caches getters, move MigrationRegistry annotation from common to imagestream plugin~~ [OADP-459] Caches getters, move MigrationRegistry annotation from common to imagestream plugin May 9, 2022

dymurray merged commit c231cbe into openshift:master May 9, 2022

This was referenced May 23, 2022

Openshift plugin being executed on each item in namespace caused timeout #115

Closed

[master] OADP-532 registry route cache multi-bsl collision fix #143

Closed

kaovilai mentioned this pull request Apr 12, 2023

oadp-1.0: OADP-1650 Add annotation to exclude build pod volumes during backup (#189) #190

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OADP-459] Caches getters, move MigrationRegistry annotation from common to imagestream plugin #133

[OADP-459] Caches getters, move MigrationRegistry annotation from common to imagestream plugin #133

kaovilai commented Apr 26, 2022 •

edited

kaovilai commented Apr 26, 2022

sseago commented Apr 26, 2022

kaovilai Apr 28, 2022

kaovilai Apr 28, 2022

kaovilai left a comment

kaovilai commented Apr 28, 2022

kaovilai commented Apr 28, 2022

sseago left a comment

kaovilai commented Apr 28, 2022

kaovilai commented Apr 28, 2022

sseago left a comment

sseago Apr 28, 2022

kaovilai Apr 28, 2022

sseago Apr 28, 2022

kaovilai commented May 2, 2022

[OADP-459] Caches getters, move MigrationRegistry annotation from common to imagestream plugin #133

[OADP-459] Caches getters, move MigrationRegistry annotation from common to imagestream plugin #133

Conversation

kaovilai commented Apr 26, 2022 • edited

kaovilai commented Apr 26, 2022

sseago commented Apr 26, 2022

kaovilai Apr 28, 2022

Choose a reason for hiding this comment

kaovilai Apr 28, 2022

Choose a reason for hiding this comment

kaovilai left a comment

Choose a reason for hiding this comment

kaovilai commented Apr 28, 2022

kaovilai commented Apr 28, 2022

sseago left a comment

Choose a reason for hiding this comment

kaovilai commented Apr 28, 2022

kaovilai commented Apr 28, 2022

sseago left a comment

Choose a reason for hiding this comment

sseago Apr 28, 2022

Choose a reason for hiding this comment

kaovilai Apr 28, 2022

Choose a reason for hiding this comment

sseago Apr 28, 2022

Choose a reason for hiding this comment

kaovilai commented May 2, 2022

kaovilai commented Apr 26, 2022 •

edited