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

Support InfiniBand GUID in the Network annotation #4

Open
Mmduh-483 opened this issue Dec 17, 2019 · 7 comments
Open

Support InfiniBand GUID in the Network annotation #4

Mmduh-483 opened this issue Dec 17, 2019 · 7 comments

Comments

@Mmduh-483
Copy link

InfiniBand is using GUID as hardware address instead of mac, supporting GUID in the k8s.v1.cni.cncf.io/networks structure can play major role at the applications that use InfiniBand.

@adrianchiris
Copy link

adrianchiris commented Jan 2, 2020

looking at [1] i see:

4.1.2.1.4 "mac"
This optional key with value type string requires the plugin handling this network attachment to assign the given MAC address to the pod. This key’s value must contain a valid 6-byte Ethernet MAC address or a valid 20-byte IP-over-InfiniBand Hardware address (as described in RFC4391 section 9.1.1). If the value is invalid, the Network Attachment Selection Annotation shall be invalid and ignored by the implementation.

We can either extend this to support a 64 bit GUID or have GUID as a first class citizen here (possibly mutually exclusive with mac)

also im not sure if you can actually set the IPoIB 20-bytes address
according to section 9.1.1 in the RFC mentioned:

  • bytes 0-3 bytes are flags and queue pair number which user cannot control
  • bytes 4-19 GID which is composed as follows (can be found in IBTA Infiniband Specification):
    • 8 bytes subnet prefix - set by the infiniband subnet manager (initial value is 0xfe80000000000000)
    • 8 bytes GUID - the element which is user configurable

@s1061123
Copy link
Member

Hi @adrianchiris,

Thank you for the PR!
I already taked at the NPWG meeting, but let me summarize it.

I am going to discuss about it from implementation point of view. The mac field implementation in multus is done by following:

  • CNI Plugin (i.e. tuning plugin) to support to change MAC with capabilities/runtimeConfig support, defined in CNI CONVENTIONS.md.
  • multus supports 'mac' field in pod annotation

When the multus get 'mac' filed in pod annotation, multus just passed it to delegating plugin as capabilities/runtimeConfig.

User defined CNI Config file in net-attach-def:

{
  "cniVersion": "0.3.1",
  "plugins": [
    {
      "type": "macvlan",
      (snip)
    },
    {
      "capabilities": {
        "mac": true
      },
      "type": "tuning"
    }
  ]
}

After multus parsing with 'mac' fields, actual delegating CNI plugin will get following config:

{
  "cniVersion": "0.3.1",
  "plugins": [
    {
      "type": "macvlan",
      (snip)
    },
    {
      "capabilities": {
        "mac": true
      },
      "type": "tuning",
      "runtimeConfig": {
        "mac": <user provided mac in 'mac' annotation>
      } 
    }
  ]
}

'runtimeConfig' is injected by multus as capabilities flag.
Hence to support guid as mac, need to do following:

  • Implement CNI plugin to support guid, in somewhere
  • Add guid in well-known capability [CONVENTIONS.md] (if possible)
  • Add meta plugin implementation (e.g. multus) to support guid annotations

As alternative option, add 'guid' option can be implemented as CNI's args as following. In this case, you can change guid from pod annotation without any spec change.

---
apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
  name: infiniband-test
spec: 
  config: '{
            "cniVersion": "0.3.1",
            "plugins": [
                {
		    // snip: Inifiband CNI config
                    // assume this pluggin supports guid as args
		    // (see: https://github.com/containernetworking/cni/blob/master/CONVENTIONS.md)
                } ]
        }'
---
apiVersion: v1
kind: Pod
metadata:
  name: infiniband-pod-1
  annotations:
    k8s.v1.cni.cncf.io/networks: '[
            { "name": "infiniband-test",
              "cni-args": {
                  "guid": <your expected guid>
              }
    ]'
spec:
  containers:
  // snip

I suppose you could implement it in phased approach like that:

  1. Implement guid as CNI args first
  2. Based on above experiment, standardize it in spec

@adrianchiris
Copy link

adrianchiris commented Jan 17, 2020

@s1061123 thanks for providing inputs !
i still need to catch up on the interaction of multus with delegate plugins
and capabilities/runtimeConfig vs other cni args.
id like to avoid overlapping (e.g have guid defined once as a cni extra arg and another as runtimeConfig).

also part of the goal is to have this a part of CNI and net crd spec.

@adrianchiris
Copy link

adrianchiris commented Jan 20, 2020

Issue opened in containernetworking/cni
to support GUID as capabilities/runtime config.

if and when this gets approved then we can proceed with this issue.

@moshe010
Copy link

  1. This PR Add GUID to well known Capabilities containernetworking/cni#742 that add the GUID to the Convention.rst
  2. We have https://github.com/Mellanox/ib-sriov-cni CNI which will use the GUID
  3. Once the first PR will approve we will push multus the changes to support GUID

@adrianchiris
Copy link

adrianchiris commented Apr 12, 2020

as containernetworking/cni#742 was merged, proposed updates to multi-net-spec can be found in the following google doc

adrianchiris added a commit to adrianchiris/network-attachment-definition-client that referenced this issue Apr 27, 2020
This commit defines `infiniband-guid` as a new network attachment
attribute as discussed in issue[1].

This attribute is defined be converted to `infinibandGUID` CNI runtime
config by delegate plugin.

[1] k8snetworkplumbingwg/multi-net-spec#4
@adrianchiris
Copy link

This change was approved on NPWG meeting on 04.06.2020

Is there a periodic update of the spec ? as changes are not proposed directly to the source file.

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

No branches or pull requests

4 participants