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

LACP support #9

Closed
wants to merge 23 commits into from
Closed

LACP support #9

wants to merge 23 commits into from

Conversation

ydirson
Copy link
Collaborator

@ydirson ydirson commented Jan 13, 2023

NOTE: submitted upstream

This branch deals with the installer side of things, firstboot data needs to be handled in XAPI, for which we depend on xapi-project/xen-api#4333:

  • configuration from cmdline (covers fetching answerfile and the next items)
  • configuration from answerfile
  • configuration UI for netinstall
  • configuration UI of mgmt interface, writing data for firstboot

Known limitations we may want to address:

  • bad cmdline config could be reported cleanly (we currently get a python backtrace, which gets garbled by the TUI)
  • does not deal with DebStyleInterface, ie. does not support Debian-based installer; shouldn't that code be simply be nuked ?
  • limitations that could be addressed by moving bond creation/management to a preliminar "advanced networking" step similar to "advanced storage" (adding a new F key for this to initial screen may look bad, though):
    • bond interface is not created immediately, so appears in the interface list as "[no link]". Maybe this can be dealt with, by first creating ifcfg-* files without an IP config.
    • cannot change our mind once the eth* to create the bond have been made

This PR explicitly does not deal with:

  • creating more than 1 bond
  • system upgrade parsing during upgrade (get mgmt-iface config in ExistingInstallation.readSettings()), to which network_db seems to be unable to report about bonding (but the impact seems to be limited to default values). The system's bonds are already preserved on upgrade, though.

What the screens currently look like:

lacp-0-ifaces
lacp-1-create
lacp-2-confirm
lacp-3-ifaces

@ydirson ydirson marked this pull request as draft January 13, 2023 09:41
@ydirson ydirson changed the title WIP: LACP support LACP support Jan 13, 2023
@ydirson ydirson requested review from benjamreis and stormi January 13, 2023 09:42
netutil.py Outdated Show resolved Hide resolved
@benjamreis
Copy link

ydirson and others added 2 commits January 13, 2023 11:58
netutil.py Outdated Show resolved Hide resolved
tui/network.py Show resolved Hide resolved
@ydirson ydirson force-pushed the lacp branch 3 times, most recently from c2e1193 to 98396b0 Compare January 16, 2023 16:27
Always safer, and will facilitate further splitting
@ydirson
Copy link
Collaborator Author

ydirson commented Jan 18, 2023

Working on answerfile shows exacerbated need to pass a NIC object to NetInterface ctor (instead of "just hwaddr" initially, getting to which we now add bonding parameters). The only call sites for which conversion is still not obvious to me are the ones in product.py, dealing with the upgrade process.

This check was already not very useful, as if an eth device is not in
getNetifList(), the ifup command will just fail properly.

But in the case of a bond interface, the bond device will not even exist
before running ifup.

Signed-off-by: Yann Dirson <[email protected]>
This is a prerequisite to allowing more than just eth*

Signed-off-by: Yann Dirson <[email protected]>
Less complexity for the nominal case, and makes it easier to add different
non-nominal cases.

Signed-off-by: Yann Dirson <[email protected]>
…rpart

Don't repeat all parameters on the deprecated version, as we're going to
add more options.

Signed-off-by: Yann Dirson <[email protected]>
Using 1 name for 2 concepts is a bad idea, especially in a single function.

Also nukes one line of dead code.

Signed-off-by: Yann Dirson <[email protected]>
Signed-off-by: Yann Dirson <[email protected]>
(cherry picked from commit 50ecc56)
For bonding support we'll add more fields to NIC, that NetInterface needs
to know about.
FIXME:
- maybe support DebStyleInterface ?  how would we test ?
Bonding configuration created from commandline cannot be properly
passed to the UI, which needs it both for netinstall and
host-management, so we have to re-discover the existing configuration
to present it to the user.  This includes identifying the configured
bonding interface, and filtering out their member interfaces from
available choices.

This is not very satisfying, and we may want to improve the overall
installer architecture to make this better in the future, but it would be
way out of scope here.

Signed-off-by: Yann Dirson <[email protected]>
benjamreis and others added 3 commits January 20, 2023 17:09
We'll call them from upcoming lacp_bond_ui().
Making things visible is good (esp. here it allows to check interface
details), especially once we add the ability to create a bond
interface.

Signed-off-by: Yann Dirson <[email protected]>
Works at selectNetif level, and as such applies both to installer and
host-management networt configs.
@ydirson ydirson force-pushed the lacp branch 3 times, most recently from 9e3a7b1 to df5bed0 Compare January 23, 2023 14:50
@ydirson ydirson deleted the branch 10.10.x-8.3 May 23, 2023 08:54
@ydirson ydirson closed this May 23, 2023
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.

3 participants