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 InfiniBand (IPoIB) support (LP: #1848471) #283

Merged
merged 5 commits into from
Jun 29, 2022

Conversation

slyon
Copy link
Collaborator

@slyon slyon commented Jun 7, 2022

Description

InfiniBand (IPoIB) devices have a long-form HWADDR, as opposed to the 'normal' 6-byte ethernet HWADDR. When trying to use this HWADDR in a netplan config, it errors out expecting a 6-byte HWADDR. Example:
a0:00:02:20:fe:80:00:00:00:00:00:00:ec:0d:9a:03:00:15:e2:c1

  • We want to adapt netplan's MAC address validation, to accept any 20-byte IPoIB HWADDR.
  • We also want to introduce a new YAML stanza infiniband-mode: datagram|connected to enable IB mode configuration.

Example:

network:
  version: 2
  renderer: networkd
  ethernets:
    ib0:
      match:
        macaddress: "a0:00:02:20:fe:80:00:00:00:00:00:00:ec:0d:9a:03:00:15:e2:c1"
      dhcp4: true
      infiniband-mode: "connected"

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#1848471)

@slyon slyon force-pushed the slyon/infiniband-mac-fr-2119 branch from e0e6113 to e0aae0c Compare June 7, 2022 10:45
@slyon slyon changed the title parse: accept 20 byte InfiniBand MAC addresses (LP: #1848471) Add InfiniBand (IPoIB) support (LP: #1848471) Jun 15, 2022
@slyon slyon added the schema change This PR includes a change to netplan's YAML schema, which needs sign-off label Jun 15, 2022
@slyon slyon force-pushed the slyon/infiniband-mac-fr-2119 branch from 474aa2d to 4fb1990 Compare June 15, 2022 15:54
@slyon slyon requested a review from sparkiegeek June 15, 2022 16:01
@slyon slyon marked this pull request as ready for review June 15, 2022 16:01
@slyon slyon requested a review from schopin-pro June 15, 2022 16:02
Copy link
Contributor

@schopin-pro schopin-pro left a comment

Choose a reason for hiding this comment

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

I only have one holdout about this, which is the use of a generic string in the data model. However, since we already do it elsewhere you can disregard the comment entirely, I won't hold it against you ;-)

src/parse.c Show resolved Hide resolved
src/abi.h Outdated Show resolved Hide resolved
@sparkiegeek
Copy link

I'm not in a position to review this - I don't know C and don't have access to Infiniband

V2: store ib_mode as enum type
@slyon slyon force-pushed the slyon/infiniband-mac-fr-2119 branch from 4fb1990 to 6826cb1 Compare June 20, 2022 11:10
@slyon slyon requested a review from schopin-pro June 20, 2022 11:11
@slyon
Copy link
Collaborator Author

slyon commented Jun 20, 2022

I'm not in a position to review this - I don't know C and don't have access to Infiniband

Alright @sparkiegeek, I just wanted to keep you in the loop as this feature request originated from a MAAS context a while ago.

We already got @schopin-pro's review (thanks for that!) and I adopted the remarks accordingly. Let's try to get the new infiniband-mode stanza approved schema wise, otherwise this PR should be good to be merged!

Copy link
Contributor

@schopin-pro schopin-pro left a comment

Choose a reason for hiding this comment

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

LGTM with the new changes :)

Copy link

@sparkiegeek sparkiegeek left a comment

Choose a reason for hiding this comment

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

As noted - am an interested end-customer but not qualified to review :)

@slyon
Copy link
Collaborator Author

slyon commented Jun 23, 2022

As noted - am an interested end-customer but not qualified to review :)

ACK. We're waiting for schema review of the new infiniband-mode: datagram|connected setting, which is already on Steve's list. Afterwards we are good to merge this.

Copy link

@vorlonofportland vorlonofportland left a comment

Choose a reason for hiding this comment

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

Proposed schema additions look ok, with one question inline (and a minor editorial fix requested).

doc/netplan.md Show resolved Hide resolved
ethernets:
ib0:
match:
macaddress: "11:22:33:44:55:66:77:88:99:00:11:22:33:44:55:66:77:88:99:00"

Choose a reason for hiding this comment

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

Is this the preferred format for representing infiniband mac addresses? It doesn't seem very readable. It does seem to be the format that NM uses, but are there alternatives in common use that we should consider?

Copy link
Collaborator Author

@slyon slyon Jun 27, 2022

Choose a reason for hiding this comment

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

Yes, I couldn't find any other common format for infiniband mac addresses. In general, "XX-XX-XX-XX-XX-XX" or "XXXXXXXXXXXX" are other common mac address formats, but those aren't any more readable if extended to 20 bytes.

It is also the format that has been proposed (and requested) at other places:

@slyon slyon force-pushed the slyon/infiniband-mac-fr-2119 branch from 6826cb1 to e5cde84 Compare June 27, 2022 08:59
@slyon
Copy link
Collaborator Author

slyon commented Jun 27, 2022

Proposed schema additions look ok, with one question inline (and a minor editorial fix requested).

Thanks, I've added the "schema ok" label. Fixed your documentation remark and commented on the MAC address format. This should be good for merging now, if no further remarks are added.

Copy link
Collaborator

@sil2100 sil2100 left a comment

Choose a reason for hiding this comment

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

LGTM!

@slyon
Copy link
Collaborator Author

slyon commented Jun 29, 2022

Merging. I think all comments have been addressed. If we'd want to support an additional format in the future, we can still add this as a follow-up.

@slyon slyon merged commit 8a1ba54 into main Jun 29, 2022
@slyon slyon deleted the slyon/infiniband-mac-fr-2119 branch June 29, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
schema change This PR includes a change to netplan's YAML schema, which needs sign-off schema ok
Projects
None yet
5 participants