Opened 4 years ago

Closed 4 years ago

#15989 closed enhancement (implemented)

Add AccountingRule in and out

Reported by: teor Owned by:
Priority: Low Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: lorax tor-relay easy
Cc: charles@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

AccountingRule max is often used for services that actually only charge for bandwidth out, such as AWS.

There may also be services that only charge for bandwidth in, although it seems somewhat unlikely. However, it should be included for completeness.

The current AccountingRule max and AccountingRule sum implementation assumes that an increase in outgoing bandwidth always increases the amount being measured. Therefore, it disabes the DirPort when AccountingMax is in effect.

It might make sense to only do this for AccountingRules max, sum, and out; and not for AccountingRule in.

Child Tickets

Change History (17)

comment:1 Changed 4 years ago by teor

Type: defectenhancement

comment:2 Changed 4 years ago by nickm

Milestone: Tor: 0.2.???

comment:3 Changed 4 years ago by teor

I wouldn't mind an override for the automatic disabling of DirPort with AccountingRules max, sum, and out. This would fit well with the exception for AccountingRule in.

comment:4 Changed 4 years ago by teor

Keywords: easy added

comment:5 Changed 4 years ago by teor

We could call the DirPort override AccountingEnableDirPort (autobool).

comment:6 Changed 4 years ago by teor

See #16636 for AccountingSetRead and AccountingSetWritten.

See https://lists.torproject.org/pipermail/tor-relays/2015-July/007430.html for a user request for AccountingRule out.

comment:7 Changed 4 years ago by unixninja92

Cc: charles@… added
Severity: Normal
Status: newneeds_review

https://github.com/unixninja92/Tor/tree/ticket15989
This is a patch for AccountRule in and out. I would add AccountingEnableDirPort, but I have yet to think of a good way to explain it in the documentation.

comment:8 Changed 4 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.2.8.x-final

Thanks for the patch. The core of the changes looks great. (And we can work on AccountingEnableDirPort later if you'd like.)

Just a few minor tweaks to the GETINFO/logging changes:

  • For "accounting/bytes", we don't change it based on the accounting rule. Tor always provides the read and written values. We need to continue to provide both values, otherwise, we might break tools that rely on there being two values.
    • (If tools want to know how much is left under the current rule, they can use "accounting/bytes-left".)
  • In "accounting/bytes-left", please provide values for read and written even if the accounting rule is IN or OUT. (Tools might rely on there being two values.) I suggest using limit when read or written is unlimited.
    • Note that the order of the values is read then written, so IN will be read-left limit and OUT will be limit written-left.
  • the code in log_accounting that handles SUM is wrong, and the current format isn't great for IN or OUT either. Can we change the format to:
    log_notice(LD_HEARTBEAT, "Heartbeat: Accounting enabled. "
          "Sent: %s, Received: %s, Used: %s / %s, Rule: %s. The "
          "current accounting interval ends on %s, in %s.",
          acc_sent, acc_rcvd, acc_used, acc_max, acc_rule, end_buf, remaining);
    
    Where acc_used is bytes_to_usage(get_accounting_bytes()), and acc_rule is an accounting rule string like the one in the man page?

A minor nitpick:

The changes file has no newline at the end of the file. This might screw up our ChangeLog scripts.

comment:9 Changed 4 years ago by unixninja92

Pushed changes.
Thanks for reviewing so quickly :)

Last edited 4 years ago by unixninja92 (previous) (diff)

comment:10 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

(I'm awake and working and not in the middle of something!)

Replying to unixninja92:

In log_accounting for SUM we shouldn't have to double acc_bytes, right?

Yeah, I have no idea why that was implemented. Let's get rid of it.

Other changes are pushed.

Thanks! Looks great.

One more issue:

In options_validate, the text should mention in and out:

REJECT("AccountingRule must be 'sum' or 'max'");

Once those two changes are done, I think this is good to go.

comment:11 Changed 4 years ago by unixninja92

All fixed!

comment:12 Changed 4 years ago by teor

Status: needs_revisionneeds_review

OK, let's get this feature squashed and merged!
(nickm will look at this at some point and make a merge decision.)

comment:13 Changed 4 years ago by unixninja92

Does tor have squashing guidelines? I'm familiar with Freenet's, but they have them because they use git commit messages for change logs.

comment:14 in reply to:  13 Changed 4 years ago by teor

Replying to unixninja92:

Does tor have squashing guidelines? I'm familiar with Freenet's, but they have them because they use git commit messages for change logs.

Oh, nickm would likely have squashed all your commits into one before merging it.

For a change this size, one commit is ok, even though it includes the bugfix and new feature. (The bugfix is on a log message, so we're not going to backport it.)

comment:15 Changed 4 years ago by unixninja92

Ok, sounds good :)

comment:16 Changed 4 years ago by nickm

Whoa, that's pretty straightforward. Squashed and merged. Creating a ticket for the secondary issue so it has a number, then closing it. (#18024)

comment:17 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.