8000 Add opt-in device auto cleanup to config entries by bdraco · Pull Request #66209 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add opt-in device auto cleanup to config entries #66209 8000

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Add opt-in device auto cleanup to config entries #66209

wants to merge 6 commits into from

Conversation

bdraco
Copy link
Member
@bdraco bdraco commented Feb 9, 2022 •
edited
Loading

Proposed change

Config entries can now call entry.async_enable_device_auto_cleanup()

This call controls how the system considers a device orphaned
when considering to cleanup devices.

Orphaned devices are defined by not have any entities
referenced and no config entries with auto cleanup
disabled (default).

Implements
https://community.home-assistant.io/t/delete-individual-device-and-entity-from-integration/157428

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@bdraco
Add opt-in device auto cleanup to config entries
6521c20 
Config entries can now call entry.async_enable_device_auto_cleanup()

This call controls how the system considers a device orphaned
when considering to cleanup devices.

Orphened devices are defined by not have any entities
referenced and no config entries with auto cleanup
disabled (default).
@bdraco bdraco mentioned this pull request Feb 9, 2022
Merged
22 tasks
@MartinHjelmare MartinHjelmare mentioned this pull request Feb 9, 2022
Merged
22 tasks
@MartinHjelmare
Copy link
Member

Related PR: #66188

@bdraco bdraco marked this pull request as ready for review February 9, 2022 20:20
@bdraco bdraco requested a review from a team as a code owner February 9, 2022 20:20
@bdraco
Copy link
Member Author
bdraco commented Feb 9, 2022

Testing

diff --git a/homeassistant/components/august/__init__.py b/homeassistant/components/august/__init__.py
index 9b340096fd..51cc4dec43 100644
--- a/homeassistant/components/august/__init__.py
+++ b/homeassistant/components/august/__init__.py
@@ -36,6 +36,8 @@ API_CACHED_ATTRS = (
 async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
     """Set up August from a config entry."""
 
+    entry.async_enable_device_auto_cleanup()
+
     august_gateway = AugustGateway(hass)
 
     try:
diff --git a/homeassistant/components/unifiprotect/__init__.py b/homeassistant/components/unifiprotect/__init__.py
index 97fdc6eac2..d42c7d2490 100644
--- a/homeassistant/components/unifiprotect/__init__.py
+++ b/homeassistant/components/unifiprotect/__init__.py
@@ -43,6 +43,7 @@ SCAN_INTERVAL = timedelta(seconds=DEFAULT_SCAN_INTERVAL)
 
 async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
     """Set up the UniFi Protect config entries."""
+    entry.async_enable_device_auto_cleanup()
 
     async_start_discovery(hass)
     session = async_create_clientsession(hass, cookie_jar=CookieJar(unsafe=True))

Restarted, all the devices with no entities on august and unifi protect that have been hanging for years are gone 👍

@balloob
Copy link
Member
balloob commented Feb 10, 2022

In this case, shouldn't August and Unifi have cleaned up the devices when they removed the entities? Any device without config entries should be automatically removed.

I commented something similar at #66188 (comment)

  • If a config entry removes itself from a device, we should automatically remove all it's entities tied to that device.

  • Allow deleting a device in the UI if all entities related to it are marked as "restored" (so not provided by an entity)

The challenge we have for things like Sonos, Wemo and Cast is that they don't know if a device is temporarily disconnected or gone. It might just be in a closet waiting to control the Christmas tree in December.

@bdraco
Copy link
Member Author
bdraco commented Feb 10, 2022
  • If a config entry removes itself from a device, we should automatically remove all it's entities tied to that device.

August doesn't know if the unavailable lock is coming back or not, Unifi Protect doesn't know if the camera is temporary or permanently offline either.

The entities are being deleted in the UI after the camera/lock was replaced but the device entry was left orphaned but since it still pointed to the config entry that set it up, it was not removed.

@bdraco
Copy link
Member Author
bdraco commented Feb 10, 2022

I guess its pretty much the same case as Sonos, Wemo and Cast

@balloob
Copy link
Member
balloob commented Feb 10, 2022

Ah yes, in that case it's all the same.

What do you think of the idea of a user removing a config entry from a device if all it's entities are restored? That way a user can clean it up without an integration having to flag it.

@amelchio
Copy link
Contributor

@balloob I think that would be great. It seems to be what everyone has been asking for and couldn't understand why it was being rejected.

However, it must also cover the case where the user has first deleted all the restored entities so none are left.

(BTW, adding a core flag like this PR does is much preferable to implementing the cleanup in each integration but if the flag is opt-in it will still take a long time before all integrations are updated. And someone will keep missing the flag in new integrations.)

@emontnemery
Copy link
Contributor

Devices can have things that are not entities tied to them, e.g. device triggers, tags, etc.
We should check for that too before classifying a device as orphaned, then it will be easier to opt-in.

@bdraco
Copy link
Member Author
bdraco commented Feb 10, 2022

device triggers, tags, etc.

That makes it a bit more complex since we can't really tell if a device is in use or not.

For lutron caseta we register the pico remotes as devices but they have no linked entities. As I replace remotes I end up with extra devices lying around. Thinking about the removal implementation, I'd check if there had been a call to async_get_or_create and then remove the ones that didn't get recreated.

In this case maybe we could only remove devices for everything that wasn't freshened with async_get_or_create after the config entry state changes to LOADED?

@bdraco bdraco mentioned this pull request Feb 20, 2022
Closed
@bdraco
Copy link
Member Author
bdraco commented Feb 22, 2022

We should be able to keep track of which config entries have touched the device recently here AKA fresh state

I'm not sure this is race safe though as we would have to only do it for LOADED config entries but the device may be created later since we set LOADED before all platforms have finished loading : https://github.com/home-assistant/core/blob/dev/homeassistant/config_entries.py#L408

@bdraco bdraco marked this pull request as draft February 22, 2022 22:15
@bdraco
Copy link
Member Author
bdraco commented May 5, 2022

We might be able to add some type of done callback to async_setup_platforms to run a cleanup once it’s done if they flip it on

@bdraco
Copy link
Member Author
bdraco commented May 5, 2022

hass.config_entries.async_setup_platforms(config_entry, PLATFORMS, cleanup_unused_devices=True)

@bdraco
Copy link
Member Author
bdraco commented Jun 9, 2022

Let's close this since we have async_remove_config_entry_device now.

@bdraco bdraco closed this Jun 9, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla-signed core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bdraco @MartinHjelmare @balloob @amelchio @emontnemery @homeassistant
0