Skip to content

Handle parent=none for physical networks #2073

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

shama7g
Copy link

@shama7g shama7g commented May 3, 2025

These are the following steps I took to handle the issue. Please let me know if there are any problems or changes I should implement. Thank you!

Part (1): Skip uplink IP allocation when parent == "none"

  • File: internal/server/network/network_ovn.go
  • Function: setupUplinkPort()
  • Change:
    • Added a continue to skip uplinkAllocateIP logic if parent == "none".
    • Ensures we don't try to allocate uplink IPs when the uplink network is intentionally disabled.
      Part (2): Avoid startUplinkPort() when parent == "none"
  • File: internal/server/network/network_ovn.go
  • Function: Start()
  • Change:
    • Early return (return nil) if n.config["parent"] == "none" before calling startUplinkPort() and BGP logic.
    • Prevents unnecessary OVN uplink initialization when there's no external network.
      Part (3): Avoid joining chassis group when parent == "none"
  • File: internal/server/network/network_ovn.go
  • Function: chassisEnabled()
  • Change:
    • Added conditional check: if n.config["parent"] == "none", return false.
    • Ensures we don't participate in OVN chassis groups in this disabled uplink mode.

@stgraber
Copy link
Member

stgraber commented May 7, 2025

I'm going to be looking into this one next.

@stgraber stgraber linked an issue May 8, 2025 that may be closed by this pull request
1 task
@@ -459,6 +459,11 @@ func networksPost(d *Daemon, r *http.Request) response.Response {
// A targetNode was specified, let's just define the node's network without actually creating it.
// Check that only NodeSpecificNetworkConfig keys are specified.
for key := range req.Config {
// Special-case: allow "parent=none". Used to indicate that this node should not act as a gateway chassis.
Copy link
Member

Choose a reason for hiding this comment

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

parent is a node-specific config, so this change isn't needed

Comment on lines +3454 to +3459
// Skip full start logic if parent=none.
if n.config["parent"] == "none" {
n.logger.Info("Skipping OVN Start due to parent=none", logger.Ctx{"project": n.project, "name": n.name})
n.setAvailable()
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

That looks wrong. You're skipping everything in the function which includes a lot of bits that are actually needed.

I think you should instead modify startUplinkPort to skip when the uplink network is of type physical and has a parent property set to none.

Comment on lines +3402 to +3405
// If parent is "none", this network is standalone and should not act as a chassis.
if n.config["parent"] == "none" {
return false, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

This check is incorrect. n.config here refers to the config of the OVN network when the parent check needs to be done on the uplink network instead.

So you should change the check to:

  • Check that we have an uplink network at all d.config["network"] != "none"
  • Check that the uplink network is of type physical
  • Check if the parent property on the uplink network is set to none

@stgraber stgraber marked this pull request as draft May 8, 2025 03:26
@stgraber stgraber marked this pull request as ready for review May 8, 2025 03:26
@stgraber
Copy link
Member

stgraber commented May 8, 2025

Moving this back to draft as this doesn't actually fix the issue at all.
Hopefully the comments I left point you in the right direction to get this to actually work.

@stgraber stgraber marked this pull request as draft May 26, 2025 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Handle parent=none for physical networks
2 participants