Skip to content

fix(supervisor): reread resolv.conf for policy DNS upstreams - #5224

Open
ericcurtin wants to merge 5 commits into
NVIDIA:mainfrom
ericcurtin:fix/5197-policy-dns-resolv-reload/ericcurtin
Open

fix(supervisor): reread resolv.conf for policy DNS upstreams#5224
ericcurtin wants to merge 5 commits into
NVIDIA:mainfrom
ericcurtin:fix/5197-policy-dns-resolv-reload/ericcurtin

Conversation

@ericcurtin

@ericcurtin ericcurtin commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Policy DNS now rereads /etc/resolv.conf per query and falls through to the next nameserver on timeout or I/O error.

Related Issue

Closes #5197

Changes

  • Reread nameservers per exchange, keep the last good list on a bad read
  • Try each nameserver in order on every query, primary first
  • Log the upstream list at start and on change
  • Drop the duplicate resolv.conf parser in runtime.rs

Testing

  • Checks appropriate to the affected code and behavior pass
  • Unit tests added/updated (if applicable)
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

@ericcurtin
fix(supervisor): reread resolv.conf for policy DNS upstreams
dd943e7 
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
@copy-pr-bot

copy-pr-bot Bot commented Nov 3, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@TaylorMutch

TaylorMutch commented Nov 3, 2026

Copy link
Copy Markdown
Collaborator

/ok to test dd943e7

TaylorMutch
TaylorMutch reviewed Nov 3, 2026
View reviewed changes

@TaylorMutch TaylorMutch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I found one resolver-selection concern. Validation used a standalone Rust reproduction of the PR's selection method; full crate tests and sandbox E2E were not run.

Comment thread crates/openshell-supervisor-network/src/policy_dns/resolver.rs Resolved
@ericcurtin
fix(supervisor): reset DNS upstream preference on reload
1b62ba2 
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
@ericcurtin

ericcurtin commented Nov 3, 2026

Copy link
Copy Markdown
Contributor Author

@TaylorMutch @shailendra-nv The resolver preference now resets on config change and expires so the primary is retried (1b62ba2). PTAL when you get a chance, and /ok to test 1b62ba2ec5ed1e7d6cbd823ea106641acab6c7c4 if it looks good. Thank you!

@ericcurtin
refactor(supervisor): share policy DNS resolver setup
812f16a 
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
@ericcurtin

ericcurtin commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Extracted the duplicated resolver setup into one helper (812f16a); clippy and policy_dns tests pass. @TaylorMutch @shailendra-nv PTAL when you get a chance, and /ok to test 812f16a5148ed2e7919701437f4111aa0e3ab960 if it looks good. Thank you!

@ericcurtin
fix(supervisor): always try the primary DNS upstream first
9a1f175 
Signed-off-by: Eric Curtin <eric.curtin@docker.com>
@ericcurtin

ericcurtin commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

@TaylorMutch @johntmyers PTAL, the primary is now always tried first. Could one of you comment ok to test 9a1f175? Thank you!

@TaylorMutch

TaylorMutch commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator

/ok to test 9a1f175

@TaylorMutch

TaylorMutch commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator

@sideDesert, are you able to build OpenShell from source locally and test this PR against the macOS/GlobalProtect VPN scenario you reported in #5197?

If so, please check whether DNS lookups in the same running VM sandbox continue working after connecting or reconnecting the VPN, without restarting the sandbox or supervisor. Does this fix resolve the issue you observed? Please share the commit tested and the result. If building locally isn't practical, let us know.

@TaylorMutch TaylorMutch mentioned this pull request Nov 10, 2026
Open
@ericcurtin
Merge remote-tracking branch 'origin/main' into pr5224-merge
29ce881 
Signed-off-by: Eric Curtin <eric.curtin@docker.com>

# Conflicts:
#	crates/openshell-supervisor-network/src/policy_dns/resolver.rs
@ericcurtin

ericcurtin commented Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Merged main, conflict resolved. PTAL.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TaylorMutch TaylorMutch TaylorMutch left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Policy DNS keeps the host's first nameserver from supervisor start, so all sandbox lookups fail after the host's DNS changes (e.g. VPN connect)

2 participants

@ericcurtin @TaylorMutch