v3: Implement status socket support for v3

[asn-d6]

There is currently no status socket support for v3. We need to implement this since people seem to use it. Patches welcome.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

This issue now has a funding of 1500.0 DAI (1500.0 USD @ $1.0/DAI) attached to it.

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Chat
• $127,978.91 more funded OSS Work available on the Gitcoin Issue Explorer

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

Work has been started.

These users each claimed they can complete the work by 1 month from now.
Please review their action plans below:

1) vporton has been approved to start work.

I know Django very well. I also know Unix sockets. Please allow me to implement it.

Learn more on the Gitcoin Issue Details page.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

Work for 1500.0 DAI (1500.0 USD @ $1.0/DAI) has been submitted by:

1. @vporton

@ceresstation please take a look at the submitted work:

• PR by @vporton

---

• Learn more on the Gitcoin Issue Details page
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Chat
• $140,040.21 more funded OSS Work available on the Gitcoin Issue Explorer

[asn-d6]

Hello there, thanks for your work!

A few things to complete this task:

1. We need documentation on how exactly the status socket works and what kind of messages it transmits.
2. We need to unify the old v2 code (see hs_v2/status.py) with the new v3 code wherever possible to avoid code duplication.
3. We need unittests and coverage.
4. We will need some more status signals specific for v3.
   • In particular, the status socket should have a heartbeat message with a bunch of statistics. The purpose here is two-fold: We learn useful info as part of the heartbeat (e.g. number of descriptor publishes/fetches, number of consensus fetches), and the heartbeat can also be used a s a ping so that the other end can realize when onionbalance dies for any reason.
5. We need a squashed clean branch.

[vporton]

I don't quite understand about the heartbeat:

In the v2 (whose logic I copied to v3) the socket is usually closed by onionbalance after a very little amount of time (after transmitting all the information) after requesting information from it. So there is no time available for heartbeat.

Or do you want the socket logic in v3 be wastly different than in v2, not closing the socket after request? It was not specified so in the original issue.

With the current (copied from v2) logic, the socket already can be used "as a ping" without heartbeat, simply by opening and closing it say every second. It is even easier to use than a special heartbeat. So is heartbeat necessary?

Even if heartbeat is not necessary, which information should I add to the socket output? Number of descriptor publishes/fetches, number of consensus fetches, these things?

[FedericoCeratto]

FYI there's a PR for a simple dashboard that consumes the status socket: https://github.com/DonnchaC/onionbalance/pull/54/files

[FedericoCeratto]

tor uses /var/run/tor/control for its own control protocol and using the same name might be misleading as we are talking different protocols. Maybe renaming the file obstatus or status_fifo

[asn-d6]

I don't quite understand about the heartbeat:

In the v2 (whose logic I copied to v3) the socket is usually closed by onionbalance after a very little amount of time (after transmitting all the information) after requesting information from it. So there is no time available for heartbeat.

Or do you want the socket logic in v3 be wastly different than in v2, not closing the socket after request? It was not specified so in the original issue.

With the current (copied from v2) logic, the socket already can be used "as a ping" without heartbeat, simply by opening and closing it say every second. It is even easier to use than a special heartbeat. So is heartbeat necessary?

You are right, thats indeed easier to use than a special heartbeat so no need to do that. While this has been requested in the past ,after discussing this with other OB devs, I decided that there is absolutely no use for that. Sorry for the confusion and thanks for pointing this out.

Even if heartbeat is not necessary, which information should I add to the socket output? Number of descriptor publishes/fetches, number of consensus fetches, these things?

Sorry for not specifying this on the original post. Information has been garbled when we transitioned repositories from OBv2 to OBv3.

To recap here is what we need for this ticket:

• The status socket should provide the information requested by Alec in the respective OBv2 ticket
  . That plus what it currently has.
• The status socket location and filename should be configurable through the OBv3 config file. Currently there is no clean way to pass such configuration through the OBv3 config file, but it shouldn't be too hard since it's yaml. Please do this in a clean OBv3-specific way because the way OBv2 did it was too ugly.
• We've gotten complains in the past by people who tried to parse the socket output (e.g. for netdata) that it's a custom format. Let's use json for the format of v3 status socket from here and after.
• We need documentation (see docs/) on how status socket works and the format of the mesages it transmits.
• Unify v2 and v3 status code wherever possible (see common/)
• Unittests for the status socket and full coverage if possible.
• A clean squashed branch.

Bonus points if you write a small plugin for netdata that consumes the output of the socket. This has been asked before by onionbalance operators. I'm not adding it as a requirement but it would be great if it was done.

Thanks a lot for the help and happy hacking!

[vporton]

tor uses /var/run/tor/control for its own control protocol and using the same name might be misleading as we are talking different protocols. Maybe renaming the file obstatus or status_fifo

It is /var/run/onionbalance/control not /var/run/tor/control. I can rename it anyway, but then should rename it in v2, too! Do renaming in this branch?

[vporton]

From DonnchaC/onionbalance#60: "the time of the last attempt to assemble a new master descriptor, and status thereof".

What does the word "assemble" mean? Which function "assembles"?

[vporton]

• Unittests for the status socket and full coverage if possible.

Clearly not possible:

Unittesting this code would require its complete rewrite with dependency injection (because existing code uses the server state not something that could be put into a unit test as an object). That would introduce rather than solve bugs, what is directly contrary to the purpose of unittesting. Clearly, big work not worth to be done.

[asn-d6]

It is /var/run/onionbalance/control not /var/run/tor/control. I can rename it anyway, but then should rename it in v2, too! Do renaming in this branch?

Let's not change the v2 behavior, but yes we can rename the default v3 filename. Perhaps something like ./obv3_status_socket would work.
Also, as mentioned above, it should be possible for the operator to configure the filepath and filename.

From DonnchaC/onionbalance#60: "the time of the last attempt to assemble a new master descriptor, and status thereof".

What does the word "assemble" mean? Which function "assembles"?

That would be _publish_descriptor().

• Unittests for the status socket and full coverage if possible.

Clearly not possible:

Unittesting this code would require its complete rewrite with dependency injection (because existing code uses the server state not something that could be put into a unit test as an object). That would introduce rather than solve bugs, what is directly contrary to the purpose of unittesting. Clearly, big work not worth to be done.

I think unittests are essential here. In particular, I'd like unittests that check how we go from onionbalance config file to status socket filepath, and unittests that check the status socket output given various states of onionbalance (in terms of services, instances, descriptors, intro points).

I'm not sure what you mean by "dependency injection" but it should be possible to write clean unittests for this feature by using mocking (e.g. see ./test/v3/test_v3_hashring.py). Let me know if there is another complexity I'm not aware of and I can try to help you.

Thanks a lot for the great work :)

[vporton]

From DonnchaC/onionbalance#60: "the time of the last attempt to assemble a new master descriptor, and status thereof".
What does the word "assemble" mean? Which function "assembles"?

That would be _publish_descriptor().

Is "master descriptor" the same as "first descriptor"? Or are both attempts to publish first descriptor and attempts to publish second descriptor attempts to assemble a new master descriptor?

[asn-d6]

From DonnchaC/onionbalance#60: "the time of the last attempt to assemble a new master descriptor, and status thereof".
What does the word "assemble" mean? Which function "assembles"?

That would be _publish_descriptor().

Is "master descriptor" the same as "first descriptor"? Or are both attempts to publish first descriptor and attempts to publish second descriptor attempts to assemble a new master descriptor?

The concept of 'master descriptor' does not exist in v3. Both 'first' and 'second' are master descriptors. So we need publish timestamps for both of them.

[vporton]

"the time of the last attempt to assemble a new master descriptor, and status thereof"

Should this time be updated every time _publish_descriptor() is called or only if _should_publish_descriptor_now() return True?

[asn-d6]

"the time of the last attempt to assemble a new master descriptor, and status thereof"

It should only be updated when we actually publish a descriptor. So yeah def only after _should_publish_descriptor_now() returns True and probably towards the end of the function. Perhaps near the log line.

[vporton]

"the time of the last attempt to assemble a new master descriptor, and status thereof"

What is "status thereof"? What to show yet?

[asn-d6]

"the time of the last attempt to assemble a new master descriptor, and status thereof"

What is "status thereof"? What to show yet?

Status is whether the upload succeded or failed. In theory we can get the status using the events from handle_new_desc_event(). I think this will be pretty complicated for this iteration so perhaps you can assume that any descriptor that got uploaded was successful for now.

[vporton]

unittests that check how we go from onionbalance config file to status socket filepath

Do you really want to unittest this single Python line?

        status_socket_location = my_onionbalance.config_data.get('status-socket-location',
                                                                 params.STATUS_SOCKET_LOCATION)

[asn-d6]

unittests that check how we go from onionbalance config file to status socket filepath

Do you really want to unittest this single Python line?

        status_socket_location = my_onionbalance.config_data.get('status-socket-location',
                                                                 params.STATUS_SOCKET_LOCATION)

Hello. I was thinking that a unittest here would start with read_config_data_from_file with a test config file. It would then test the line you mentioned (and perhaps the env variable approach too). For example you can put the code from https://github.com/asn-d6/onionbalance/pull/30/files#diff-7226c83f11bab4c2a39e8c7c4581c692R30 into a function and then use the resulting socket (and a fake my_onionbalance object) to test deeper into its output.

Hope that makes sense.

[vporton]

All done! Well, except "write a small plugin for netdata that consumes the output of the socket". How much will I receive in addition if I do this additional task?

[asn-d6]

Looking pretty good @vporton ! Pointed out a bunch of stuff and we should be close to done.

About how much more you will receive if you do the netdata thing, I don't really control the financial here, so I don't think I can expand the bounty... :/

[asn-d6]

OK this looks and tests good. A final round of revisions and this should be good to go.
I will also need a squashed and clean branch to merge this (also some reasonable commit msgs)

[asn-d6]

Merged! Thanks!

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

The funding of 1500.0 DAI (1500.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @vporton.

• Learn more on the Gitcoin Issue Details page
• Questions? Checkout Gitcoin Help or the Gitcoin Chat
• $176,288.50 more funded OSS Work available on the Gitcoin Issue Explorer