Check config before restarting by andrey-git · Pull Request #5609 · home-assistant/core

andrey-git · 2017-01-28T20:42:15Z

Description:

Check config by running --script check_config before restarting. Cancel restart on failure.
If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

mention-bot · 2017-01-28T20:42:17Z

@andrey-git, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @kellerza and @pvizeli to be potential reviewers.

kellerza · 2017-01-28T20:52:12Z

homeassistant/scripts/check_config.py

        res['secret_cache'] = dict(yaml.__SECRET_CACHE)
    except Exception as err:  # pylint: disable=broad-except
        print(color('red', 'Fatal error while loading config:'), str(err))
+        res['except_global'].append(err)


There will always only be one of these, so instead of adding a new key, simply add it to "General Errors" res['except'].setdefault(ERROR_STR, []).append(err)

kellerza · 2017-01-28T20:53:41Z

homeassistant/core.py

    @callback
    def _async_restart_handler(self, *args):
        """Restart Home Assistant."""
+        if subprocess.call([sys.argv[0], '--script', 'check_config']):


Is this not a blocking call? Should be scheduled on the event loop

…into restart

pvizeli · 2017-01-28T22:44:47Z

I'm not sure if that the right solution, but the core is async and need https://docs.python.org/3.4/library/asyncio-subprocess.html

balloob · 2017-01-28T23:12:14Z

homeassistant/core.py

    def _async_restart_handler(self, *args):
        """Restart Home Assistant."""
+        if subprocess.call([sys.argv[0], '--script', 'check_config']):
+            _LOGGER.error("check_config failed. Not restarting.")


You will need to create a persistent notification to notify the user or they won't know.

This is a great idea, with the same notification name, you can do a "checking config" message and change it into a "failed" config message if it fails.

Checking does take a while on slower rPi's!

Added notification and switched to async

kellerza · 2017-01-29T08:36:25Z

One of my ideas is still to rewrite the config loader into a seperate function. The current check_config script is a monumental hack (was a good learning experience).

The idea would be to have one function to provide a validated config, after voluptuous with all the correct line numbers references, include packages and stretch goal maybe load_order (although this migh not make sense)

This function can then be the input for bootstrap, check_config or such validation requirements as we have here.

The current way we do this is deeply ingrained into bootstrap and suffers from incorrect yaml references (log_exception shortcomings:, voluptuous strips line info (so needs to be re-added), named component keys "switch named1:" )

For a start no changes required to bootstrap, but in the long run I belive we can greatly simply bootstrap if we can trust the config coming in

Would this be worth the effort?

…into restart

andrey-git · 2017-01-29T19:26:59Z

Added notification and switched to async

kellerza · 2017-01-29T19:54:53Z

homeassistant/core.py

+        yield from self._notify('Config validated')
+        yield from self.async_stop()
+
    @callback


Cant you simplify this (get rid of _async_check_config_and_restart function), by changing this @callback into a @asyncio.coroutine?

Tried that. add_signal_handler refuses to accept a coroutine.

kellerza · 2017-01-29T19:55:07Z

homeassistant/core.py

        """Restart Home Assistant."""
-        self.exit_code = RESTART_EXIT_CODE
-        self.loop.create_task(self.async_stop())
+        if self.services.has_service('persistent_notification', 'create'):


This is always setup

kellerza · 2017-01-29T19:56:18Z

homeassistant/core.py

+            print(content)
+            # Put error cleaned from color codes in the error log so it
+            # will be visible at the UI.
+            _LOGGER.error(re.sub(r'\033\[[^m]*m', '', content))


Won't this error now be duplicated in stdout?

Yes, but I think this is the best outcome, as stdout will have the colored printout which is easiest to read and the UI will have the error log.

We should only log it with _LOGGER.error and not with print.

Agreed, the print shows the colour, but in the context of a server, you rarely see stdout. It's mostly helpful in a dev environment.

homeassistant · 2017-01-30T00:52:59Z

Hi @andrey-git,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

pvizeli

I don't like the idea to call self inside self or using subprocess stuff inside core. It's like a hack.

I think @kellerza have right to move the validate layer into next level and split it from setup. I think that is the harder way but not a hack like the check_config (sorry @kellerza)

pvizeli · 2017-01-30T22:24:09Z

homeassistant/core.py

+            'check_config',
+            stdout=asyncio.subprocess.PIPE)
+        # Wait for the subprocess exit
+        result = yield from proc.wait()


you should use communicate to protect from subprocess deadlock.

andrey-git · 2017-01-31T07:05:23Z

@pvizeli I agree that the solution @kellerza proposed is much better.
However that would be a large change that I'm not comfortable making.

This change still brings value to users.

andrey-git · 2017-02-02T06:49:05Z

@kellerza , @pvizeli Is this ready to merge?

balloob · 2017-02-02T07:22:47Z

@pvizeli the non-hack way is obviously better, but that is for the future. For now I think that this is a fine intermediate solution.

(and since all the hacky stuff is hidden in a command line script, it's fine)

balloob · 2017-02-02T20:46:47Z

homeassistant/core.py

        self.loop.create_task(self.async_stop())

+    @asyncio.coroutine
+    def _notify(self, message):


There is no need for this function. Every component already exports functions for their services: https://github.com/andrey-git/home-assistant/blob/7b5b1fb7511bbc42c943d27eb0563a1e17ac4d24/homeassistant/components/persistent_notification.py#L47-L58

I think core shouldn't depend on a component. Even a "core" components such as persistent notification.

balloob · 2017-02-02T20:47:36Z

homeassistant/core.py

        """Restart Home Assistant."""
-        self.exit_code = RESTART_EXIT_CODE
-        self.loop.create_task(self.async_stop())
+        self.loop.create_task(self._notify('Checking config...'))


Please remove this notification. We should only report on result.

balloob · 2017-02-02T20:48:21Z

homeassistant/core.py

+            # Put error cleaned from color codes in the error log so it
+            # will be visible at the UI.
+            _LOGGER.error(re.sub(r'\033\[[^m]*m', '', content))
+            yield from self._notify('Restart failed')


Please remove this function and inline the calls inside this method. Also use notification_id so that you will overwrite previous restart hass notifications instead of crowding the UI.

The _notification_id is captured in the _notify methods, but also likethe idea more that it rather calls the complete method

This code makes two notification (restarting, and error). Why duplicate code for the title and the id?

Oh, I see that you want to remove the "checking config". In this case the inline is indeed in order.

You'll need to move the import of persistent notification also in this method, or else we'll get circular imports.

Don't you think depending here on component/ is bad?
I think core should just call the service, so it will "work" even if the persistent_notification component is missing.

balloob · 2017-02-02T20:48:49Z

homeassistant/core.py

+            yield from self._notify('Restart failed')
+            return
+        self.exit_code = RESTART_EXIT_CODE
+        # This notification is unlikely to arrive before HA is stopped.


Indeed unlikely, let's remove it.

balloob · 2017-02-02T20:54:20Z

Since it's core functionality, we will need some tests. You can stub out the call to asyncio.subprocess to simulate getting a certain result and see if a restart would go through.

…into restart

andrey-git · 2017-02-03T13:26:14Z

Tests added, comments addressed, except for importing persistent_notification in core which I think shouldn't happen.

pvizeli · 2017-02-03T17:50:31Z

Other question. Why not move to components/__init__ and create a service for check config. So we have no hack inside core @balloob. @andrey-git your code is good that is not my point.

danielperna84 · 2017-02-04T13:10:25Z

@pvizeli
Good point. Although I'm biased because I would need that for my configurator.

pvizeli · 2017-02-06T06:19:36Z

Hack is the wrong word, I don't want to disrate that. I think the real workflow is not solve with that. After I upload a congig I will check that with script and it would be nice if I can use it with a service. I never restart my hass after every config update. I think a user should have this possibility and if a user don't use it... The other is that the check config overwritte the logs and make the restart complexer as it needed and make it more trouble maker.

pvizeli · 2017-02-06T06:22:16Z

And in the future we have a online config editor and that need it as service or we need to restart everthing after save :)

balloob · 2017-02-07T09:16:46Z

tests/test_core.py

+        self.hass.start()
+        with patch.object(self.hass, 'async_stop') as mock_stop:
+            self.hass.services.call(ha.DOMAIN, SERVICE_HOMEASSISTANT_RESTART)
+            mock_stop.assert_not_called()


Tip for the future: don't use assert_not_called because if you would make a typo like calling mock_stop.assert_not_called_obviously_wrong() it will pass because it will return a mock object. Better to test explicitly:

assert mock_stop.called

balloob · 2017-02-07T09:18:01Z

So I agree with @pvizeli that calling a subprocess etc should not be handled in the core but I think that moving our start/stop/check to components/__init__.py is out of scope for this PR.

So let's merge this and then put on our to do to move all three services.

Check config before restarting.

d613310

balloobbot added the core label Jan 28, 2017

kellerza requested changes Jan 28, 2017

View reviewed changes

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

e2cefa0

…into restart

balloob reviewed Jan 28, 2017

View reviewed changes

fabaff changed the title ~~Check config before restarting.~~ Check config before restarting Jan 29, 2017

andrey-git added 2 commits January 29, 2017 18:47

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

9e7b45d

…into restart

Make check_config on restart async

0755787

kellerza reviewed Jan 29, 2017

View reviewed changes

don't check if notification service exists

30338e0

robbiet480 added the bump-cla-temp label Jan 30, 2017

homeassistant added cla-needed labels Jan 30, 2017

robbiet480 removed the bump-cla-temp label Jan 30, 2017

homeassistant added cla-signed and removed cla-needed labels Jan 30, 2017

robbiet480 added the cla-recheck label Jan 30, 2017

homeassistant added cla-signed and removed cla-recheck labels Jan 30, 2017

pvizeli reviewed Jan 30, 2017

View reviewed changes

Use .communicate()

7b5b1fb

homeassistant added the cla-signed label Jan 31, 2017

kellerza approved these changes Feb 2, 2017

View reviewed changes

balloob reviewed Feb 2, 2017

View reviewed changes

Merge branch 'dev' of https://github.com/home-assistant/home-assistant …

4151fcd

…into restart

homeassistant added the cla-signed label Feb 3, 2017

Reduce the number of notifications. Add tests.

c270695

balloob reviewed Feb 7, 2017

View reviewed changes

balloob merged commit f774538 into home-assistant:dev Feb 7, 2017

pvizeli mentioned this pull request Feb 7, 2017

Move core service from core to components #5787

Merged

1 task

home-assistant locked and limited conversation to collaborators May 19, 2017

andrey-git deleted the restart branch May 10, 2018 18:14

Uh oh!

Conversation

andrey-git commented Jan 28, 2017

Uh oh!

mention-bot commented Jan 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvizeli commented Jan 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kellerza commented Jan 29, 2017

Uh oh!

andrey-git commented Jan 29, 2017

Uh oh!

kellerza Jan 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

homeassistant commented Jan 30, 2017

Uh oh!

pvizeli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrey-git commented Jan 31, 2017

Uh oh!

andrey-git commented Feb 2, 2017

Uh oh!

balloob commented Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kellerza Jan 29, 2017 •

edited

Loading

balloob Feb 2, 2017 •

edited

Loading

balloob commented Feb 2, 2017 •

edited

Loading