Opened 6 weeks ago

Closed 4 weeks ago

#31657 closed defect (implemented)

Rephrase "missing descriptors" notice log to be less confusing

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version: Tor: 0.3.3.1-alpha
Severity: Normal Keywords: consider-backport-after-0421, fast-fix, log, tor-guard, tor-bridge, tor-client, BugSmashFund, 042-should 041-backport? 040-backport? 035-backport?
Cc: catalyst Actual Points: 0.1
Parent ID: #21969 Points: 0.1
Reviewer: ahf Sponsor:

Description

Some operators are confused or alarmed by these logs:

Sep 05 03:22:50.000 [notice] Our directory information is no longer up-to-date enough to build circuits: We're missing descriptors for 1/3 of our primary entry guards (total microdescriptors: 6515/6541).
Sep 05 03:22:50.000 [notice] I learned some more directory information, but not enough to build a circuit: We're missing descriptors for 1/3 of our primary entry guards (total microdescriptors: 6515/6541).

We should rephrase them or document that:

  • tor tries to keep active 3 primary guards for anonymity and safety
  • we'll try to get new microdescs soon
  • tor usually recovers quickly from this issue

Child Tickets

TicketStatusOwnerSummaryComponent
#31644closedNumPrimaryGuards is set to 6, but the log file keep reports that is missing descriptors for 1/3 of our primary entry guardsCore Tor/Tor

Change History (14)

comment:1 Changed 6 weeks ago by teor

  • It's normal for microdescs to expire once a week, so if we have 3 primary guards, we will see an expiry every few days

comment:2 Changed 6 weeks ago by teor

Actual Points: 0.1
Keywords: BugSmash added; doc? removed
Status: assignedneeds_review
Version: Tor: 0.3.0.6Tor: 0.3.3.1-alpha

See my pull request:

I added a short explanation to the existing log message, and updated the test that depends on that message.

The merge forward was clean.
Here are the test branches for merging forwards:

comment:3 Changed 5 weeks ago by teor

Keywords: BugSmashFund added; BugSmash removed

Fix bug smash fund spelling

comment:4 Changed 5 weeks ago by dgoulet

Reviewer: asn

comment:5 Changed 5 weeks ago by asn

Status: needs_reviewneeds_revision

I'm OK with the change, but if the log message should not be acted upon, why should we expose it to the users as a [notice]?

Is it so that if we ever get issues like (#30746/#21969) again we know that this is the underlying issue? In this case, could we just output this message just in pathological cases?

I'm marking this as needs_revision to hear teor's opinion.

comment:6 Changed 5 weeks ago by teor

How do we reliably detect pathological cases?
Suppress the message, until it occurs a few times within a short timeframe?

comment:7 Changed 5 weeks ago by nickm

Keywords: 042-should added

Mark some needs_revision tickets as 042-should

comment:8 in reply to:  6 Changed 5 weeks ago by asn

Replying to teor:

How do we reliably detect pathological cases?
Suppress the message, until it occurs a few times within a short timeframe?

That's a good question I don't have an answer for.

Unfortunately, I don't think that's the kind of message that occurs multiple times, looking at #30746 (and friends) this seems to be able to cause havoc with just a single repeatition.

I'm not sure why this is the case, since router_have_minimum_dir_info() seems to be called all the time and that should eventually call entry_guards_get_err_str_if_dir_info_missing() which is the source of the log message... Things are kinda messy between these two functions tho, so it's kinda hard to understand what's the issue.

Perhaps we can merge this patch for now since it does not seem to make the situation worse, and we can think in the future how to improve the UX? If you agree, feel free to toggle this into merge_ready since the patch LGTM.

comment:9 Changed 5 weeks ago by teor

Status: needs_revisionmerge_ready

comment:10 Changed 5 weeks ago by teor

Let's deal with the UX / behaviour in #31707.

comment:11 Changed 5 weeks ago by nickm

Keywords: 041-backport? 040-backport? 035-backport? added
Milestone: Tor: 0.4.2.x-finalTor: 0.4.1.x-final

Merged to master; marking for possible backport, though I don't have strong feelings either way.

comment:12 Changed 5 weeks ago by teor

Keywords: consider-backport-after-0421 added

If a backport stops us getting one more bug report, I think the cost of the backport would be worthwhile.

comment:13 Changed 4 weeks ago by ahf

Reviewer: asnahf

comment:14 Changed 4 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

Backporting to 0.3.5 and forward.

Note: See TracTickets for help on using tickets.