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

Adding used storage quota to borg info #7121

Merged
merged 35 commits into from
Dec 24, 2022

Conversation

francoo98
Copy link
Contributor

@francoo98 francoo98 commented Nov 3, 2022

Fixes #2870.

Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

also: please run the test locally before pushing to github.

you can just invoke pytest or tox in your workdir.

@ThomasWaldmann
Copy link
Member

The quota info should be persisted in the hints file.

@francoo98
Copy link
Contributor Author

francoo98 commented Nov 4, 2022

I'm running the tests on my computer. I have a lot of failing tests because RemoteArchiver doesn't have the attr storage_quota.

In the new implementarion of get_storage_quota there is an unhandled exception (raised by msgunpack), it is raised when the hints file cannot be read, in this case should I skip the line or should I add an error?
With an error it could like:

Security dir: /home/user/.config/borg/security/78e961e6605e242750c7a76edb72688d81deasdfqwerasdf12431234
Storage quota: Error reading hints file.
Original size: 652.18 MB

self.cmd(f"--repo={self.repository_location}", "rcreate", RK_ENCRYPTION, "--storage-quota=1G")
self.cmd(f"--repo={self.repository_location}", "create", "test", "input")
info_repo = self.cmd(f"--repo={self.repository_location}", "rinfo")
assert "Storage quota" in info_repo
Copy link
Member

Choose a reason for hiding this comment

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

would be good if this also tested the 2 values (used/total).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a problem with this. In the test I added the used quota is different if I execute the test file or if I execute the test on its own.
The difference is in the tens of byte I think, so I made the file in the test bigger to make it work.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds strange, considering that the test rcreates a new repository in both cases.

Hmm, due to it using new key material, the chunker might behave differently, cutting different chunks, leading to slightly different compressed chunk sizes or different chunk counts.

Try replacing RK_ENCRYPTION with "--encryption=none" to avoid this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also fails without encryption

Copy link
Member

Choose a reason for hiding this comment

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

So, this is kind of resolved using the bigger test file now?

@ThomasWaldmann
Copy link
Member

btw, there is already a repository.info method that could also return quota infos.

@ThomasWaldmann
Copy link
Member

are you still working on this? some stuff still to do, see my comments.

@francoo98
Copy link
Contributor Author

I've been busy the last two weeks. I'm gonna be able to resume next Monday I think.

@francoo98
Copy link
Contributor Author

francoo98 commented Dec 14, 2022

I can't find the cause of the error when working with a remote repository.
Reading the code I found out that (with remotes repos) the info method cannot call get_transaction_id, it returns None. I tested this in the master branch too.
Do you know why this happens?

By the way, do you think everything else I have worked on is finished?

@ThomasWaldmann
Copy link
Member

Hmm, does the problem with remote repos only happen when testing manually? The tests on github CI do not show a problem.

@francoo98
Copy link
Contributor Author

francoo98 commented Dec 14, 2022

The tests were passing because of the diaper pattern. I removed it.
I'm seeing that the tests are passing with python 3.9. Maybe it is a problem with newer version of python. (It isn't)

@ThomasWaldmann
Copy link
Member

It would fail for all python versions. That you do not see failure for all is just because the fail-fast setting is active on the github CI, so it terminates all jobs after the first one detected a failure.

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Dec 15, 2022

TypeError('%d format: a real number is required, not NoneType')
repository.py:570 in _unpack_hints
    hints_path = os.path.join(self.path, "hints.%d" % transaction_id)

Triggered by:

remote.py:622: in do_open
    info = self.info()

It looks like it calls info() from there just to get the repo id and mode. Check if it needs the quota info there.

The problem might be that if do_open is called with create=True, the index.nnnn file will not exist because it is a fresh repo just being created. Thus transaction_id is None.

@francoo98
Copy link
Contributor Author

That was the case, there was no hints file. But this only happens with remote repos, when creating a local one the files hints, integrity, etc are created.

@francoo98
Copy link
Contributor Author

Is there a reason why you haven't seen this?

@ThomasWaldmann
Copy link
Member

Is there a reason why you haven't seen this?

I've seen it, just needed some time to have a deeper look at the code.

Overall, it looks pretty good now, just 2 minor changes.

After that I guess it can be squashed together and merged.

Copy link
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

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

LGTM

@ThomasWaldmann ThomasWaldmann merged commit ab465a7 into borgbackup:master Dec 24, 2022
@francoo98 francoo98 mentioned this pull request Dec 31, 2022
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.

query quota set / use from repository
3 participants