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.

Sign up for GitHub

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

Jump to bottom

network: fix crash upon accessing Help menu after creating a bond #1786

Merged
merged 2 commits into from
Sep 7, 2023
Merged

network: fix crash upon accessing Help menu after creating a bond #1786

merged 2 commits into from
Sep 7, 2023

Conversation

ogayot
Copy link
Member

@ogayot ogayot commented Sep 5, 2023 •
edited
Loading

Steps to reproduce (works in dry-run mode too):

  1. Navigate to the network page.
  2. Create a bond
  3. Go to the help menu
 Traceback (most recent call last):
   File "subiquity/common/api/server.py", line 164, in handler
     result = await implementation(**args)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   File "subiquity/server/server.py", line 117, in ssh_info_GET
     ips.extend(map(str, dev.actual_global_ip_addresses))
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   File "subiquitycore/models/network.py", line 394, in actual_global_ip_addresses
     for _, addr in sorted(self.info.addresses.items())
                           ^^^^^^^^^^^^^^^^^^^
 AttributeError: 'NoneType' object has no attribute 'addresses'

LP: #2039086

mwhudson
mwhudson approved these changes Sep 5, 2023
View reviewed changes
Copy link
Collaborator

@mwhudson mwhudson left a comment

Choose a reason for hiding this comment

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

Hm finding bugs in code I wrote in like 2017, rude!

Would this be better fixed in the device object, like the various _address properties should be empty in this case?

@ogayot
Copy link
Member Author

ogayot commented Sep 6, 2023 •
edited
Loading

Hm finding bugs in code I wrote in like 2017, rude!

Would this be better fixed in the device object, like the various _address properties should be empty in this case?

Yes, I imagine it would be a better fix for it. Actually, I am trying to debug this code (because of another crash occurring when an interface is removed) and the NetworkDev.info instance variable is puzzling me.

When creating a NetworkDev instance, the info variable gets set to None. IIUC, we expect it to hold a NetDevInfo instance at a later stage. To create such instance, we can call the dev.netdev_info() method ... but it is implemented in a way that dereferences dev.info for specific types of interfaces.

dev = NetworkDev(...)
dev.info = dev.netdev_info()  #  <-- This will crash for some types of interfaces (e.g., "eth", "wlan")

dev.info can be assigned by the NetworkModel.new_link method , but I don't know where the NetDevInfo instances comes from..

EDIT I looked more into it and figured dev.info is actually a probert.network.Link instance - although it resembles NetDevInfo in a way.

I don't think it makes sense to manually create a probert.network.Link instance in Subiquity. And probert is surely not going to help until the device is "physically" created.

@ogayot ogayot force-pushed the virt-netdev-help-crash branch from a24dfd1 to 5fd2620 Compare September 6, 2023 12:56
@ogayot
network: fix crash upon accessing Help after creating a bond
dc26a1d 
When accessing the Help menu, Subiquity looks up the IP addresses
currently configured - so it knows whether to show the "Help on SSH
access" option.

Unfortunately, it also looks for IP addresses on devices that were
"configured" through the network screen but that still do not exist in
the system. When such a device exist (e.g., a bond), the Subiquity
client crashes with the following exception:

 Traceback (most recent call last):
   File "subiquity/common/api/server.py", line 164, in handler
     result = await implementation(**args)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   File "subiquity/server/server.py", line 117, in ssh_info_GET
     ips.extend(map(str, dev.actual_global_ip_addresses))
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   File "subiquitycore/models/network.py", line 394, in actual_global_ip_addresses
     for _, addr in sorted(self.info.addresses.items())
                           ^^^^^^^^^^^^^^^^^^^
 AttributeError: 'NoneType' object has no attribute 'addresses'

A similar crash is observed when calling /network/global_addresses after
creating the bond.

Fixed by only checking the IP addresses of devices that have a
probert.network.Link instance (i.e., they exist in the system).

Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
@ogayot
network: document and add type hint for NetworkDev.info
221466a 
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
@ogayot ogayot force-pushed the virt-netdev-help-crash branch from 5fd2620 to 221466a Compare September 6, 2023 12:59
mwhudson
mwhudson approved these changes Sep 7, 2023
View reviewed changes
Copy link
Collaborator

@mwhudson mwhudson left a comment

Choose a reason for hiding this comment

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

Yes, self.info is None for devices that exist in the UI but not yet on the system (or at least that's the idea -- like I said this is very old code!)

@ogayot ogayot merged commit 5cc6f5d into canonical:main Sep 7, 2023
@ogayot ogayot deleted the virt-netdev-help-crash branch September 7, 2023 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwhudson mwhudson mwhudson approved these changes

@dbungert dbungert dbungert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ogayot @dbungert @mwhudson