#666 new task

Accounting: limit storage space used by different parties

Reported by: warner Owned by: davidsarah
Priority: major Milestone: 2.0.0
Component: code-storage Version: 1.3.0
Keywords: accounting Cc:
Launchpad Bug:

Description (last modified by 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.

Change History (22)

comment:1 Changed at 2009-05-24T01:23:47Z by warner

  • Description modified (diff)

comment:2 Changed at 2010-02-23T03:09:38Z by zooko

  • Milestone changed from undecided to 2.0.0

comment:3 Changed at 2011-11-20T19:58:59Z by 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.

comment:4 Changed at 2012-09-28T01:29:22Z by 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.

comment:5 follow-up: Changed at 2012-09-29T12:36:41Z by 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.

comment:6 Changed at 2012-09-29T12:42:14Z by 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.

comment:7 Changed at 2012-09-29T12:44:04Z by 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.

comment:8 follow-up: Changed at 2012-09-29T12:52:10Z by 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.

comment:9 in reply to: ↑ 8 Changed at 2012-09-29T18:41:33Z by 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.

comment:10 in reply to: ↑ 5 Changed at 2012-09-29T18:42:22Z by 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.

comment:11 follow-up: Changed at 2012-09-30T08:47:36Z by 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?

comment:12 Changed at 2012-09-30T09:24:15Z by 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?

comment:13 Changed at 2012-09-30T09:40:01Z by 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.

comment:14 Changed at 2012-09-30T19:06:21Z by 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

comment:15 in reply to: ↑ 11 Changed at 2012-10-01T01:41:03Z by 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 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.

Last edited at 2012-10-01T01:43:46Z by davidsarah (previous) (diff)

comment:16 Changed at 2012-11-13T20:29:15Z by Zancas

  • Owner set to davidsarah

comment:17 Changed at 2013-07-04T18:22:20Z by 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"
  • #2012 "Translate accounting-overview.txt to rst format."
  • #2013 "Crosslink accounting-overview.txt and ticket #666."

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

Last edited at 2013-07-04T18:54:16Z by nejucomo (previous) (diff)

comment:18 follow-up: Changed at 2013-07-04T18:51:34Z by 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?

comment:19 in reply to: ↑ 18 ; follow-ups: Changed at 2013-07-05T19:54:02Z by 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.)

comment:20 in reply to: ↑ 19 ; follow-up: Changed at 2013-07-05T23:14:58Z by 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.

comment:21 in reply to: ↑ 20 Changed at 2013-07-07T19:42:41Z by 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.

comment:22 in reply to: ↑ 19 Changed at 2013-07-07T19:47:06Z by 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.

Note: See TracTickets for help on using tickets.