Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VC: Fix timecheck log statements and DNS resolve cache issue #5675 #5846

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

cheatfate
Copy link
Contributor

No description provided.

@cheatfate cheatfate changed the title Address issues #5675 and #5681. VC: Fix timecheck log statements and DNS resolve cache issue #5675 Feb 3, 2024
Copy link

github-actions bot commented Feb 3, 2024

Unit Test Results

         9 files  ±0    1 104 suites  ±0   27m 43s ⏱️ +18s
  4 229 tests ±0    3 882 ✔️ ±0  347 💤 ±0  0 ±0 
16 882 runs  ±0  16 484 ✔️ ±0  398 💤 ±0  0 ±0 

Results for commit c331182. ± Comparison against base commit 3ac0432.

♻️ This comment has been updated with latest results.

@cheatfate cheatfate requested a review from tersec February 4, 2024 00:49
Copy link
Contributor

@tersec tersec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On balance, sure, this seems like an improvement -- people concerned about blocking DNS lookups (which as far as I know still is the case?) can either use IP addresses or some /etc/hosts-equivalent to effectively guarantee that won't become a practical issue, and for one-BN configurations, invoking this recovery codepath essentially implies small latencies won't matter in that moment anyway, if doppelganger protection's going to be active.

Also, reasonably often, at least personally/anecdotally, when I notice "DNS" failing, it's just a canary for Internet connectivity failing as a whole, so even skipping it doesn't help.

One concern I'd have would be, does this risk in multiple-BN configurations where the DNS isn't so dynamic as in a Kubernetes container or similar configuration, so repeatedly lookups wouldn't have been expected to change results, and there's for some reason a plausibility of DNS resolution timeouts long enough to disrupt other, hitherto-working VC/BN pairings by timing out on DNS lookup for an incorrect pairing. Right now, that doesn't happen, but of course, it also never figures out any updated IP addresses, either.

But even as-is, I'd argue using PR reflects a better risk balance than not using this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants