Use runtime_data and validate connection at setup for dnsip#169745
Conversation
|
Hey there @gjohansson-ST, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
Refactors the dnsip integration to create and validate aiodns.DNSResolver instances once during config entry setup, store them in entry.runtime_data, and re-use them from the sensors (with accompanying test updates).
Changes:
- Add
DnsIPRuntimeData/DnsIPConfigEntryand construct IPv4/IPv6 resolvers duringasync_setup_entry, including a setup-time DNS validation query and cleanup on unload. - Update
WanIpSensorto consume resolvers fromentry.runtime_datarather than creating/closing its own resolver. - Adjust and extend tests to patch resolver construction in
homeassistant.components.dnsipand cover setup-time DNS failure + IPv6-only setup.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| homeassistant/components/dnsip/init.py | Creates resolvers at config-entry setup, performs a setup-time DNS probe, stores runtime_data, and closes resolvers on unload. |
| homeassistant/components/dnsip/sensor.py | Switches sensors to use entry.runtime_data resolvers and removes per-entity resolver lifecycle handling. |
| tests/components/dnsip/test_sensor.py | Updates patch targets and removes now-redundant resolver re-patching during periodic updates. |
| tests/components/dnsip/test_init.py | Updates patch targets and adds tests for setup-time DNS error handling and IPv6-only setup. |
gjohansson-ST
left a comment
There was a problem hiding this comment.
Take a look at the copilot comments too as they seem valid
|
Applied all the requested changes, copilot too.
Added two tests for the new branches ( Will wait and see if any further corrections needed including copilots suggestions |
| def create_dns_resolver(self) -> None: | ||
| """Create the DNS resolver.""" |
There was a problem hiding this comment.
The recreated resolver isn't closed on unload so small leak. But, keeping the recreate path because @gjohansson-ST asked for it earlier. Happy to drop it if you'd prefer no leak over no recreate.
There was a problem hiding this comment.
We could use this
or we simply skip self.resolver and use the ones from runtime_data directly
|
Please resolve the comments that has been addressed and if you don't agree with copilot's comments (or mine) you need to respond to those |
| errors = [ | ||
| result for result in results if isinstance(result, (TimeoutError, DNSError)) | ||
| ] | ||
| if errors and len(errors) == len(results): |
There was a problem hiding this comment.
| if errors and len(errors) == len(results): | |
| if errors and len(errors) == 2: |
Let's be specific as we know it's 2
| try: | ||
| await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS) | ||
| except Exception: | ||
| await _close_resolvers() | ||
| raise |
There was a problem hiding this comment.
What could possibly raise here? And we should not do this anyway.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| try: | ||
| async with asyncio.timeout(10): | ||
| results = await asyncio.gather( | ||
| resolver_ipv4.query(hostname, "A"), | ||
| resolver_ipv6.query(hostname, "AAAA"), | ||
| return_exceptions=True, | ||
| ) | ||
| except TimeoutError as err: | ||
| await resolver_ipv4.close() | ||
| await resolver_ipv6.close() | ||
| raise ConfigEntryNotReady( | ||
| f"DNS lookup timed out for {hostname}: {err}" | ||
| ) from err | ||
|
|
||
| errors = [ | ||
| result for result in results if isinstance(result, (TimeoutError, DNSError)) | ||
| ] | ||
| if errors and len(errors) == 2: | ||
| await resolver_ipv4.close() | ||
| await resolver_ipv6.close() | ||
| raise ConfigEntryNotReady( | ||
| f"DNS lookup failed for {hostname}: {errors[0]}" | ||
| ) from errors[0] |
There was a problem hiding this comment.
Tried that earlier in this PR "let's be specific as we know it's 2" suggestion, so we ended up with always-create-both. Going with the codeowner on this one.
| hostname = entry.data[CONF_HOSTNAME] | ||
|
|
||
| resolver_ipv4 = aiodns.DNSResolver( | ||
| nameservers=[entry.options[CONF_RESOLVER]], | ||
| tcp_port=entry.options[CONF_PORT], | ||
| udp_port=entry.options[CONF_PORT], | ||
| ) | ||
| resolver_ipv6 = aiodns.DNSResolver( | ||
| nameservers=[entry.options[CONF_RESOLVER_IPV6]], | ||
| tcp_port=entry.options[CONF_PORT_IPV6], | ||
| udp_port=entry.options[CONF_PORT_IPV6], | ||
| ) |
There was a problem hiding this comment.
We had this for a round and reverted after review. Sticking with the always-both pattern you asked for.
| async def test_setup_ipv6_only(hass: HomeAssistant) -> None: | ||
| """Test setup with only IPv6 enabled exercises the IPv6 lookup branch.""" | ||
|
|
||
| entry = MockConfigEntry( | ||
| domain=DOMAIN, | ||
| source=SOURCE_USER, | ||
| data={ | ||
| CONF_HOSTNAME: "home-assistant.io", | ||
| CONF_NAME: "home-assistant.io", | ||
| CONF_IPV4: False, | ||
| CONF_IPV6: True, | ||
| }, | ||
| options={ | ||
| CONF_RESOLVER: "208.67.222.222", | ||
| CONF_RESOLVER_IPV6: "2620:119:53::53", | ||
| CONF_PORT: 53, | ||
| CONF_PORT_IPV6: 53, | ||
| }, | ||
| entry_id="1", | ||
| unique_id="home-assistant.io", | ||
| ) | ||
| entry.add_to_hass(hass) | ||
|
|
||
| with patch( | ||
| "homeassistant.components.dnsip.aiodns.DNSResolver", | ||
| side_effect=[RetrieveDNS(), RetrieveDNS()], | ||
| ): | ||
| await hass.config_entries.async_setup(entry.entry_id) | ||
| await hass.async_block_till_done() | ||
|
|
||
| assert entry.state is ConfigEntryState.LOADED | ||
|
|
There was a problem hiding this comment.
Fair point about the docstring — it's a bit stale after the revert. Test still verifies the IPv6-only entry loads, just isn't exercising a setup branch anymore
gjohansson-ST
left a comment
There was a problem hiding this comment.
Also look at #170048 as it has impact
| errors = [ | ||
| result | ||
| for result in results | ||
| if isinstance( | ||
| result, (TimeoutError, DNSError, AresError, asyncio.CancelledError) | ||
| ) |
There was a problem hiding this comment.
Not implementing. Suggestion that introduced this filter explicitly included asyncio.CancelledError. Background context of PR #170048, where broader handling of pycares/aiodns leaks treats CancelledError as a leakable exception rather than a true cancellation.
gjohansson-ST
left a comment
There was a problem hiding this comment.
Some very few minor points, looks good otherwise from my side.
Breaking change
Proposed change
Split out from #169688 per @gjohansson-ST's request.
Refactors
dnsipso that DNS resolvers are created once at config-entry setup rather than per-entity:async_setup_entryconstructs both IPv4 and IPv6aiodns.DNSResolverinstances, runs a single test query against the configured hostname, and raisesConfigEntryNotReadyonTimeoutError/DNSError.entry.runtime_datavia aDnsIPRuntimeDatadataclass and aDnsIPConfigEntry = ConfigEntry[DnsIPRuntimeData]type alias.WanIpSensorconsumes the resolver fromruntime_datainstead of constructing its own.homeassistant.components.dnsip.aiodns.DNSResolver(since DNSResolver construction now happens in__init__.py); redundant patches in periodic-update sections removed; new tests added for the setup-time DNS error path and the IPv6-only setup branch.Required so #169688 can claim
runtime-data: doneandtest-before-setup: donein the quality scale.Type of change
Additional information
This PR is related to issue: Add bronze quality scale to DNS IP #169688
This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:
Link to developer documentation pull request:
Link to frontend pull request:
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: