Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use restic stats instead of check to check repo existence #1171

Merged
merged 1 commit into from
Feb 6, 2019

Conversation

skriss
Copy link
Contributor

@skriss skriss commented Jan 24, 2019

Signed-off-by: Steve Kriss [email protected]

The first time Velero is performing an operation against a restic repository, it first checks to see if the repo exists, and if not, it initializes it. Today, the way it checks if the repo exists is by running the restic check command with the right flags and auth, and sees if that succeeds.

Turns out that this command can be quite slow the first time that it's run on a host/container that doesn't already have a local restic cache set up (e.g. in Cluster B's Velero pod in a Cluster A -> Cluster B migration). An alternate command, restic stats, is much faster and also achieves the desired outcome of ensuring the restic repo exists and can be authenticated to.

I've tested this manually and can replicate the slow behavior with the current code, and can confirm that the updated code is much faster.

@@ -162,10 +162,10 @@ func (c *resticRepositoryController) initializeRepo(req *v1.ResticRepository, lo
})
}

// ensureRepo first checks the repo, and returns if check passes. If it fails,
// ensureRepo first tries to connect to the repo, and returns if it succeeds. If it fails,
// attempts to init the repo, and returns the result.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit pick: should be "it attempts".

Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the immense nit pick, lgtm. 👍

Copy link
Contributor

@nrb nrb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Willing to hold off on merging if the comment's going to be fixed, otherwise I'm fine merging this as is.

@skriss
Copy link
Contributor Author

skriss commented Feb 6, 2019

addressed.

@nrb nrb merged commit 46e8766 into vmware-tanzu:master Feb 6, 2019
@skriss skriss deleted the restic-stats branch February 7, 2019 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants