Improve error handling in backup restore #4791
Conversation
mdegat01
added
the
refactor
| if attr in data: | ||
| self._data[attr] = data[attr] |
There was a problem hiding this comment.
I'm aware this change feels unrelated to the PR. However after the changes this failed tests. Many of these attributes are vol.Maybe in the schema so they may not exist (and refresh_token in particular did not exist in our mock backups). This is an example of an error we were hiding. Nothing was raised and all we did was log an error so the test appeared to succeed, now it needs to be fixed.
| raise BackupInvalidError( | ||
| f"Backup was made on supervisor version {backup.supervisor_version}, " | ||
| f"can't restore on {self.sys_supervisor.version}. Must update supervisor first.", | ||
| _LOGGER.error, |
There was a problem hiding this comment.
What is the concern exactly to disallow restoring backup? Can we maybe have a backup version number instead, and rely on that?
E.g. it would be nice if people can restore a backup taken on the beta channel on the stable channel (which potentially has an older Supervisor). This check currently prevents this. From what I understand this was possible before #3769.
There was a problem hiding this comment.
There is no way to rollback a supervisor update. If a user goes from beta channel to stable channel, supervisor remains on the beta channel version until there is a version on the stable channel which is newer then itself. So I'm not entirely sure how this situation is occurring, other then users directly manipulating things with the docker CLI.
The reason though is that we don't attempt to maintain backwards compatibility with internal things. For example:
- We may rewrite the schema of saved config files in a newer supervisor version. The upgrade must maintain backwards compatibility here, that doesn't mean its reversible though.
- We may enhance/change the schema addons use for their config. A user may have installed or updated to a version of an addon that makes use of these new features. Older versions of supervisor will then fail processing these addons.
- We may have changed the schema/structure of backups. It may not even be possible to successfully restore that backup on prior versions of supervisor.
Supervisor isn't designed to go backwards, only forwards. If you do roll back supervisor somehow and try and keep a config/setup you used successfully on that newer version of supervisor we really don't know what will and won't work, its untested space. That's why there is no API for rolling back supervisor.
There was a problem hiding this comment.
There is no way to rollback a supervisor update. If a user goes from beta channel to stable channel, supervisor remains on the beta channel version until there is a version on the stable channel which is newer then itself. So I'm not entirely sure how this situation is occurring, other then users directly manipulating things with the docker CLI.
The issue is occurring when taking a backup from an installation on the beta channel and then on the same system start over with a new installation (which starts in stable mode...).
We may rewrite the schema of saved config files in a newer supervisor version. The upgrade must maintain backwards compatibility here, that doesn't mean its reversible though.
At the very least we need to maintain the backward compatibility to read the Supervisor version number, otherwise this functionality would not work either. At that point, we could also maintain a schema version, no?
I understand that we don't want to maintain backwards compatibility in Supervisor, that is also not what I am asking for here.
I am only asking for a schema version to widen the compatibility of a certain backup. It is mainly meant as a convince feature for developers and testers. I myself get constantly into the annoying situation that I can't restore a test backup from one of my tests systems (which are typically on beta/dev channel) on a new installation (which is on stable channel) for no real reason 😢 . Sure, I can hack around, by just opening the backup and manually alter the version number of Supervisor. But why not just introduce a backup schema version? 🤔
Proposed change
When something goes wrong in restore users often have no idea since all we do is log a warning or error. When we do return an error all the UI says is "something went wrong, check the logs". This is particularly egregious during onboarding since the users can't even check supervisor logs.
This PR changes it so we raise an exception with what went wrong if the restore process did not begin due to invalid input or if restore was halted with an exception. If restore proceeded to the end but something went wrong along the way we always return
Falseto tell users to check the logs to see what worked and what didn't.The last part is not ideal but currently we have no other option. Restore can be partially successful, we don't have a good way to inform and display the details of what happened in the UI in this case. And if restore stopped Home Assistant then the UI can't actually display the response at all since it lost its connection to Supervisor. There are additional improvements to be made here that we'll follow up on in another PR.
Type of change
Additional information
Checklist
black --fast supervisor tests)If API endpoints of add-on configuration are added/changed: