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

[WiP] add possibility to configure lacp bonding at install #25

Closed
wants to merge 2 commits into from

Conversation

benjamreis
Copy link
Contributor

No description provided.

@benjamreis
Copy link
Contributor Author

xcp-ng/xcp#350

Copy link
Member

@stormi stormi left a comment

Choose a reason for hiding this comment

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

Given the amount of changes, it becomes really important to find a way to contribute it upstream, to avoid merge conflicts each time they change code around this.

@@ -69,6 +69,13 @@ def __init__(self, mode, hwaddr, ipaddr=None, netmask=None, gateway=None,
self.ipv6addr = None
self.ipv6_gateway = None

if bond_mode is not None:
# Not `balance-slb` because it's openvswitch specific
assert bond_mode in ["lacp", "active-backup"]
Copy link
Member

Choose a reason for hiding this comment

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

What if the users provided a different value? How will the error be displayed to them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk, i've reproduced how such errors are handled in the code.
The UI won't allow to give other values. The answerfiles think will be handled in answerfile.py so this is not where to treat user error display.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so this means you'll sanitize answerfile input so that those asserts are never reached?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, once i'm tackling this part in the parsong of the answerfile.

@benjamreis
Copy link
Contributor Author

Yes upstream our changes would be nice.
Especially since nothing is us-specific in terms of features. :)

@benjamreis
Copy link
Contributor Author

Obsoleted by xcp-ng/host-installer#9

@benjamreis benjamreis closed this Jan 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.

2 participants