Skip to content

vxlan: convert some settings to tristate (LP: #2000712) #311

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

Merged
merged 6 commits into from
Feb 6, 2023

Conversation

daniloegea
Copy link
Contributor

@daniloegea daniloegea commented Jan 10, 2023

Vxlan learning is enabled by default on Linux. On Netplan it's a boolean variable that we initialize with false and only include in the generated configuration if it's true. That means we can't disable it on vxlan interfaces even if we define mac-learning: false explicitly in the yaml file.

This PR converts the settings mac-learning, arp-proxy and short-circuit from gboolean to tristate.

This addresses LP#2000712

Description

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

Vxlan learning is enabled by default on Linux. On Netplan it's a boolean
variable that we initialize with false and only include in the generated
configuration if it's true. That means we can't disable it on vxlan
interfaces even if we define mac-learning: false explicitly in the yaml
file.

That is not the ideal solution! The proper way to do that is defining it
as a tristate variable but it requires changes in the netplan_vxlan
structure. One bad side effect of this change is that the yaml
configuration generated from netdefs and the NM/networkd configuration
will contain the option mac-learning even if we don't have it in the
original yaml.

This addresses LP#2000712
@daniloegea daniloegea marked this pull request as draft January 10, 2023 18:54
@daniloegea
Copy link
Contributor Author

I'm not really happy with this change, but it avoids ABI changes.

There are other options that will not be emitted if they are false, but the problem doesn't really shows up when they are false by default on the system.

The proper solution would be to convert all these options to tristates.

@slyon slyon changed the title vxlan: initialize mac_learning with 1 vxlan: initialize mac_learning with 1 (LP: #2000712) Jan 23, 2023
Danilo Egea Gondolfo added 2 commits February 1, 2023 12:03
Settings mac-learning, arp-proxy and short-circuit were converted from
gboolean to tristate.

When these options are enabled by default in the backend, it's not
possible to disable them via Netplan as it was including the option in
the generated configuration only when it was set to true.

It addresses LP#2000712
@daniloegea daniloegea changed the title vxlan: initialize mac_learning with 1 (LP: #2000712) vxlan: convert some settings to tristate (LP: #2000712) Feb 1, 2023
@daniloegea
Copy link
Contributor Author

I converted three settings from gboolean to tristates. It's shouldn't change the struct size/layout as both types are actually ints.

@daniloegea daniloegea marked this pull request as ready for review February 1, 2023 16:11
@daniloegea daniloegea requested a review from slyon February 1, 2023 16:11
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

lgtm overall! Let's reduce that now unused handle_vxlan_bool helper and try to reduce the new test's size a bit, by using some of the available templates from base.py

@slyon slyon merged commit 492d162 into canonical:main Feb 6, 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