Add network configuration check and report to ros2doctor #319

claireyywang · 2019-08-21T22:55:00Z

Most changes happen in ros2doctor/api/network.py. Network configuration is checked and printed using ifcfg python-pip package. ros2doctor/api/format.py is created to format the report. Current padding is 20 chars, but will be changed in the future to dynamically allocate padding based on string length. I have also modified print statements in ros2doctor/api/platform.py to reflect new report format.

Signed-off-by: claireyywang <clairewang@openrobotics.org>

waiting for rosdistro PR approval ros/rosdistro#22071

Signed-off-by: claireyywang <clairewang@openrobotics.org>

…os2cli into claire/ros2doctor-network Signed-off-by: claireyywang <clairewang@openrobotics.org>

claireyywang · 2019-08-21T23:04:20Z

Linux
Linux-aarch64
OSX
Windows

sloretz · 2019-08-21T23:09:30Z

ros2doctor/package.xml

@@ -9,7 +9,7 @@

  <depend>ros2cli</depend>

-  <exec_depend>python3-numpy</exec_depend>
+  <exec_depend>ifcfg-pip</exec_depend>


Is there a Debian release of this package? Packages released into a ROS distro (like the packages in this repo) can't use packages from PyPi because .debs can only depend on other Debian packages.

If there isn't a Debian release, an alternative is to release our own deb, which means creating a vendor package for ifcfg.

vendor package for ifcfg at https://github.com/ros2/ifcfg_vendor

need to change this dependency to ifcfg_vendor.

do I also need to change to ifcfg_vendor on rosdistro rosdep key?

do I also need to change to ifcfg_vendor on rosdistro rosdep key?

I don't think so. ifcfg-pip is the right name for the rosdistro key (the key isn't needed for this PR since there's a vendor package). The package.xml should be changed to depend on the new package ifcfg_vendor.

sloretz

mostly nitpicks

sloretz · 2019-08-23T16:24:49Z

ros2doctor/package.xml

@@ -9,7 +9,7 @@

  <depend>ros2cli</depend>

-  <exec_depend>python3-numpy</exec_depend>
+  <exec_depend>ifcfg-pip</exec_depend>


need to change this dependency to ifcfg_vendor.

sloretz · 2019-08-23T16:29:25Z

ros2doctor/ros2doctor/api/format.py

+def print_term(k, v):
+    """Print term only if it exists."""
+    if v:
+        # TODO: 20 padding needs to be dynamically set


nit, add parenthesis with your github username. TODO(claireyywang). The name in the parenthesis is the person who can answer questions about what needs to be done. Most of the time it's the person who created the comment.

sloretz · 2019-08-23T16:32:00Z

ros2doctor/ros2doctor/api/network.py

+
+def _is_unix_like_platform():
+    """Return True if conforms to UNIX/POSIX-style APIs."""
+    return platform.system() in ['Linux', 'FreeBSD', 'Darwin']


Recommend 'posix' == os.name instead

sloretz · 2019-08-23T16:40:06Z

ros2doctor/ros2doctor/api/network.py

+    return platform.system() in ['Linux', 'FreeBSD', 'Darwin']
+
+
+def check_sys_ips():


This function name doesn't seem very descriptive. It's a little more confusing in the context of ros2doctor since it's not one of the ros2doctor.checks. I don't have an alternative though.

changing to check_network_config_helper

sloretz · 2019-08-23T16:42:08Z

ros2doctor/ros2doctor/api/network.py

+        if 'LOOPBACK' in flags:
+            has_loopback = True
+        else:
+            has_others = True


recommend has_non_loopback instead

sloretz · 2019-08-23T16:45:23Z

ros2doctor/ros2doctor/api/network.py

+        sys.stderr.write('WARNING: Only loopback IP address is found.\n')
+    if not has_multicast:
+        sys.stderr.write('WARNING: No multicast IP address is found.\n')
+    if has_loopback and has_others and has_multicast:


nit, can eliminate result with return has_loopback and has_others and has_multicast

sloretz · 2019-08-23T16:51:12Z

ros2doctor/ros2doctor/api/platform.py

-    print('build                : ', platform.python_build())
+    print_term('version', platform.python_version())
+    print_term('compiler', platform.python_compiler())
+    print_term('build', platform.python_build())


This is definitely an improvement. I still think it would be good if the reports just returned information and the command was responsible for outputting it.

I was thinking about making that change as a feature in the next PR. Like you suggested last time, I'd like to add arguments like --report_all or --report_failed to let users control what they would like to see based on the check results.

dirk-thomas · 2019-08-23T17:03:20Z

Since adding a vendor package for ifcfg is quite some overhead and maintenance burden moving forward I would like to double check on this. What exact API / functionality is being used from that package? What other Python packages have been considered / checked if they could provide the same functionality?

claireyywang · 2019-08-23T17:30:18Z

Since adding a vendor package for ifcfg is quite some overhead and maintenance burden moving forward I would like to double check on this. What exact API / functionality is being used from that package? What other Python packages have been considered / checked if they could provide the same functionality?

ifcfg provides the same information as cmd ifconfig on terminal. It provides 'flags' that show whether an IP is used for loopback, broadcast or multicast etc. which I believe is important for ros2 checks. I also tried netifaces and socket but could not find enough information to check for multicast network interface.

Signed-off-by: claireyywang <clairewang@openrobotics.org>

claireyywang · 2019-08-23T21:39:41Z

Linux:
Linux-aarch64:
OSX:
Windows:

sloretz

LGTM

Before merging this, please also create a PR adding ifcfg_vendor to ros2.repos (like this PR). This and that will need to be merged at the same time to avoid breaking CI.

claireyywang added 8 commits August 19, 2019 17:31

add network checks and report

0a55cfa

Signed-off-by: claireyywang <clairewang@openrobotics.org>

network shenanigens

9f2428e

Signed-off-by: claireyywang <clairewang@openrobotics.org>

network shenanigens

f1d4071

Signed-off-by: claireyywang <clairewang@openrobotics.org>

network shenanigens

eee518c

Signed-off-by: claireyywang <clairewang@openrobotics.org>

add network check and report

0a13602

Signed-off-by: claireyywang <clairewang@openrobotics.org>

update code format

f0b0ce9

Signed-off-by: claireyywang <clairewang@openrobotics.org>

revised code format

5dec7e1

Signed-off-by: claireyywang <clairewang@openrobotics.org>

add ifcfg-pip rosdep key

0e3f0b6

waiting for rosdistro PR approval ros/rosdistro#22071

claireyywang added the in progress Actively being worked on (Kanban column) label Aug 21, 2019

claireyywang self-assigned this Aug 21, 2019

claireyywang added 2 commits August 21, 2019 15:56

added rosdep key ifcfg-pip

871a350

Signed-off-by: claireyywang <clairewang@openrobotics.org>

Merge branch 'claire/ros2doctor-network' of https://github.com/ros2/r…

3373522

…os2cli into claire/ros2doctor-network Signed-off-by: claireyywang <clairewang@openrobotics.org>

claireyywang force-pushed the claire/ros2doctor-network branch from 658e710 to 3373522 Compare August 21, 2019 23:00

claireyywang requested a review from sloretz August 21, 2019 23:09

sloretz reviewed Aug 21, 2019

View reviewed changes

sloretz reviewed Aug 23, 2019

View reviewed changes

revise code

2389097

Signed-off-by: claireyywang <clairewang@openrobotics.org>

claireyywang force-pushed the claire/ros2doctor-network branch from 386f0b1 to 2389097 Compare August 23, 2019 21:34

claireyywang added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 23, 2019

claireyywang requested a review from sloretz August 23, 2019 22:11

sloretz approved these changes Aug 26, 2019

View reviewed changes

claireyywang mentioned this pull request Aug 26, 2019

Add ifcfg_vendor to ros2 repos ros2/ros2#768

Merged

claireyywang merged commit 9f28544 into master Aug 26, 2019

delete-merged-branch bot deleted the claire/ros2doctor-network branch August 26, 2019 18:42

claireyywang mentioned this pull request Aug 28, 2019

Update report feature with new argument, add temp fix for ifcfg module #324

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add network configuration check and report to ros2doctor #319

Add network configuration check and report to ros2doctor #319

claireyywang commented Aug 21, 2019

claireyywang commented Aug 21, 2019 •

edited

Loading

sloretz Aug 21, 2019

claireyywang Aug 22, 2019

sloretz Aug 23, 2019

claireyywang Aug 23, 2019

sloretz Aug 23, 2019

sloretz left a comment •

edited

Loading

sloretz Aug 23, 2019

sloretz Aug 23, 2019

sloretz Aug 23, 2019

sloretz Aug 23, 2019

claireyywang Aug 23, 2019

sloretz Aug 23, 2019

sloretz Aug 23, 2019

sloretz Aug 23, 2019

claireyywang Aug 23, 2019

dirk-thomas commented Aug 23, 2019

claireyywang commented Aug 23, 2019

claireyywang commented Aug 23, 2019 •

edited

Loading

sloretz left a comment

		return platform.system() in ['Linux', 'FreeBSD', 'Darwin']


		def check_sys_ips():

Add network configuration check and report to ros2doctor #319

Add network configuration check and report to ros2doctor #319

Conversation

claireyywang commented Aug 21, 2019

claireyywang commented Aug 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Aug 23, 2019

claireyywang commented Aug 23, 2019

claireyywang commented Aug 23, 2019 • edited Loading

sloretz left a comment

Choose a reason for hiding this comment

claireyywang commented Aug 21, 2019 •

edited

Loading

sloretz left a comment •

edited

Loading

claireyywang commented Aug 23, 2019 •

edited

Loading