Accounting: limit storage space used by different parties #666

New issue

Open

opened 2009-03-24 01:57:08 +00:00 by warner · 20 comments

warner commented

2009-03-24 01:57:08 +00:00

Please see source:docs/proposed/accounting-overview.txt for a description of how I currently think Accounting should work. Also look at the AccountingDesign page for some older work.

Please see source:docs/proposed/accounting-overview.txt for a description of how I currently think Accounting should work. Also look at the [AccountingDesign](wiki/AccountingDesign) page for some older work.

warner added the

labels 2009-03-24 01:57:08 +00:00

warner added this to the undecided milestone 2009-03-24 01:57:08 +00:00

zooko modified the milestone from undecided to 2.0.0

2010-02-23 03:09:38 +00:00

warner commented

2011-11-20 19:58:59 +00:00

Author

My current work on this is in my github branch at:

https://github.com/warner/tahoe-lafs/tree/accounting

(be aware that I rebase that branch frequently)

It depends upon the signed-introducer-announcements from #466 .
The code is still rough, and needs both more design work and more implementation work, as well as proper tests. But the general idea is visible.

My current work on this is in my github branch at: <https://github.com/warner/tahoe-lafs/tree/accounting> (be aware that I rebase that branch frequently) It depends upon the signed-introducer-announcements from #466 . The code is still rough, and needs both more design work and more implementation work, as well as proper tests. But the general idea is visible.

daira commented

2012-09-28 01:29:22 +00:00

I've taken over this work for now; the current branch is https://github.com/davidsarah/tahoe-lafs/tree/666-accounting .

Our current design involves adding an sqlite LeaseDB to hold lease information local to the storage server (even when the actual shares are stored remotely, e.g. by the new cloud backend). This replaces the leases held in share files; the share file format does not change but any lease information there is ignored, and discarded if mutable shares are modified.

I've taken over this work for now; the current branch is <https://github.com/davidsarah/tahoe-lafs/tree/666-accounting> . Our current design involves adding an sqlite LeaseDB to hold lease information local to the storage server (even when the actual shares are stored remotely, e.g. by the new cloud backend). This replaces the leases held in share files; the share file format does not change but any lease information there is ignored, and discarded if mutable shares are modified.

zooko commented

2012-09-29 12:36:41 +00:00

I'm reviewing this branch, starting with this file:

https://github.com/davidsarah/tahoe-lafs/blob/666-accounting/src/allmydata/storage/leasedb.py

And, also starting with this series of patches:

https://github.com/davidsarah/tahoe-lafs/commits/666-accounting?page=3

One thing that would help is a document in the docs directory with a rationale for the leasedb's "coming and going" state machine.

I'm reviewing this branch, starting with this file: <https://github.com/davidsarah/tahoe-lafs/blob/666-accounting/src/allmydata/storage/leasedb.py> And, also starting with this series of patches: <https://github.com/davidsarah/tahoe-lafs/commits/666-accounting?page=3> One thing that would help is a document in the docs directory with a rationale for the leasedb's "coming and going" state machine.

zooko commented

2012-09-29 12:42:14 +00:00

(https://github.com/davidsarah/tahoe-lafs/blob/666-accounting/src/allmydata/storage/leasedb.py#L171)

I don't understand the comment about a share having been deleted from on-disk. So the case is that the share itself may be gone, but the share entry in the leasedb is still in place, and then someone calls add_new_share on it? And this triggers a dbutil.IntegrityError, but we want to resolve it as a success? The FIXME sounds a little scary. I guess the fix would be to see whether this particular dbutil.IntegrityError instance is being raised because the row (or at least the primary key) already exists in the table vs. some other integrity error.

(https://github.com/davidsarah/tahoe-lafs/blob/666-accounting/src/allmydata/storage/leasedb.py#L171) I don't understand the comment about a share having been deleted from on-disk. So the case is that the share itself may be gone, but the share entry in the leasedb is still in place, and then someone calls `add_new_share` on it? And this triggers a `dbutil.IntegrityError`, but we want to resolve it as a success? The `FIXME` sounds a little scary. I guess the fix would be to see whether this particular `dbutil.IntegrityError` instance is being raised because the row (or at least the primary key) already exists in the table vs. some other integrity error.

zooko commented

2012-09-29 12:44:04 +00:00

Following-up to my comment:371182, I guess a good unit test is to put in a fake for self._cursor which raises a different IntegrityError (i.e., signalling some sort of corruption or bug), and then have a test which calls add_new_share and the code under test fails unless it propagates that IntegrityError to its caller.

Following-up to my [comment:371182](/tahoe-lafs/trac/issues/666#issuecomment-371182), I guess a good unit test is to put in a fake for `self._cursor` which raises a different `IntegrityError` (i.e., signalling some sort of corruption or bug), and then have a test which calls `add_new_share` and the code under test fails unless it propagates that `IntegrityError` to its caller.

zooko commented

2012-09-29 12:52:10 +00:00

It would be simpler, and probably just as efficient, to call sqlite's self._db.commit() method every time instead of maintaining a dirty flag in a member variable and calling self._db.commit() only when that flag is True. Maintaining that flag gives us opportunities to write bugs, and also it adds load to reviewers -- I don't understand why there is an always option and why it is used from the history writer. If we always just call self._db.commit() then I as a reviewer wouldn't have to wonder about that and would have more cognitive bandwidth leftover to look for other issues.

It would be simpler, and probably just as efficient, to call sqlite's `self._db.commit()` method every time instead of maintaining a dirty flag in a member variable and calling `self._db.commit()` only when that flag is True. Maintaining that flag gives us opportunities to write bugs, and also it adds load to reviewers -- I don't understand why there is an `always` option and why it is used from the history writer. If we always just call `self._db.commit()` then I as a reviewer wouldn't have to wonder about that and would have more cognitive bandwidth leftover to look for other issues.

daira commented

2012-09-29 18:41:33 +00:00

Replying to zooko:

It would be simpler, and probably just as efficient, to call sqlite's self._db.commit() method every time instead of maintaining a dirty flag in a member variable and calling self._db.commit() only when that flag is True. Maintaining that flag gives us opportunities to write bugs, and also it adds load to reviewers -- I don't understand why there is an always option and why it is used from the history writer. If we always just call self._db.commit() then I as a reviewer wouldn't have to wonder about that and would have more cognitive bandwidth leftover to look for other issues.

This was in Brian's code, to allow reducing the number of DB transactions, but I agree it is possibly a premature optimization. I'll think about removing it.

Replying to [zooko](/tahoe-lafs/trac/issues/666#issuecomment-371184): > It would be simpler, and probably just as efficient, to call sqlite's `self._db.commit()` method every time instead of maintaining a dirty flag in a member variable and calling `self._db.commit()` only when that flag is True. Maintaining that flag gives us opportunities to write bugs, and also it adds load to reviewers -- I don't understand why there is an `always` option and why it is used from the history writer. If we always just call `self._db.commit()` then I as a reviewer wouldn't have to wonder about that and would have more cognitive bandwidth leftover to look for other issues. This was in Brian's code, to allow reducing the number of DB transactions, but I agree it is possibly a premature optimization. I'll think about removing it.

daira commented

2012-09-29 18:42:22 +00:00

Replying to zooko:

One thing that would help is a document in the docs directory with a rationale for the leasedb's "coming and going" state machine.

+1, I will add that.

Replying to [zooko](/tahoe-lafs/trac/issues/666#issuecomment-371181): > One thing that would help is a document in the docs directory with a rationale for the leasedb's "coming and going" state machine. +1, I will add that.

zooko commented

2012-09-30 08:47:36 +00:00

(https://github.com/davidsarah/tahoe-lafs/blob/666-accounting/src/allmydata/storage/leasedb.py#L223) says "delete leases first to maintain integrity constraint", but there isn't actually an invariant that a lease exists only if a share exists -- that invariant would be broken by the user rm'ing a share externally.

Also, shouldn't it check that the share is already marked as "going", and raise an exception if not?

zooko commented

2012-09-30 09:24:15 +00:00

(https://github.com/davidsarah/tahoe-lafs/blob/666-accounting/src/allmydata/storage/account.py#L82)

"# XXX do we actually need this?"

Well, there are no current callers of it. I guess it would be used when a mutable share is changed to a new size, right?

(https://github.com/davidsarah/tahoe-lafs/blob/666-accounting/src/allmydata/storage/account.py#L82) "# XXX do we actually need this?" Well, there are no current callers of it. I guess it would be used when a mutable share is changed to a new size, right?

zooko commented

2012-09-30 09:40:01 +00:00

I'm stopping for a bit. I've been reading through the source, but I don't feel like I understand the overall design, which is the thing that davidsarah asked me to look at most closely. It would help if there were a design document summarizing that. If I read that document first, that would probably help me understand the source code better.

warner commented

2012-09-30 19:06:21 +00:00

Author

server-side control panels / dashboards to show how much space is being used, and enable/disable access
client-side dashboard to show how much space we're using on each server
eventually mechanisms to advertise payment methods

I've split off the leasedb task to ticket #1818 (which happens to be 3*606, in case you're numerologistically inclined), to keep this ticket focussed on the longer-term Accounting issues. Leasedb is a necessary dependency for accounting (to quickly calculate things like "how much space is Alice using?"), but not sufficient. The full Accounting task will include: * server-side control panels / dashboards to show how much space is being used, and enable/disable access * client-side dashboard to show how much space we're using on each server * eventually mechanisms to advertise payment methods

daira commented

2012-10-01 01:41:03 +00:00

Replying to zooko:

https://github.com/davidsarah/tahoe-lafs/blob/666-accounting/src/allmydata/storage/leasedb.py#L223 says "delete leases first to maintain integrity constraint", but there isn't actually an invariant that a lease exists only if a share exists -- that invariant would be broken by the user rm'ing a share externally.

There is an invariant that leases exist only for shares that exist in the database:

CREATE TABLE `leases` ( ...
 FOREIGN KEY (`storage_index`, `shnum`) REFERENCES `shares` (`storage_index`, `shnum`),
... )

rm'ing the share externally doesn't break that invariant: the AccountingCrawler will call remove_deleted_share, which will delete the leases before deleting the share entry.

Also, shouldn't it check that the share is already marked as "going", and raise an exception if not?

No, because if the AccountingCrawler notices that a share has disappeared unexpectedly from backend storage, it will delete the share entry (and any leases) from the database without it ever being in STATE_GOING. STATE_GOING is for when we've sent a request to a remote backend for it to delete the objects representing share chunks, but have not received confirmation that all such objects have been deleted.

Replying to [zooko](/tahoe-lafs/trac/issues/666#issuecomment-371187): > <https://github.com/davidsarah/tahoe-lafs/blob/666-accounting/src/allmydata/storage/leasedb.py#L223> says "delete leases first to maintain integrity constraint", but there isn't actually an invariant that a lease exists only if a share exists -- that invariant would be broken by the user rm'ing a share externally. There is an invariant that leases exist only for shares that exist *in the database*: ``` CREATE TABLE `leases` ( ... FOREIGN KEY (`storage_index`, `shnum`) REFERENCES `shares` (`storage_index`, `shnum`), ... ) ``` rm'ing the share externally doesn't break that invariant: the AccountingCrawler will call `remove_deleted_share`, which will delete the leases before deleting the share entry. > Also, shouldn't it check that the share is already marked as `"going"`, and raise an exception if not? No, because if the AccountingCrawler notices that a share has disappeared unexpectedly from backend storage, it will delete the share entry (and any leases) from the database without it ever being in STATE_GOING. STATE_GOING is for when we've sent a request to a remote backend for it to delete the objects representing share chunks, but have not received confirmation that all such objects have been deleted.

daira was assigned by zancas

2012-11-13 20:29:15 +00:00

nejucomo commented

2013-07-04 18:22:20 +00:00

Hi. I want to find / create / garden "subtickets" related to accounting. This is a huge topic, so let's create bite-sized subtickets.

Related tickets (this list is incomplete, but this parent ticket should be the canonical collection of all subtickets):

#1818 "leasedb: track leases in a sqlite database, not inside shares"
- This appears to interact with #1819 and #1921.
#2012 "Translate accounting-overview.txt to rst format."
#2013 "Crosslink accounting-overview.txt and ticket #666."

Edit: Added #1819 and #1921 transitively under #1818

Hi. I want to find / create / garden "subtickets" related to accounting. This is a huge topic, so let's create bite-sized subtickets. Related tickets (this list is incomplete, but this parent ticket should be the canonical collection of all subtickets): * #1818 "leasedb: track leases in a sqlite database, not inside shares" * This appears to interact with #1819 and #1921. * #2012 "Translate accounting-overview.txt to rst format." * #2013 "Crosslink accounting-overview.txt and ticket #666." **Edit:** Added #1819 and #1921 transitively under #1818

nejucomo commented

2013-07-04 18:51:34 +00:00

Understanding the Scope of 666-accounting:

I want to understand which parts of https://tahoe-lafs.org/trac/tahoe-lafs/browser/docs/proposed/accounting-overview.txt are in scope for branch https://github.com/davidsarah/tahoe-lafs/tree/666-accounting .

Then, after I understand that, I'd like to create smaller more bite-sized tickets to serve as a roadmap and as an indicator of progress.

I want to know which of the following coarse features are present or not in that branch, as well as if these feature distinctions are accurate or useful "bite sized pieces" of the accounting-overview.txt design:

Storage Server:
1. Authority creation implementation on the server without a UI.
- This would provide a concrete specification of the certificate formats.
- Possibly with special-case uses like "create a general-public account for backwards compatibility / opt-in behavior."
1. Upload/write restriction based on authority in the storage server. (Selective request denial.)
2. Authority creation UI: commandline
3. Authority creation UI: webapi (Is this a goal? I did not see it in the overview.)
4. Backwards compatibility in the storage server.
- I consider this a separable task, because a branch could break compatibility, then fix it later, as distinct phases.
- I recall warner describing something that assigns legacy client requests to a "general public" account. That's one example implementation of this bullet.
Client-gateway:
1. The ability to provide account authority in foolscap requests, with no exposed api.
- Perhaps this includes a hardcoded "general public" request.
1. Backwards compatibility with storage servers that don't speak accounting.
2. The ability to provide account authority in requests with a webapi.
- Updated webapi.rst describing the interface.
1. Commandline tools for passing explicit account authorities.
2. Commandline tools for "account petnames" and "ambient account authority".

I'm kind of guessing here at the proper way to split up the implementation and specification work.

Desire for a roadmap:

I'd like to create tickets at a level of granularity so that we have a tangible roadmap.

If no one comments on these bullets, then when I next invest time in accounting, I'll examine the branches and try to determine which bullets are in scope for the 666-accounting branch, and which are out of scope.

For any "bite sized chunks" that I notice, I will create distinct tickets and link them into this parent ticket.

Landing Changes:

I believe it may be possible to land onto trunk/default at multiple intermediate stages. I recognize this might imply more overall work. I prefer it because it signals to the general community where the progress is.

If we don't land intermediate stages which are tracked in separate tickets, perhaps we can create a "branch666" keyword? Is that sensible?

**Understanding the Scope of 666-accounting:** I want to understand which parts of <https://tahoe-lafs.org/trac/tahoe-lafs/browser/docs/proposed/accounting-overview.txt> are in scope for branch <https://github.com/davidsarah/tahoe-lafs/tree/666-accounting> . Then, after I understand that, I'd like to create smaller more bite-sized tickets to serve as a roadmap and as an indicator of progress. I want to know which of the following coarse features are present or not in that branch, as well as if these feature distinctions are accurate or useful "bite sized pieces" of the `accounting-overview.txt` design: * Storage Server: 1. Authority creation implementation on the server without a UI. * This would provide a concrete specification of the certificate formats. * Possibly with special-case uses like "create a general-public account for backwards compatibility / opt-in behavior." 1. Upload/write restriction based on authority in the storage server. (Selective request denial.) 1. Authority creation UI: commandline 1. Authority creation UI: webapi (Is this a goal? I did not see it in the overview.) 1. Backwards compatibility in the storage server. * I consider this a separable task, because a branch could break compatibility, then fix it later, as distinct phases. * I recall warner describing something that assigns legacy client requests to a "general public" account. That's one example implementation of this bullet. * Client-gateway: 1. The ability to provide account authority in foolscap requests, with no exposed api. * Perhaps this includes a hardcoded "general public" request. 1. Backwards compatibility with storage servers that don't speak accounting. 1. The ability to provide account authority in requests with a webapi. * Updated `webapi.rst` describing the interface. 1. Commandline tools for passing explicit account authorities. 1. Commandline tools for "account petnames" and "ambient account authority". I'm kind of guessing here at the proper way to split up the implementation and specification work. **Desire for a roadmap:** I'd like to create tickets at a level of granularity so that we have a tangible roadmap. If no one comments on these bullets, then when I next invest time in accounting, I'll examine the branches and try to determine which bullets are in scope for the `666-accounting` branch, and which are out of scope. For any "bite sized chunks" that I notice, I will create distinct tickets and link them into this parent ticket. **Landing Changes:** I believe it may be possible to land onto trunk/default at multiple intermediate stages. I recognize this might imply more overall work. I prefer it because it signals to the general community where the progress is. If we *don't* land intermediate stages which are tracked in separate tickets, perhaps we can create a "branch666" keyword? Is that sensible?

daira commented

2013-07-05 19:54:02 +00:00

Replying to nejucomo:

Understanding the Scope of 666-accounting:

I want to understand which parts of https://tahoe-lafs.org/trac/tahoe-lafs/browser/docs/proposed/accounting-overview.txt are in scope for branch https://github.com/davidsarah/tahoe-lafs/tree/666-accounting .

That branch is dead, replaced by https://github.com/LeastAuthority/tahoe-lafs/tree/1819-cloud-merge.

(There was originally some accounting-related code written by warner on the 666-accounting branch that didn't make it to 1819-cloud-merge, but I'm not sure how useful it would be to restore it rather than rewriting it.)

Replying to [nejucomo](/tahoe-lafs/trac/issues/666#issuecomment-371194): > **Understanding the Scope of 666-accounting:** > > I want to understand which parts of <https://tahoe-lafs.org/trac/tahoe-lafs/browser/docs/proposed/accounting-overview.txt> are in scope for branch <https://github.com/davidsarah/tahoe-lafs/tree/666-accounting> . That branch is dead, replaced by <https://github.com/LeastAuthority/tahoe-lafs/tree/1819-cloud-merge>. (There was originally some accounting-related code written by warner on the 666-accounting branch that didn't make it to 1819-cloud-merge, but I'm not sure how useful it would be to restore it rather than rewriting it.)

nejucomo commented

2013-07-05 23:14:58 +00:00

Replying to [daira]comment:19:

Replying to nejucomo:

Understanding the Scope of 666-accounting:

I want to understand which parts of https://tahoe-lafs.org/trac/tahoe-lafs/browser/docs/proposed/accounting-overview.txt are in scope for branch https://github.com/davidsarah/tahoe-lafs/tree/666-accounting .

That branch is dead, replaced by https://github.com/LeastAuthority/tahoe-lafs/tree/1819-cloud-merge.

(There was originally some accounting-related code written by warner on the 666-accounting branch that didn't make it to 1819-cloud-merge, but I'm not sure how useful it would be to restore it rather than rewriting it.)

Good to know and document here.

Daira, are you willing and available to document which features of accounting-overview.txt are present in 1819-cloud-merge and which are not? (This may or may not follow the bullets I proposed above.)

It's fine if you are not, in which case I can take a crack at reviewing it specifically for this purpose.

Replying to [daira]comment:19: > Replying to [nejucomo](/tahoe-lafs/trac/issues/666#issuecomment-371194): > > **Understanding the Scope of 666-accounting:** > > > > I want to understand which parts of <https://tahoe-lafs.org/trac/tahoe-lafs/browser/docs/proposed/accounting-overview.txt> are in scope for branch <https://github.com/davidsarah/tahoe-lafs/tree/666-accounting> . > > That branch is dead, replaced by <https://github.com/LeastAuthority/tahoe-lafs/tree/1819-cloud-merge>. > > (There was originally some accounting-related code written by warner on the 666-accounting branch that didn't make it to 1819-cloud-merge, but I'm not sure how useful it would be to restore it rather than rewriting it.) Good to know and document here. Daira, are you willing and available to document which features of [accounting-overview.txt](https://tahoe-lafs.org/trac/tahoe-lafs/browser/docs/proposed/accounting-overview.txt) are present in [1819-cloud-merge](https://github.com/LeastAuthority/tahoe-lafs/tree/1819-cloud-merge) and which are not? (This may or may not follow the bullets I proposed above.) It's fine if you are not, in which case I can take a crack at reviewing it specifically for this purpose.

daira commented

2013-07-07 19:42:41 +00:00

Replying to [nejucomo]comment:20:

Daira, are you willing and available to document which features of accounting-overview.txt are present in 1819-cloud-merge and which are not?

I'm willing, but I'm not sure I'm available.

Replying to [nejucomo]comment:20: > Daira, are you willing and available to document which features of [accounting-overview.txt](https://tahoe-lafs.org/trac/tahoe-lafs/browser/docs/proposed/accounting-overview.txt) are present in [1819-cloud-merge](https://github.com/LeastAuthority/tahoe-lafs/tree/1819-cloud-merge) and which are not? I'm willing, but I'm not sure I'm available.

daira commented

2013-07-07 19:47:06 +00:00

Replying to [daira]comment:19:

(There was originally some accounting-related code written by warner on the 666-accounting branch that didn't make it to 1819-cloud-merge, but I'm not sure how useful it would be to restore it rather than rewriting it.)

It seems to still be available in the ds-1818* branches of https://github.com/warner/tahoe-lafs.

Replying to [daira]comment:19: > (There was originally some accounting-related code written by warner on the 666-accounting branch that didn't make it to 1819-cloud-merge, but I'm not sure how useful it would be to restore it rather than rewriting it.) It seems to still be available in the ds-1818* branches of <https://github.com/warner/tahoe-lafs>.