Skip to content

Update parsing of ip addr show (SC-723)#1183

Closed
TheRealFalcon wants to merge 2 commits into
canonical:mainfrom
TheRealFalcon:ip-metric
Closed

Update parsing of ip addr show (SC-723)#1183
TheRealFalcon wants to merge 2 commits into
canonical:mainfrom
TheRealFalcon:ip-metric

Conversation

@TheRealFalcon
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon commented Jan 11, 2022

Proposed Commit Message

Update parsing of ip addr show

"ip addr show" may include a metric on the inet line.
Parse this while also keeping backwards compatibility
for lines without.

Additional Context

Example ip addr show on Jammy:

root@jammy:~# ip addr show
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host 
       valid_lft forever preferred_lft forever
118: eth0@if119: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether 00:16:3e:80:4f:ea brd ff:ff:ff:ff:ff:ff link-netnsid 0
    inet 10.85.130.57/24 metric 100 brd 10.85.130.255 scope global dynamic eth0
       valid_lft 2912sec preferred_lft 2912sec
    inet6 fd42:baa2:3dd:17a:216:3eff:fe80:4fea/64 scope global dynamic mngtmpaddr noprefixroute 
       valid_lft 3545sec preferred_lft 3545sec
    inet6 fe80::216:3eff:fe80:4fea/64 scope link 
       valid_lft forever preferred_lft forever

Notice the metric 100 on the inet line.

Test Steps

Can be seen in jammy integration tests on LXD as a new unexpected warning.

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

Newer versions of "ip addr show" include a metric on the inet line.
Parse this while also keeping backwards compatibility.
@TheRealFalcon
Copy link
Copy Markdown
Contributor Author

pytest parametrization doesn't work with the base class, so I removed the base class from the tests.

The only functional test change is in test_netdev_iproute_pformat

@TheRealFalcon TheRealFalcon changed the title Update parsing of ip addr show Update parsing of ip addr show (SC-723) Jan 11, 2022
@@ -0,0 +1,12 @@
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
inet 127.0.0.1/8 scope host lo\ valid_lft forever preferred_lft forever
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are the backslashes on lines 3 and 4 a typo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I just copied the sample-ipaddrshow-output file and added the metric part

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see, nevermind. It doesn't matter what it says after scope since the regex ends with .*.

Comment thread cloudinit/netinfo.py
r"\s+inet\s(?P<cidr4>\S+)(\sbrd\s(?P<bcast>\S+))?\sscope\s"
r"(?P<scope>\S+).*",
r"\s+inet\s(?P<cidr4>\S+)"
r"(\smetric\s(?P<metric>\d+))?"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I did a little bit of manual testing with this regex and didn't spot any issues.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The bigger this regex gets, the more it makes me question whether we should really be extending a fragile regex parsing of human-readable output of ip. While I originally weighed in to the contrary because cloud-init is supported even more broadly than just Ubuntu, I think it would probably be preferable given that we attempt to try ip -json format first, and fallback to the legacy parsing if -json param is not present. This way most of the latest *nix distros with recent ip tooling can benefit from simple parsing of machine-readable output and any older distributions will continue to parse correctly with the older ip tooling and output formats.

Copy link
Copy Markdown
Member

@holmanb holmanb left a comment

Choose a reason for hiding this comment

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

lgtm

@mwhudson
Copy link
Copy Markdown
Contributor

Is there a reason you are not parsing the output of 'ip -json addr show' instead?

@TheRealFalcon
Copy link
Copy Markdown
Contributor Author

@mwhudson It was proposed, but we decided against it due to lack of availability everywhere.

@mwhudson
Copy link
Copy Markdown
Contributor

@mwhudson It was proposed, but we decided against it due to lack of availability everywhere.

Ah yes I see it's not supported in xenial.

@TheRealFalcon
Copy link
Copy Markdown
Contributor Author

Closing in favor of #1210

@TheRealFalcon TheRealFalcon deleted the ip-metric branch January 24, 2022 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants