Skip to content
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.

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

feat(linux.net): DHCP server selection method #5578

Merged
merged 13 commits into from
Nov 25, 2024

Conversation

pierantoniomerlino
Copy link
Contributor

This PR updates the method used to check if dnsmasq is available on the system.

Related Issue: This PR fixes/closes N/A

Description of the solution adopted: Since the DHCP server manager uses the Systemd unit for dnsmasq, the method used to check the presence of the tool on the system has to check the existence of the unit and the command. This PR adds this check.

The check runs this command: systemctl status dnsmasq. Then the exit code is checked, based on the official documentation. Since the DhcpServerManager class uses a static method to check the tools, the method in LinuxNetworkUtil has to be static too.
This implies that the CommandExecutorService cannot be used since we haven't a reference to it. So, the method uses the standard ProcessBuilder class to run the command.

I don't like it.

In addition to the test for this specific feature, this PR adds few test on the getTool method.

Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
@pierantoniomerlino pierantoniomerlino force-pushed the update_dhcp_server_selection branch from 01fbd13 to ff50d42 Compare November 21, 2024 13:24
Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
@mattdibi
Copy link
Contributor

mattdibi commented Nov 22, 2024

Tested on RPi and found the following issue.

Starting point: in this setup isc-dhcp-server, dnsmasq and dnsmasq-base are all installed on the system.

When dnsmasq is removed with apt remove

Kura still thinks the unit is "functional" and "available".

image

In Kura logs:

2024-11-22T10:48:51,302 [Start Level: Equinox Container: 831d6796-ff4e-4b32-b8e2-7ea0f15b04f7] INFO  o.e.k.l.n.d.DhcpServerManager - Using dnsmasq as DHCP server.

which results in client not being able to correctly get an IP address from Kura's access point.

When dnsmasq is removed with apt purge

image

In Kura logs:

2024-11-22T11:08:25,565 [Start Level: Equinox Container: 76f5aed4-bdae-40e0-8249-9cac2fb13f8a] INFO  o.e.k.l.n.d.DhcpServerManager - Using dhcpd as DHCP server.

Conclusion

systemctl status return value is not entirely reliable to evaluate whether the systemd unit is installed and functional.

@marcellorinaldo
Copy link
Contributor

marcellorinaldo commented Nov 22, 2024

Tested working on 3 different scenarios:

  1. dnsmasq-base installed, dnsmasq not installed, /usr/sbin/dnsmasq binary present, isc-dhcp-server installed
  2. dnsmasq-base installed, dnsmasq installed, /usr/sbin/dnsmasq binary present, isc-dhcp-server installed
  3. dnsmasq-base installed, dnsmasq installed, /usr/sbin/dnsmasq binary present, isc-dhcp-server not installed

In scenario 1, Kura picked up dhcpd as DHCP server tool. In scenarios 2 and 3 dnsmasq was selected correctly. In all 3 scenarios the following basic test passes:

  • Configure an interface as WAN, configure another one as DHCP server with NAT with passDNS=true
  • Connect a client to the DHCP server interface and ping google

@marcellorinaldo
Copy link
Contributor

Conclusion

systemctl status return value is not entirely reliable to evaluate whether the systemd unit is installed and functional.

Exit code is 3 when apt remove. When purging, exit code is 4. Currently we check that systemctl status returns an exit code < 4, we might solve this by changing just that

@mattdibi
Copy link
Contributor

  1. dnsmasq-base installed, dnsmasq not installed, /usr/sbin/dnsmasq binary present, isc-dhcp-server installed

3 is returned when dnsmasq is installed but stopped. That's why this is not reliable.

Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
mattdibi
mattdibi previously approved these changes Nov 22, 2024
Copy link
Contributor

@mattdibi mattdibi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with the latest changes: when the condition described in #5578 (comment) verifies, we get the following error in the log:

2024-11-22T14:18:08,345 [pool-22-thread-1] ERROR o.e.k.l.n.d.s.DnsmasqTool - dnsmasq Systemd unit startup failed. Is dnsmasq package installed on the system?

which gives enough information for the user to fix this issue.

Therefore, given that the situation in which

  1. The user has isc-dhcp-server, dnsmasq and dnsmasq-base installed.
  2. They remove dnsmasq without purging
  3. Install kura

is quite unlikely and programmatically handling this situation brings diminishing returns, we accept current solution albeit not completely reliable.

We're planning to remove the support for isc-dhcp-server in the near future and thus this issue should be even less relevant.

Signed-off-by: pierantoniomerlino <[email protected]>
Signed-off-by: pierantoniomerlino <[email protected]>
@pierantoniomerlino pierantoniomerlino merged commit af10940 into develop Nov 25, 2024
3 checks passed
@pierantoniomerlino pierantoniomerlino deleted the update_dhcp_server_selection branch November 25, 2024 08:04
MMaiero pushed a commit that referenced this pull request Dec 20, 2024
* Implemented systemd unit exists method

Signed-off-by: pierantoniomerlino <[email protected]>

* Added folder and tests

Signed-off-by: pierantoniomerlino <[email protected]>

* Replaced method for checking unit presence; test updated

Signed-off-by: pierantoniomerlino <[email protected]>

* Applied cleanup

Signed-off-by: pierantoniomerlino <[email protected]>

* Reverted variable name

Signed-off-by: pierantoniomerlino <[email protected]>

* Fixed sonar complains

Signed-off-by: pierantoniomerlino <[email protected]>

* Improved tool search

Signed-off-by: pierantoniomerlino <[email protected]>

* Changed logger level

Signed-off-by: pierantoniomerlino <[email protected]>

* Added log when dnsmasq start fails

Signed-off-by: pierantoniomerlino <[email protected]>

* Fixed minor issues

Signed-off-by: pierantoniomerlino <[email protected]>

* Simplified command process

Signed-off-by: pierantoniomerlino <[email protected]>

* Removed useless inner class

Signed-off-by: pierantoniomerlino <[email protected]>

* Improved logging

Signed-off-by: pierantoniomerlino <[email protected]>

---------

Signed-off-by: pierantoniomerlino <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants