Skip to content

Conversation

@hustcat
Copy link
Owner

@hustcat hustcat commented Mar 7, 2017

Move 'VF/MAC/VLAN' from NetConf to NetArgs by following the CNI spec.

@rkamudhan
Copy link
Collaborator

rkamudhan commented Mar 7, 2017

Hi Yin,

Is it something like below?

{
  "name": "sriov-net",
  "type": "sriov",
  "master": "eth1",
  "args": {
    "vf": 1,
    "mac": "66:d8:02:77:aa:aa"
  },
  "ipam": {
    "type": "host-local",
    // ipam specific
    "subnet": "10.1.0.0/16",
    "gateway": "10.1.0.1"
  },
  "dns": {
    "nameservers": [ "10.1.0.1" ]
  }
}

"mac": "66:d8:02:77:aa:aa",
"ipam": {
"type": "host-local",
"type": "fixipam",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason to use fixipam?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No, any IPAM plugin is OK:)

@@ -45,10 +48,8 @@ Given the following network configuration:
"name": "mynet",
"type": "sriov",
"master": "eth1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a example of Args here. So that user can understand it ?

Copy link
Collaborator

@rkamudhan rkamudhan left a comment

Choose a reason for hiding this comment

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

I will test it and let you know asap.

@hustcat
Copy link
Owner Author

hustcat commented Mar 7, 2017

@rkamudhan By CNI spec:
Maybe we use master as args field in network configuration, and use VFMAC etc. as args field in Parameters that for simple configuration on a per-container basis.

However, this PR don't include using master as args field in network configuration. I will add it later:)

@hustcat
Copy link
Owner Author

hustcat commented Mar 7, 2017

@rkamudhan I am waiting your testing reply :)

@rkamudhan
Copy link
Collaborator

@hustcat
I think, it is better "master" field not added in the args field, I see args as the parameter, which should be optional, without args parameter also SRIOV plugins should work. But without master and ipam, Currently, SRIOV plugin can't work. Please join in our slack https://intel-corp.herokuapp.com/, I like to discuss with you about our forked version of sriov https://github.com/Intel-Corp/sriov-cni, we added so many features to your SRIOV plugin.

@hustcat
Copy link
Owner Author

hustcat commented Mar 13, 2017

@rkamudhan The features you added are so amazing, I'd like to merged it if you like. CNI spec allow plugin added its own optional fields for network configuration, so add master field is OK.

But I still wonder how these optional fields can be integrated with Kubernetes. see 1, 2.

Mapping Network Objects to CNI
When using CNI networking, the kubernetes CNI plugin would search for a file in /etc/cni/net.d with a JSON “name” key that matched the Network object name. It would then execute this CNI config only. When the “ordered chaining” (#346) PR gets merged, this would also search for a “.conflist” file that contains a sequence of plugins to run.

IPAM would be handled by the CNI config JSON just like it currently is.

In addition, I'd like to merge this PR if you don't have other opinions.

@rkamudhan
Copy link
Collaborator

rkamudhan commented Mar 13, 2017

@hustcat , I think CNI Args will be a part of pod spec in kubernetes, which are optional. The .conflist file work similar to the Multus plugin

@hustcat
Copy link
Owner Author

hustcat commented Mar 14, 2017

@rkamudhan Thanks, I‘ll merged this PR now. After this, let's merge your DPDK and other features:)

@hustcat hustcat merged commit caed171 into master Mar 14, 2017
@hustcat hustcat deleted the devel branch May 5, 2017 03:09
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