Accounting: limit storage space used by different parties

[warner]

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.

[warner]

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.

[davidsarah]

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]

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]

​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]

Following-up to my comment:6, 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]

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.

[davidsarah]

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.

[davidsarah]

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.

[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.

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

[zooko]

​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]

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]

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

[davidsarah]

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 (when finished) 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.

[nejucomo]

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]

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."
  2. Upload/write restriction based on authority in the storage server. (Selective request denial.)
  3. Authority creation UI: commandline
  4. Authority creation UI: webapi (Is this a goal? I did not see it in the overview.)
  5. 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.
  2. Backwards compatibility with storage servers that don't speak accounting.
  3. The ability to provide account authority in requests with a webapi.
     • Updated webapi.rst describing the interface.
  4. Commandline tools for passing explicit account authorities.
  5. 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]

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.)

[nejucomo]

Replying to daira:

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.

[daira]

Replying to nejucomo:

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.

[daira]

Replying to daira:

(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.

[meejah]

Ticket retargeted after milestone closed (editing milestones)