Prevent ping integration from delaying startup #43869

bdraco · 2020-12-02T21:23:19Z

Breaking change

When restarting Home Assistant, the previous ping sensor state is now
restored and then updated in the background to allow startup to proceed
without the risk of timing out.

When the user has many ping sensors, the ping integration could
timeout starting up because each ping has to happen in the
executor.

Proposed change

Now that icmplib does not require root for newer kernels, we
can use it if the can_use_icmp_lib_with_privilege test passes
with privileged=False

The startup time problem with ping has been solved by
restoring the previous state instead of waiting for the ping to run
before starting.

NameLookupError is now handled to avoid the log filling up
when a host is offline.

device_tracker now uses multiping when possible to avoid
tying up the executor when there are many hosts.

ValentinBELYN/icmplib@v2.0.2...v2.1.1

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

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes ping offline hostname results in repeated log message "Error running command: ping -n -q -c 1 -W1 myhost.mynetwork.lan, return code: 2" #43785 and fixes Binary ping sensor unavailable after HA (re)start #43188
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:

Documentation added/updated for www.home-assistant.io

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.
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:

I have reviewed two other open pull requests in this repository.

ValentinBELYN · 2020-12-02T22:44:03Z

Hi @bdraco :)

To let the kernel handle some parts of the ICMP headers and thus perform the ping function without root privileges, you must set the privileged parameter to False.

You should also handle these exceptions:

NameLookupError
If you pass a hostname or FQDN in parameters and it does not exist or cannot be resolved.
SocketPermissionError
If the privileges are insufficient to create the socket.
(normally not necessary to handle this exception except for Windows, details below)
SocketAddressError
If the source address cannot be assigned to the socket.
(only if you define a source address, which is not currently the case)
ICMPSocketError
If another error occurs. See the ICMPv4Socket or ICMPv6Socket class for details.
(normally not necessary to handle this exception)

The complete documentation is up to date. You can find useful information about parameters, return value, and exceptions here: https://github.com/ValentinBELYN/icmplib#ping

Finally, the privileged parameter is ignored on Windows. Indeed, Windows is not compatible. However, icmplib can be used without administrator privileges on my Windows VM. It will be necessary to ensure that this is the case for other Home Assistant installations which are based on Windows before completely removing the fallback.

bdraco · 2020-12-02T23:04:33Z

We only support WSL for Windows (https://github.com/home-assistant/architecture/blob/master/adr/0016-home-assistant-core.md#supported-operating-systems-and-versions) so I think we can treat it the same as linux in this case.

I'll make the adjustments to handle the NameLookupError exception and set privileged=False using the original logic when we get SocketPermissionError so it should work regardless of the underly OS

bdraco · 2020-12-03T00:34:16Z

Thanks @ValentinBELYN. Your feedback was very helpful. I think I've addressed the issues.

bdraco · 2020-12-03T08:38:12Z

It looks like it won't work unless the OS enables it

# sysctl net.ipv4.ping_group_range
net.ipv4.ping_group_range = 1	0

ValentinBELYN · 2020-12-03T23:45:06Z

net.ipv4.ping_group_range is not needed with icmplib 2.0. When the privileged parameter is disabled, a DGRAM socket is used instead of a RAW socket. You can find more details about this mechanism here: https://lwn.net/Articles/420800/

Can you run the following lines to verify that the ping function can run without root privileges on your computer?

# Run these lines without root privileges
from icmplib import ping

ping('1.1.1.1', privileged=False)

If it doesn't work, can you give me the result you get by running these lines:

from icmplib.utils import platform, PLATFORM_LINUX, PLATFORM_MACOS, PLATFORM_WINDOWS

print(platform, PLATFORM_LINUX, PLATFORM_MACOS, PLATFORM_WINDOWS, sep='\n')

Thanks :)

bdraco · 2020-12-03T23:48:53Z

# Run these lines without root privileges
from icmplib import ping

ping('1.1.1.1', privileged=False)

root@ha-dev:/tmp# vim test1.py
root@ha-dev:/tmp# su nobody -s /bin/bash
nobody@ha-dev:/tmp$ python3 /tmp/test1.py
[False]
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/icmplib/sockets.py", line 89, in __init__
    self._sock = self._create_socket(
  File "/usr/local/lib/python3.8/dist-packages/icmplib/sockets.py", line 448, in _create_socket
    return socket.socket(
  File "/usr/lib/python3.8/socket.py", line 231, in __init__
    _socket.socket.__init__(self, family, type, proto, fileno)
PermissionError: [Errno 13] Permission denied

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/tmp/test1.py", line 4, in <module>
    ping('1.1.1.1', privileged=False)
  File "/usr/local/lib/python3.8/dist-packages/icmplib/ping.py", line 131, in ping
    sock = ICMPv4Socket(
  File "/usr/local/lib/python3.8/dist-packages/icmplib/sockets.py", line 103, in __init__
    raise ICMPSocketError(str(err))
icmplib.exceptions.ICMPSocketError: [Errno 13] Permission denied

bdraco · 2020-12-03T23:49:26Z

from icmplib.utils import platform, PLATFORM_LINUX, PLATFORM_MACOS, PLATFORM_WINDOWS

print(platform, PLATFORM_LINUX, PLATFORM_MACOS, PLATFORM_WINDOWS, sep='\n')

root@ha-dev:/tmp# vim test2.py
root@ha-dev:/tmp# su nobody -s /bin/bash
nobody@ha-dev:/tmp$ python3 test2.py
linux
True
False
False

bdraco · 2020-12-03T23:50:20Z

From the first test
socket(AF_INET, SOCK_DGRAM|SOCK_CLOEXEC, IPPROTO_ICMP) = -1 EACCES (Permission denied)

read(3, "U\r\r\n\0\0\0\0\242\226\v^\354S\0\0\343\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 16282) = 16281
read(3, "", 1)                          = 0
close(3)                                = 0
write(1, "[False]\n", 8[False]
)                = 8
socket(AF_INET, SOCK_DGRAM|SOCK_CLOEXEC, IPPROTO_ICMP) = -1 EACCES (Permission denied)
write(2, "Traceback (most recent call last"..., 35Traceback (most recent call last):
) = 35
write(2, "  File \"/usr/local/lib/python3.8"..., 89  File "/usr/local/lib/python3.8/dist-packages/icmplib/sockets.py", line 89, in __init__
) = 89

ValentinBELYN · 2020-12-03T23:57:14Z

What is your Linux distro + version?

ValentinBELYN · 2020-12-04T00:07:07Z

I will investigate tomorrow. Personally on macOS 10.15 and Ubuntu 20.04 I do not encounter this problem.

bdraco · 2020-12-04T00:22:52Z

What is your Linux distro + version?

Ubuntu 18.04.3 LTS (GNU/Linux 4.4.59+ x86_64)

davet2001 · 2020-12-05T10:40:26Z

I get exactly the same on raspbian 10/buster.

ValentinBELYN · 2020-12-12T21:16:05Z

Sorry for taking some time for testing. I confirm the behavior you are seeing and indeed, this is related to the net.ipv4.ping_group_range parameter. On Debian 10.6, Debian 11 (testing) and Ubuntu 18.04 LTS, the corresponding value is 1 0 which prevents all users from using datagram sockets for pinging.

On Ubuntu 20.04 LTS and later, the value is 0 2147483647, which is Ok.

I added explanations on the project page: https://github.com/ValentinBELYN/icmplib#how-to-use-the-library-without-root-privileges

Unfortunately, on operating systems that do not allow this feature by default and to use the library without root privileges, the following command is required:

$ sudo sysctl -w net.ipv4.ping_group_range='0 2147483647'

On another level, I released the version 2.0.1 to fix old bugs.

davet2001 · 2020-12-12T21:39:18Z

So, would it be possible to keep the drop back to the old method for the time being? Are there any other options?

rds76 · 2020-12-14T17:14:04Z

Old ping method doesn't work well. What about to create new sensor for old style ping? Currently I'm running patched version of new ping integration with latest HA for couple of days and it work like a charm.

ValentinBELYN · 2020-12-15T23:18:44Z

If you want to remove the fallback, you need to make sure that the operating system allows datagram sockets to use icmplib without root privileges. One solution would be to add the following line to the /etc/sysctl.conf configuration file in the installation and update prerequisites of Home Assistant:

net.ipv4.ping_group_range='0 2147483647'

A restart or the sysctl -p command will be necessary to take this new parameter into account.

To check the current value:

$ sysctl net.ipv4.ping_group_range

I don't know why this feature is disabled by default, especially since it doesn't introduce any security breach or compatibility errors.

8000

rds76 · 2020-12-16T08:46:31Z

@ValentinBELYN it's not issue for me, check here: #43188 (comment)
I'm already using it with updated net.ipv4.ping_group_range value, but for those that wants to use old ping version of sensor due to this limitation...

bdraco · 2020-12-30T03:01:09Z

I think we can use the unpriv code in more places with the ping('1.1.1.1', privileged=False) but we have to keep both paths for now

github-actions · 2021-01-29T03:27:48Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

bdraco · 2021-01-29T03:32:51Z

I still need to rework this

github-actions · 2021-02-28T05:17:48Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

bdraco · 2021-03-31T08:48:39Z

Been testing this for a week with device tracker and binary sensors. Startup only takes ~11s more with it vs multiple minutes before with many binary sensors.

frenck

LGTM, thanks @bdraco 👍

homeassistant added the cla-signed label Dec 2, 2020

probot-home-assistant bot added integration: ping small-pr PRs with less than 30 lines. labels Dec 2, 2020

bdraco marked this pull request as ready for review December 2, 2020 22:34

This was referenced Dec 2, 2020

ping offline hostname results in repeated log message "Error running command: ping -n -q -c 1 -W1 myhost.mynetwork.lan, return code: 2" #43785

Closed

Binary ping sensor unavailable after HA (re)start #43188

Closed

bdraco marked this pull request as draft December 2, 2020 22:54

github-actions bot added the stale label Jan 29, 2021

github-actions bot removed the stale label Jan 29, 2021

bdraco added 2 commits March 25, 2021 13:17

bump version

28638b2

bump version

89643b7

bdraco changed the title ~~Always use icmplib for ping~~ Use icmplib on newer kernels for ping Mar 25, 2021

tweaks

0dfad01

bdraco added the breaking-change label Mar 25, 2021

bdraco added 16 commits March 25, 2021 13:28

timeouts not the same

435662a

log it

1a396ce

log it

20f8eb8

multiping

6697928

multiping

9de661e

available is not alive

882704b

tweaks

7dc5015

fix thread race

e4eefb8

tweak

942df93

tweak

8511997

log

26a87f6

adjust

5b1fac1

test

4731d2c

await

21f7c49

gather

4c41d1c

fixes

dd857fd

bdraco changed the title ~~Use icmplib on newer kernels for ping~~ Prevent ping integration from delaying startup Mar 26, 2021

bdraco marked this pull request as ready for review March 26, 2021 01:08

init was missing

7d9de16

bdraco mentioned this pull request Mar 26, 2021

Provide api to see which integrations are being loaded #48274

Merged

28 tasks

frenck self-requested a review March 31, 2021 12:01

frenck approved these changes Mar 31, 2021

View reviewed changes

frenck merged commit bee55a0 into home-assistant:dev Mar 31, 2021

github-actions bot locked and limited conversation to collaborators Apr 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Prevent ping integration from delaying startup #43869

Prevent ping integration from delaying startup #43869

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Prevent ping integration from delaying startup #43869

Prevent ping integration from delaying startup #43869

Uh oh!

Conversation

Uh oh!

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Example entry for `configuration.yaml`: