Support for macvlan connections`

[linux-system-roles]

Support for macvlan connections`

[thom311]

cool, thanks for reporting

Regarding initscripts support... there are various initscripts flavours, like Fedora/RHEL, SuSE. For the moment, the role only targets the Fedora/RHEL flavour, simply because there is no testing happening for other distros. So, unless somebody shows serious interest in testing and fixing initscripts for other distros, I'd rather not merge any patches that create initscripts that won't work on Fedora/RHEL. The referenced initscripts-macvlan is also not part of RHEL, so the same applies. But we can add macvlan support for NetworkManager only. Of course, optimally all providers support all options, but that is not a hard rule. I think Cmd_initscripts should overload run_prepare() and error out when encountering a macvlan device.

Regarding the patch, a few comments:

• I think it shouldn't add a macvlan_parent property, but reuse the existing parent, like for vlan.

• Regarding macvlan_mode, it is the same style as VLAN's vlan_id. I am no longer convinced this was the right decision, and would prefer that such properties are nested like

   - type: "macvlan"
   - parent: "eth0"
   - macvlan:
       - mode: "bridge"

How about that? Look at ArgValidator_DictBond() which does the same for bonds.

If you are willing, please open a pull request. Thanks!

[rpabel]

• I am totally fine with this not working with initscripts (for now... or ever). I'll take out that part of the patch and see that I add a check for macvlans in Cmd_initscripts.

• I can certainly create a ArgValidator_DictMacvlanModes class and change the structure of the parameters as you described. Right now, all modes have to be specified twice (in NMUtil and ArgValidator_DictConnection), the mode names should be private to this class only then, which is preferable.

• I first tried using "parent" for the macvlan_parent, but parent parameter is checked to be a valid connection in a several places, e.g.:
  ArgUtil.connection_find_by_name(connection['parent'], result, idx)
  But for macvlan, the parent can be an unmanaged device. nmcli calls the option "dev" and allows
  dev <parent device (connection UUID, ifname, or MAC)>
  I only called it "parent" because that is the name the libnm uses: NM.SETTING_MACVLAN_PARENT.
  I agree reusing the parameter parent is favorable, but it would mean touching lots of other lines...

I'll get on it and prepare another patch.

[thom311]

ArgValidator_DictMacvlanModes should be ArgValidator_DictMacvlan.

I first tried using "parent" for the macvlan_parent, but parent parameter is checked to be a valid
connection in a several places, e.g.:

Yeah, that's intended. As far as NetworkManager is concerned, you can create a VLAN/MACVlan with a parent that is not managed by NetworkManager. The "parent" either references an interface-name or the UUID or another connection profile.
For the role, the "parent" is always the name of another profile in the same play, like

     - name: wired
       type: ethernet
       interface_name: eth0
     - name: wired.100
       type: vlan
       parent: wired
       vlan_id: 100

so, you need to define the parent as well, otherwise, we wouldn't know which interface-name/UUID to set in NetworkManager. In the playbook, when a profile references another profile (either via parent or master), that other profile must also be fully declared in the same playbook. I think that restriction makes sense.

[rpabel]

I think that restriction makes sense.

It does. I personally would prefer to have the same functionality regardless whether it's ansible or nmcli. In my case, the ethernet interface was unmanaged so far, but I found that assigning an ip address on the subnet with mask /32 makes it all work (ansible is happy and routing works). So this is an easy workaround.

[thom311]

Note #26 which reworks vlan / infiniband handling. I think macvlan also should use such a nested setting like bond

[rpabel]

Yes, I've done that and also added the other two (boolean) options: "promiscuous" and "tap". Will send the patch today.

[rpabel]

I think I sent you the pull request last week (I haven't used github before, so I could have done something wrong), did you have a chance to look at it?

[thom311]

I think I sent you the pull request last week (I haven't used github before, so I could have done something wrong), did you have a chance to look at it?

Right. Thanks for the reminder. Replied at #27.

(you can just --force re-push the branch, the pull request will automatically get updated.

[thom311]

Fixed with pull request #27