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

backends: LXD: don't hardcode eth0 #178

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

thp-canonical
Copy link
Contributor

@thp-canonical thp-canonical commented Feb 21, 2024

With the ubuntu-22.04 image, the network interface seems to be sometimes named "enp5s0" instead of "eth0". To be compatible with both old and new releases, consider any non-loopback interface viable for retrieving an IP address.

With the ubuntu-22.04 image, the network interface seems
to be named "enp5s0" instead of "eth0". To be compatible
with both old and new releases, consider any non-loopback
interface viable for retrieving an IP address.
Copy link
Contributor

@bboozzoo bboozzoo left a comment

Choose a reason for hiding this comment

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

LGTM. I have the exact same change in my branch.

@bboozzoo
Copy link
Contributor

ping @ZeyadYasser, please have a look

Copy link

@ZeyadYasser ZeyadYasser left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this, would it be a good idea to add a check that the server is reachable by the host through this IP address (using an icmp ping or something similar) before selecting it to avoid a case where there are multiple addresses but one of them is not accessible by the host.

CC: @bboozzoo

@bboozzoo
Copy link
Contributor

Unless the configuration adds another interface explicitly, the container will have lo and another non-lo interface (the problem here is that the name was hardcoded), so I suppose we could have a check if there's at most 2 interfaces, or simply only 1 that isn't lo? I'm not sure the connectivity check is necessary, as we did not have one before and the thing will simply fail on the next step trying to connect to the VM. This is exactly what happens without this change.

@thp-canonical
Copy link
Contributor Author

I suppose we could have a check if there's at most 2 interfaces, or simply only 1 that isn't lo?

I'm assuming here that if multiple non-lo network interfaces are configured, any network interface would work for connecting (the first one that matches and has an IPv4 address), so this isn't strictly incompatible with multiple interfaces.

Spread didn't have the restriction of "only a single non-lo interface" before (it would work just fine with multiple interfaces, as long as there also was eth0), so I didn't want to introduce this additional restriction here.

If there are multiple interfaces where only some of them are suitable for Spread to connect via SSH, we probably need additional configuration anyway (and that is probably outside of the scope of this PR).

Copy link

@ZeyadYasser ZeyadYasser left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you!

spread/lxd.go Outdated
Comment on lines 446 to 447
for intf, network := range sjson.State.Network {
if intf == "lo" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you change these variable names to match the corresponding chunk in #185?

Copy link
Collaborator

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

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

Thank you

Copy link
Contributor

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. There's indeed a question about multiple interfaces, but this looks like at least a good start and solves the common case. Let's see how far we get with it alone.

@niemeyer niemeyer merged commit cbdb5e2 into canonical:master Dec 9, 2024
1 check passed
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.

5 participants