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

Add support for "dummy" interfaces (LP: #1774203) #361

Merged
merged 6 commits into from
Jun 21, 2023

Conversation

daniloegea
Copy link
Collaborator

@daniloegea daniloegea commented May 22, 2023

Description

This is a resurrection of #115. I didn't reuse the PR because it's old and incomplete.

It implements support for parsing YAMLs and Keyfiles so it works with netplan-everywhere (Network Manager + Netplan).

Related LP bug: https://bugs.launchpad.net/netplan/+bug/1774203

Unfortunately the Linux kernel name for this type of interface conflicts with our sensitive-wording policy so I had to add some annotations in the code to ignore the errors from Woke. Our words list doesn't catch the word 'dummies' though (flaw I exploited to not insert the wokeignore annotation on some places), so I don't know, maybe we could just drop "dummy" from our denylist...

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. LP#1774203

@daniloegea daniloegea requested a review from slyon May 22, 2023 19:59
@slyon slyon changed the title Add support for "dummy" interfaces Add support for "dummy" interfaces (LP: #1774203) May 23, 2023
@slyon slyon added schema change This PR includes a change to netplan's YAML schema, which needs sign-off ABI-compatible ABI changed in a compatible way labels May 23, 2023
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, some small nitpicks inline.

The new "dummies"/"dummy" schema is non-inclusive language, but I agree it's the common wording, also used by iproute2, systemd-networkd and NetworkManager. We should not diverge from that common understanding.

Generally, would you mind adding some examples for those new dummy interfaces in the docs or examples/?

Let's try to get some more opinions on that.

Also we should keep in mind this old comment from cyphermox:

I'm not fundamentally opposed to dummy devices, but I wonder if they wouldn't better fit elsewhere (like under ethernet, simply) since dummies are indeed a thing, but once you move into network hardware, you'll much more likely want to create loopback devices, which is something else that is not currently straightforward.

doc/netplan-yaml.md Outdated Show resolved Hide resolved
tests/integration/dummies.py Show resolved Hide resolved

[suppress_type]
type_kind = typedef
name = NetplanDefType
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree we can allow this ABI change to the NetplanDeFType enum, as we're only changing the internal NETPLAN_DEF_TYPE_MAX_ value, which does not affect existing consumers.

@slyon slyon mentioned this pull request May 23, 2023
4 tasks
@daniloegea
Copy link
Collaborator Author

Thanks, Lukas.

I already included one example in the docs, do you think I should also add a file in examples/?

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.

Thanks.

I think examples/*.yaml is what's being used for tools/run_asan.sh in our memory leak CI, so it would be nice to have one dummy example in there, too. But not required.

Let's wait on consensus for the schema change (& adding "schema ok" label), before merging.

@daniloegea
Copy link
Collaborator Author

That's a good point. I added the examples file and rebased.

@slyon slyon requested a review from vorlonofportland May 24, 2023 16:02
@varCharlie
Copy link

Hopping on here to say I will be very happy when netplan allows the use of dummy interfaces... anyone doing anycast routing will use this feature (lots of big network operators).

It will be a great way to make netplan more useful for managing network services on servers

@@ -12,6 +12,7 @@ network:
renderer: STRING
bonds: MAPPING
bridges: MAPPING
dummies: MAPPING

Choose a reason for hiding this comment

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

Hmm, this is difficult. While dummy is clearly correct, the plural dummies evokes for me images of mannequins or crash test dummies or martial arts training dummies. At the risk of being too long, what do you think of dummy-interfaces here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh dummy-interfaces would deviate from the naming pattern... ethernets, bonds, bridges... dummy-interfaces... as a non-native English speaker I wasn't seeing the type of dummies you mentioned but now I can't unsee them :D

If we are not going with dummies, maybe we could go with dummy-devices then? Because we already have a nm-devices category.

Choose a reason for hiding this comment

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

works for me

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for me... I didn't think about cash test dummies and thought "dummies" would be most in line with what we have so far for the other types. But I see Steve's point. Considering we also have "nm-devices", I think I'd go with "dummy-devices" instead of "dummy-interfaces".

@daniloegea daniloegea force-pushed the dummies branch 2 times, most recently from 164236a to 0f623bf Compare June 20, 2023 17:41
@daniloegea
Copy link
Collaborator Author

I replaced dummies by dummy-devices (and also rebased the branch). I also had to do some changes in our woke ruleset because of the word dummy...

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.

One small inline question about .woke vs our documentatio wrt "dummy" words. Otherwise this is good to be merged. Thanks!

.woke.yaml Outdated Show resolved Hide resolved
daniloegea and others added 5 commits June 21, 2023 16:13
Also, as we can't go around the woke linter limitations at the moment,
remove the word "dummy" from the rulset. We can add it back the day woke
allows us to use exceptions.
abidiff is failing due to the change in the enum NetplanDefType. Let's
add it to the suppression list until we bump up the ABI for the next
major release.
Co-authored-by: Lukas Märdian <slyon@ubuntu.com>
@daniloegea daniloegea merged commit 75acd64 into canonical:main Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ABI-compatible ABI changed in a compatible way schema change This PR includes a change to netplan's YAML schema, which needs sign-off schema ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants