Skip to content

Conversation

rafaellehmkuhl
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl commented Feb 16, 2022

Before this patch, cable-guy was only able to handle one address per ethernet interface. This was leading to issues like the user only being able to see one IP address on the frontend (even when the interface had multiple), which could cause some misunderstood.

With this patch, the user is now able to:

  • See multiple IP addresses associated with each ethernet interface
  • Add new IP addresses to a given interface
  • Remove specific IP addresses from a given interface

image

Also, some error messages were made more clear, raises were added where necessary and logs from settings are now done with loguru.

Fix #428

Raising allows the caller to handle the situation while still receiving details about the error.
@rafaellehmkuhl rafaellehmkuhl force-pushed the more-than-one-ip branch 4 times, most recently from 54cce82 to efc73a2 Compare February 16, 2022 20:19
Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

Is there a docker ?

@rafaellehmkuhl
Copy link
Member Author

Is there a docker ?

Yup, same same as the branch

Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

It's showing three IP blocks, but eth0 here only have 2
image
image

[
  {
    "name": "eth0", 
    "addresses": [
      {
        "ip": "192.168.2.2", 
        "mode": "unmanaged"
      }, 
      {
        "ip": "192.168.0.46", 
        "mode": "client"
      }, 
      {
        "ip": "undefined", 
        "mode": "client"
      }
    ], 
    "info": {
      "connected": true, 
      "number_of_disconnections": 6
    }
  }
]
  • It's appears possible to set a server DHCP, from the options but nothing happens.
  • When deleting an IP configuration, it still shows for a couple of seconds after finishing the update on the UI.

When creating a new dynamic IP, more 3 IPs appears as IPv6 here.
image
image

"""
logger.info(f"Adding IP '{ip_address}' on interface '{interface_name}' in {mode} mode.")
try:
self.get_interface_by_name(interface_name)
Copy link
Member

Choose a reason for hiding this comment

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

this looks a bit odd. is it here only you make sure the interface is valid? if so I'd add a comment

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, yes. Will add a comment.

@rafaellehmkuhl
Copy link
Member Author

It's showing three IP blocks, but eth0 here only have 2 image image

[
  {
    "name": "eth0", 
    "addresses": [
      {
        "ip": "192.168.2.2", 
        "mode": "unmanaged"
      }, 
      {
        "ip": "192.168.0.46", 
        "mode": "client"
      }, 
      {
        "ip": "undefined", 
        "mode": "client"
      }
    ], 
    "info": {
      "connected": true, 
      "number_of_disconnections": 6
    }
  }
]
  • It's appears possible to set a server DHCP, from the options but nothing happens.
  • When deleting an IP configuration, it still shows for a couple of seconds after finishing the update on the UI.

When creating a new dynamic IP, more 3 IPs appears as IPv6 here. image image

About the IPV6 behavior, I made some tests and came to see that this is the behavior we already have in master, which makes sense, since we don't have anything blocking IPV6 or something like that (we just don't add them to our interface models):

image

About deleting an IP and it staying there, it is a behavior we have in all frontend managers. Basically we put the component in an "updating" state when a change was made, and the next fetch will put update the data and put it in a "not-updating" state. The problem happens when a fetch initiates, a change occurs, and the fetch completes after the change but with the old data. There are multiple solutions to that and we should discuss on what is the behavior we want, blueos-wise (same behavior for all services).

I'm opening an issue to discuss that.

About the DHCP server issue, that's a bug indeed. As we already have the 192.168.2.2 IP there, when we set a DHCP server (which uses the same IP), it should remove the old one and add the new, but was not doing that. Fixed.

@patrickelectric
Copy link
Member

About the IPV6 behavior, I made some tests and came to see that this is the behavior we already have in master, which makes sense, since we don't have anything blocking IPV6 or something like that (we just don't add them to our interface models):

This was happening when adding a new dynamic IP with this patch, 3 more IP interfaces appears, and not one.

@rafaellehmkuhl rafaellehmkuhl force-pushed the more-than-one-ip branch 2 times, most recently from d8f7a8b to 94b95d3 Compare February 18, 2022 16:21
@rafaellehmkuhl
Copy link
Member Author

Image updated with proper fixes.

Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

Its failing to change to dynamic IP:
image

image

image

The first link does not change to dynamic or a new dynamic is not added.
If we only support a single dynamic ip, we should remove the dynamic IP option and server option if they are already available.

@rafaellehmkuhl
Copy link
Member Author

Its failing to change to dynamic IP: image

image

image

The first link does not change to dynamic or a new dynamic is not added. If we only support a single dynamic ip, we should remove the dynamic IP option and server option if they are already available.

Yes. I'm aiming with this patch to just support multiples IPs, but not change the manager behavior. I have a patch for the sequence that will do some behavior changes to make it clear to the user what's happening. Basically I changed the DHCP and Dynamic IP creation for a button allowing enabling/disabling DHCP server, as we support only one, and another button to "re-trigger" dynamic IP instead of "adding", as we don't control how many and which IPs the external DHCP server will send.

@patrickelectric patrickelectric merged commit 7d294b6 into bluerobotics:master Feb 21, 2022
@rafaellehmkuhl rafaellehmkuhl deleted the more-than-one-ip branch February 21, 2022 20:03
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.

cable-guy: No support for more than one IP on the same interface
3 participants