Opened 20 months ago

Closed 16 months ago

Last modified 16 months ago

#24256 closed enhancement (fixed)

Add a new "outdated" field to distinguish between outdated and too new tor versions

Reported by: arma Owned by: karsten
Priority: High Milestone: Onionoo 1.12.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords:
Cc: metrics-team Actual Points:
Parent ID: #25199 Points:
Reviewer: iwakeh Sponsor:

Description

https://atlas.torproject.org/#search/moria1
shows this text at the top:
"This relay is running an outdated Tor version and should be updated to a recent release of Tor that may contain important fixes."

But the relay is running "Tor 0.3.3.0-alpha-dev". This is not an outdated Tor version -- it is *newer* than any of the recommended versions.

Tor has some somewhat complicated logic for deciding whether a version is obsolete -- see tor_version_is_obsolete(). Is atlas's best plan to try to reproduce Tor's logic? Should we try to specify it, e.g. in version-spec.txt, so external programs have a chance of getting it right?

Child Tickets

Change History (38)

comment:1 in reply to:  description Changed 20 months ago by arma

Replying to arma:

Tor has some somewhat complicated logic for deciding whether a version is obsolete -- see tor_version_is_obsolete(). Is atlas's best plan to try to reproduce Tor's logic? Should we try to specify it, e.g. in version-spec.txt, so external programs have a chance of getting it right?

I opened #24257 for the Tor side of this fix.

comment:2 Changed 20 months ago by karsten

One possible way to fix this would be to change "outdated" to "not recommended" in that Atlas text and let the user figure out whether that means too old or too new. I'd guess that the relay operator knows pretty well whether they're running something experimental or haven't updated their package for a while.

However, if that's a major usability issue, let's add the logic for deciding whether "not recommended" means "outdated" to Onionoo. In that case I wouldn't mind working from a spec.

comment:3 Changed 20 months ago by nickm

Milestone: Tor: 0.3.3.x-final

comment:4 Changed 20 months ago by nickm

Milestone: Tor: 0.3.3.x-final

oops; wrong ticket.

comment:5 in reply to:  2 Changed 20 months ago by arma

Replying to karsten:

I'd guess that the relay operator knows pretty well whether they're running something experimental or haven't updated their package for a while.

And if only the relay operator used the atlas page for the relay, that would be enough. I worry that users look at the atlas pages for other relays and get scared.

comment:6 Changed 20 months ago by karsten

Component: Metrics/AtlasMetrics/Onionoo
Status: newneeds_information
Summary: atlas complains "outdated tor version" for too *new* tor versions tooAdd a new "outdated" field to distinguish between outdated and too new tor versions
Type: defectenhancement

Yes, agreed. I'm moving this ticket to Onionoo, because that's what we should work on first. Setting to needs_information until there's a spec or until somebody figures that a spec is not necessary, in which case I'd just read the tor code (#24257).

comment:7 Changed 20 months ago by karsten

I took a brief look at tor_version_is_obsolete() today. Quite a few cases there. Before I looked at that code I somehow assumed that we could divide the non-recommended bucket into outdated and experimental and be done with it. But it seems like we'd also need an unknown bucket and maybe even more.

If that's really the case we might want to avoid adding separate boolean fields for all possible statuses, like "outdated_version":true, "experimental_version":false, etc., and instead introduce a new string field like "version_status":"outdated" with pre-defined values like "recommended", "outdated" (or "obsolete"?), "experimental", "unknown", etc.

Does that make sense? What statuses would we need, and how would we call them to avoid confusing users when a client application decides to present version status values directly to its users?

comment:8 Changed 20 months ago by irl

I like the "version_status" idea. I was thinking to add the specific reasons why a version is not recommended (the ones that are affected by specific bugs as mentioned in the tor source) but I wonder if those do not ever actually see the consensus anyway and so wouldn't be appearing in Onionoo.

This would perhaps be something to add to Atlas when a whole bunch of relays are just about to disappear to explain to those users that they need to upgrade, but once the relays are not in Onionoo anymore the message would never be seen again.

comment:9 Changed 17 months ago by arma

The atlas side has opened #25199 as a related ticket.

comment:10 in reply to:  6 Changed 17 months ago by arma

Replying to karsten:

Setting to needs_information until there's a spec or until somebody figures that a spec is not necessary, in which case I'd just read the tor code (#24257).

Looks like #24257 got closed. There is a new Section 3, "Version status", in
https://gitweb.torproject.org/torspec.git/tree/version-spec.txt

comment:11 Changed 17 months ago by irl

Parent ID: #25199
Status: needs_informationnew

comment:12 Changed 17 months ago by karsten

Owner: changed from metrics-team to karsten
Status: newaccepted

I started working on this.

comment:13 Changed 17 months ago by karsten

Status: acceptedneeds_review

Please review commit 6dd9709 in my task-24256 branch.

Here's what this branch produces when run locally:

$ curl "http://localhost:8080/details?fields=version,version_status&type=relay" | sort | uniq -c
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  391k    0  391k    0     0   193k      0 --:--:--  0:00:02 --:--:--  193k
   1 "bridges":[
   1 "bridges_published":"2018-02-22 19:04:06",
   1 "build_revision":"6dd9709",
   1 "relays":[
   1 "relays_published":"2018-02-22 19:00:00",
   1 ],
   1 ]}
   2 {"version":"0.2.4.19","version_status":"obsolete"},
  15 {"version":"0.2.4.20","version_status":"obsolete"},
   5 {"version":"0.2.4.21","version_status":"obsolete"},
   9 {"version":"0.2.4.22","version_status":"obsolete"},
 148 {"version":"0.2.4.23","version_status":"obsolete"},
   9 {"version":"0.2.4.24","version_status":"obsolete"},
  81 {"version":"0.2.4.27","version_status":"obsolete"},
  96 {"version":"0.2.4.29","version_status":"obsolete"},
  10 {"version":"0.2.5.10","version_status":"obsolete"},
  64 {"version":"0.2.5.12","version_status":"obsolete"},
  57 {"version":"0.2.5.14","version_status":"obsolete"},
   2 {"version":"0.2.5.15","version_status":"obsolete"},
 398 {"version":"0.2.5.16","version_status":"recommended"},
   1 {"version":"0.2.5.16-dev","version_status":"unrecommended"},
   3 {"version":"0.2.6.0-alpha-dev","version_status":"unrecommended"},
  30 {"version":"0.2.6.10","version_status":"unrecommended"},
   1 {"version":"0.2.6.7","version_status":"unrecommended"},
   1 {"version":"0.2.6.8","version_status":"unrecommended"},
   5 {"version":"0.2.6.9","version_status":"unrecommended"},
   1 {"version":"0.2.7.1-alpha","version_status":"unrecommended"},
   1 {"version":"0.2.7.2-alpha","version_status":"unrecommended"},
   1 {"version":"0.2.7.4-rc","version_status":"unrecommended"},
   3 {"version":"0.2.7.5","version_status":"unrecommended"},
 217 {"version":"0.2.7.6","version_status":"unrecommended"},
   1 {"version":"0.2.8.10","version_status":"unrecommended"},
   1 {"version":"0.2.8.10-dev","version_status":"unrecommended"},
   5 {"version":"0.2.8.11","version_status":"unrecommended"},
   6 {"version":"0.2.8.12","version_status":"unrecommended"},
   2 {"version":"0.2.8.2-alpha","version_status":"unrecommended"},
   2 {"version":"0.2.8.6","version_status":"unrecommended"},
   7 {"version":"0.2.8.7","version_status":"unrecommended"},
  11 {"version":"0.2.8.8","version_status":"unrecommended"},
   8 {"version":"0.2.8.9","version_status":"unrecommended"},
  72 {"version":"0.2.9.10","version_status":"unrecommended"},
 176 {"version":"0.2.9.11","version_status":"unrecommended"},
  40 {"version":"0.2.9.12","version_status":"unrecommended"},
  22 {"version":"0.2.9.13","version_status":"unrecommended"},
 893 {"version":"0.2.9.14","version_status":"recommended"},
  10 {"version":"0.2.9.8","version_status":"unrecommended"},
  32 {"version":"0.2.9.9","version_status":"unrecommended"},
   2 {"version":"0.3.0.1-alpha","version_status":"unrecommended"},
 106 {"version":"0.3.0.10","version_status":"unrecommended"},
   2 {"version":"0.3.0.11","version_status":"unrecommended"},
  90 {"version":"0.3.0.13","version_status":"unrecommended"},
   2 {"version":"0.3.0.13-dev","version_status":"unrecommended"},
   1 {"version":"0.3.0.4-rc","version_status":"unrecommended"},
   1 {"version":"0.3.0.6-dev","version_status":"unrecommended"},
   3 {"version":"0.3.0.7","version_status":"unrecommended"},
  10 {"version":"0.3.0.8","version_status":"unrecommended"},
  49 {"version":"0.3.0.9","version_status":"unrecommended"},
   6 {"version":"0.3.1.2-alpha-dev","version_status":"unrecommended"},
   1 {"version":"0.3.1.4-alpha","version_status":"unrecommended"},
   1 {"version":"0.3.1.5-alpha","version_status":"unrecommended"},
 129 {"version":"0.3.1.7","version_status":"unrecommended"},
   1 {"version":"0.3.1.7-dev","version_status":"unrecommended"},
  89 {"version":"0.3.1.8","version_status":"unrecommended"},
   1 {"version":"0.3.1.8-dev","version_status":"unrecommended"},
1522 {"version":"0.3.1.9","version_status":"recommended"},
   1 {"version":"0.3.2.1-alpha","version_status":"unrecommended"},
   2 {"version":"0.3.2.2-alpha","version_status":"unrecommended"},
   6 {"version":"0.3.2.3-alpha","version_status":"unrecommended"},
   1 {"version":"0.3.2.4-alpha","version_status":"unrecommended"},
   1 {"version":"0.3.2.6-alpha","version_status":"unrecommended"},
   1 {"version":"0.3.2.7-rc","version_status":"unrecommended"},
  62 {"version":"0.3.2.8-rc","version_status":"recommended"},
   1 {"version":"0.3.2.8-rc-dev","version_status":"unrecommended"},
   1 {"version":"0.3.2.9","version_status":"recommended"}
2435 {"version":"0.3.2.9","version_status":"recommended"},
  17 {"version":"0.3.2.9-dev","version_status":"new in series"},
   3 {"version":"0.3.3.0-alpha-dev","version_status":"unrecommended"},
  43 {"version":"0.3.3.1-alpha","version_status":"recommended"},
  16 {"version":"0.3.3.1-alpha-dev","version_status":"unrecommended"},
 229 {"version":"0.3.3.2-alpha","version_status":"recommended"},
   8 {"version":"0.3.3.2-alpha-dev","version_status":"experimental"},
  16 {"version":"0.3.4.0-alpha-dev","version_status":"experimental"},
   1 {"version":"5.0",

(Note that the last line, version 5.0, is the Onionoo version, not a relay version.)

Please also check whether the reported "version_status" fields match what's written in version-spec.txt.

comment:14 Changed 17 months ago by cypherpunks

 398 {"version":"0.2.5.16","version_status":"recommended"},
   1 {"version":"0.2.5.16-dev","version_status":"unrecommended"},

it matches the spec but I find the spec strange and counterintuitive
if 0.2.5.16 is recommended I would declare 0.2.5.16-dev experimental,
but I guess the spec will not be changed.

comment:15 Changed 17 months ago by cypherpunks

actually, by the version spec 0.2.5.16-dev is "new in series"

comment:16 in reply to:  15 ; Changed 17 months ago by karsten

Replying to cypherpunks:

actually, by the version spec 0.2.5.16-dev is "new in series"

No, 0.2.5.17 is also a recommended version, there's just no relay running that version. And with 0.2.5.16 and 0.2.5.17 being recommended versions, 0.2.5.16-dev is an unrecommended version.

Anyway, if the spec is strange and counterintuitive, let's try to fix it.

comment:17 in reply to:  16 ; Changed 17 months ago by cypherpunks

Replying to karsten:

Replying to cypherpunks:

actually, by the version spec 0.2.5.16-dev is "new in series"

No, 0.2.5.17 is also a recommended version, there's just no relay running that version.
And with 0.2.5.16 and 0.2.5.17 being recommended versions, 0.2.5.16-dev is an unrecommended version.

I'm sorry, you are right.

recommended version might include unreleased versions (in preparation for upcoming releases). 0.2.5.17 is an unreleased version.

Anyway, if the spec is strange and counterintuitive, let's try to fix it.

comment:18 in reply to:  17 Changed 17 months ago by karsten

Replying to cypherpunks:

recommended version might include unreleased versions (in preparation for upcoming releases). 0.2.5.17 is an unreleased version.

Oh, I didn't know that 0.2.5.17 is unreleased, yet recommended. But anyway, we have to take what we get in the list of recommended server versions.

Thanks for checking the versions in the example output.

The code is still ready for review.

comment:19 Changed 17 months ago by iwakeh

Reviewer: iwakeh

comment:20 Changed 16 months ago by iwakeh

Status: needs_reviewneeds_information

From reading the code I'd say the use of enum 'Status' is good, but should be extended, i.e.,
instead of private inside the 'TorVersion' helper class use a public enum 'Status' that is also used elsewhere and the status String should only be used just before serialization. Using enums more reduces memory requirements.
The NPE possibly thrown at the beginning of 'TorVersion.of' would stop all of the processing. Is this intended?

comment:21 in reply to:  20 Changed 16 months ago by karsten

Status: needs_informationneeds_review

Replying to iwakeh:

From reading the code I'd say the use of enum 'Status' is good, but should be extended, i.e.,
instead of private inside the 'TorVersion' helper class use a public enum 'Status' that is also used elsewhere and the status String should only be used just before serialization. Using enums more reduces memory requirements.

Are there other places where using these specific "states" (recommended, experimental, obsolete, new in series, unrecommended) make any sense? If not, let's rather create new enums for those places.

The NPE possibly thrown at the beginning of 'TorVersion.of' would stop all of the processing. Is this intended?

Hmm, probably not. Can you describe a situation (or even better: add a test) where this would go wrong?

Thanks!

comment:22 Changed 16 months ago by iwakeh

Status: needs_reviewneeds_revision

While looking again:

  • The comparator interface implemented by TorVersion is not really used as such. Maybe remove the comparator interface and instead introduce two methods 'olderThan' and 'youngerThan'? This would also make 'determineVersionStatus' easier to read and save the unused 'hashCode' and 'equals' implementations.
  • Why not return null, if 'TorVersion.of' receives a null argument? Currently, it is only used once, where the null check is done before calling 'TorVersion.of'.
  • Making enum 'Status' public would also support de/serialization. The abbreviation could also be part of the enum and replace the switch statement in NodeStatus to, for example nodeStatus.setVersionStatus(Status.ofAbbreviation(parts[25]).toString()) and leave room for any number of new status types. And, the 'knowledge' about abbreviation vs. full description is in one place w/o repetition.

There are also some checkstyle complaints (mostly empty lines and missing javadoc).

comment:23 Changed 16 months ago by karsten

Status: needs_revisionneeds_review

Alright, I made most of the changes/fixes you suggested, except for the first: the reason is that equals() is being used (in recommendedVersions.contains(this)) and that I don't see a good reason against using a standard interface like Comparator in this case. Please take another look at my updated branch. Thanks!

comment:24 Changed 16 months ago by iwakeh

Status: needs_reviewneeds_revision

Replying to karsten:

Alright, I made most of the changes/fixes you suggested, except for the first: the reason is that equals() is being used (in recommendedVersions.contains(this))

Thanks! And true, 'equals' is used and 'hashCode' should give the same result for equal objects.

... and that I don't see a good reason against using a standard interface like Comparator in this case. Please take another look at my updated branch. Thanks!

I don't mind standard interfaces, but here the standard interface is a way bigger promise than we want to keep. It is broken, because it is inconsistent with the implementation of equals and hashCode. I put up a very simple example test here demonstrating this (not for merge; this should be replaced by tests checking the intended behavior).

We only need an order for valid tor versions for deciding the status of valid tor versions. Invalid versions are never recommendable, I assume, but currently they are classified as obsolete. Is that intended?

Another question is, if tor versions like '01.01' and '1.1' have the same 'age'? Method compareTo would currently return 0. If that is correct, these should be equal, too. Could it help to 'normalize' valid versions, i.e., turn '1.01' into '1.1'?

comment:25 Changed 16 months ago by iwakeh

Addendum:
For the sorted set SortedSet<TorVersion> a comparator only for valid versions should be given separately.

comment:26 Changed 16 months ago by karsten

Ah, I see where this is getting. Agreed, there are cases where the current implementation is broken. Let's fix it.

Invalid versions are never recommendable, and we could simply say that they are "unrecommended". So, we could change TorVersion#of to return null whenever it is given an invalid tor version.

And "01.01" should be the same as "1.1".

I pushed a fixup commit with something that might get us closer to completion, though I'm not sure if it resolves all open issues. (I might be smarter about this tomorrow, but I ran out of brains for today.)

Leaving on needs_revision. If you want to revise what's there, feel free! Otherwise, I'll take another look tomorrow. Thanks!

comment:27 Changed 16 months ago by subscriptionblocker

Perhaps it's time to lighten up a little? Absent a really serious bug, just why is anyone talking about flagging relays for a modestly obsolete version? As often as these versions change, I would think that "3ea version obsolete" would be a reasonable flagging standard.

What Tor using now is not. And I'd be very surprised if such hijinks aren't the reason many relay operators give up and walk away.

Is there a "Rain Man" quality to some of these rules? A dedication to self destruction in pursuit of some unattainable ideal?

Tor's relay count is dropping. How does Tor manage that within an environment of constant news warnings concerning the perils of centralized internet?

comment:28 Changed 16 months ago by teor

We discover a serious bug in tor every few months:
https://trac.torproject.org/projects/tor/wiki/TROVE

We obsolete old Tor versions every year due to lack of features, or accumulated minor bugs:
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/CoreTorReleases

comment:29 Changed 16 months ago by irl

Cc: metrics-team added

Adding metrics-team to cc

comment:30 Changed 16 months ago by karsten

Status: needs_revisionneeds_review

Please review my updated task-24256 branch with some more tweaks. I think it's okay now. Or at least I don't know what else to revise at this point. If something remains, please let me know.

comment:31 Changed 16 months ago by iwakeh

Status: needs_reviewmerge_ready

Looks fine. Maybe consider adding tests cases for the consistency of hash-map, equals, and Comparator?
Merge ready.

comment:32 Changed 16 months ago by karsten

Milestone: Onionoo 1.12.0
Resolution: fixed
Status: merge_readyclosed

Good idea, and thanks for checking everything! Added some more tests for those three methods, squashed, and pushed to master. Closing. Thanks!

comment:33 Changed 16 months ago by karsten

Priority: MediumHigh
Resolution: fixed
Status: closedreopened

Oops, I found two bugs while preparing the release that are both related to this enhancement. Please review commit 1ade07e in my task-24256 branch. Setting priority to high, so that we can still put out the release today.

comment:34 Changed 16 months ago by karsten

Status: reopenedneeds_review

comment:35 Changed 16 months ago by iwakeh

Ready for merge, if you confirm that version status cannot be null in the line added to DetailsDocumentWriter.

comment:36 in reply to:  35 ; Changed 16 months ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Replying to iwakeh:

Ready for merge, if you confirm that version status cannot be null in the line added to DetailsDocumentWriter.

I had that same thought after seeing both changes in that commit (which I found and fixed separately). Actually the version status there can be null, but that's okay, because we're not invoking a method on it. Alright, merging to master and re-closing. Thanks!

comment:37 in reply to:  36 ; Changed 16 months ago by iwakeh

Replying to karsten:

Replying to iwakeh:

Ready for merge, if you confirm that version status cannot be null in the line added to DetailsDocumentWriter.

I had that same thought after seeing both changes in that commit (which I found and fixed separately). Actually the version status there can be null, but that's okay, because we're not invoking a method on it. Alright, merging to master and re-closing. Thanks!

I'm not concerned about NPEs, but the string "null" appearing somewhere, where it shouldn't be.

comment:38 in reply to:  37 Changed 16 months ago by karsten

Replying to iwakeh:

I'm not concerned about NPEs, but the string "null" appearing somewhere, where it shouldn't be.

Ah, Gson simply leaves out fields with null values, unless told otherwise.

Note: See TracTickets for help on using tickets.