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

Add --max-size flag to limit size of repositories #72

Merged
merged 11 commits into from
Jun 14, 2018
Merged

Add --max-size flag to limit size of repositories #72

merged 11 commits into from
Jun 14, 2018

Conversation

mholt
Copy link
Contributor

@mholt mholt commented Jun 11, 2018

The --max-size limits the number of bytes that can be stored to a repo. It only takes effect on SaveBlob.

Closes #65

Can see previous discussion at https://github.com/mholt/rest-server/pull/1

/cc @dotlambda

@mholt
Copy link
Contributor Author

mholt commented Jun 12, 2018

@fd0 I have refactored to make the changes you recommended over in the other PR. But let's move the discussion here. :)

@mholt
Copy link
Contributor Author

mholt commented Jun 12, 2018

There are a "lot" of changed lines because I made the methods get a *Server instead of Server copy; I think that's needed for correctness of accessing the repoSize value atomically.

Also, I now account for the size as blobs are deleted, for example when a prune happens.

stat, err := os.Stat(path)
if err != nil {
if err == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically unrelated to my PR's purpose, but this fixes what I think was a bug that existed before, and with my PR, would affect the size counting if left unfixed.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, great catch

@mholt
Copy link
Contributor Author

mholt commented Jun 13, 2018

CI fails because golint is broken right now:

The command "go get -u github.com/golang/lint/golint" failed and exited with 2 during .

Copy link
Member

@fd0 fd0 left a comment

Choose a reason for hiding this comment

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

LGTM, just one minor thing.

@@ -41,6 +41,7 @@ func init() {
flags.BoolVar(&server.Debug, "debug", server.Debug, "output debug messages")
flags.StringVar(&server.Listen, "listen", server.Listen, "listen address")
flags.StringVar(&server.Log, "log", server.Log, "log HTTP requests in the combined log format")
flags.Int64Var(&server.MaxRepoSize, "max-size", server.MaxRepoSize, "the maximum size of the repository in bytes")
Copy link
Member

Choose a reason for hiding this comment

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

Hm, why is the default (the third parameter) set to server.MaxRepoSize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just in case the server variable is ever initialized with a default MaxRepoSize, then it will be preserved by the flag, instead of overwriting it with 0 or some hard-coded default in a different place. That's all. :)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, fine with me ;)

@mholt mholt merged commit a87d968 into restic:master Jun 14, 2018
payne8 pushed a commit to payne8/rest-server that referenced this pull request Dec 3, 2018
* Add --max-size flag to limit repository size

* Only update repo size on successful write

* Use initial size as current size for first SaveBlob

* Apply LimitReader to request body

* Use HTTP 413 for size overage responses

* Refactor size limiting; do checks after every write

* Remove extra commented lines, d'oh

* Account for deleting blobs when counting space usage

* Remove extra commented line

* Fix unrelated bug (inverted err check)

* Update comment to trigger new CI build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants