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

Introduce bridge device plugin #4

Merged
merged 3 commits into from Feb 19, 2018

Conversation

Projects
None yet
3 participants
@phoracek
Member

phoracek commented Feb 16, 2018

No description provided.

@fabiand

Looks like a great start

hostPID: true
containers:
- name: device-plugin-network-bridge
image: phoracek/device-plugin-network-bridge:latest

This comment has been minimized.

@fabiand

fabiand Feb 16, 2018

Member

probably kubevirt/…

This comment has been minimized.

@phoracek

phoracek Feb 16, 2018

Member

i had problems pushing built images to my test kubernetes environment so i chose to be lazy and used docker repo instead. will fix it

- name: device-plugin-network-bridge
image: phoracek/device-plugin-network-bridge:latest
securityContext:
privileged: true

This comment has been minimized.

@fabiand

fabiand Feb 16, 2018

Member

I wonder if fine tuning this with caps will be enough, fine for now.

This comment has been minimized.

@phoracek

phoracek Feb 16, 2018

Member

The biggest security flaw is IMO that we need to mount /var/run for docker.sock, but we can get a rid of that with the netns hack.

value: "br0"
volumeMounts:
- name: var-run
mountPath: /var/run

This comment has been minimized.

@fabiand

fabiand Feb 16, 2018

Member

Why is this needed from the host?

This comment has been minimized.

@phoracek

phoracek Feb 18, 2018

Member

For docker.sock. I use docker to find out which container has just been crated.

@fabiand

This comment has been minimized.

Member

fabiand commented Feb 16, 2018

/cc @vladikr

@phoracek

This comment has been minimized.

Member

phoracek commented Feb 16, 2018

I added some documentation and daemon set deployment option.

@phoracek

This comment has been minimized.

Member

phoracek commented Feb 17, 2018

I need to update vendoring, please merge #5 first.

@phoracek phoracek self-assigned this Feb 17, 2018

@@ -17,6 +17,8 @@ Each plugin may have it's own build instructions in the linked README.md.
* [VFIO](docs/vfio/README.md)
* [KVM](docs/kvm/README.md)
* Network

This comment has been minimized.

@mpolednik

mpolednik Feb 19, 2018

Contributor

What about just calling the plugin network-bridge? How many more plugins do you think we could have for network/ package?

This comment has been minimized.

@fabiand

fabiand Feb 19, 2018

Member

bridge, ipv6, and maybe an overlay type - that#s about the scope I'd like to play with.

This comment has been minimized.

@phoracek

phoracek Feb 19, 2018

Member

In pkg/ it makes more sense to keep it structured like that. There will be some shared network modules.

I'd like to keep it symmetric -- the same dir structure in pkg, doc and cmd.

bridgesList := strings.Split(bridgesListRaw, ",")
manager := dpm.NewDevicePluginManager(bridge.BridgeLister{Bridges: bridgesList})

This comment has been minimized.

@mpolednik

mpolednik Feb 19, 2018

Contributor

This looks like reversed-reversed responsibility: it is the lister that should be able to figure out the default bridges (but the env var check is in the right place).

This comment has been minimized.

@phoracek
@@ -0,0 +1,41 @@
# Network Bridge Device Plugin
This device plugin allows user to attach pods to bridges running on nodes.

This comment has been minimized.

@mpolednik

mpolednik Feb 19, 2018

Contributor

Aren't we assigning bridges to pods?

This comment has been minimized.

@phoracek

phoracek Feb 19, 2018

Member

Multiple pods can share one bridge. We attach pod (via veth pair) to a bridge.

)
const (
nicsPoolSize = 100

This comment has been minimized.

@mpolednik

mpolednik Feb 19, 2018

Contributor

Care to comment the default choice, even if arbitrary? (in the code!)

type NetworkBridgeDevicePlugin struct {
dpm.DevicePlugin
bridge string
attachmentCh chan *Attachment

This comment has been minimized.

@mpolednik

mpolednik Feb 19, 2018

Contributor

I wonder if bridges use attachment terminology - at least in VFIO world, we've came to conclusion that assignment is tad better match.

This comment has been minimized.

@phoracek
letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
)
func randIfaceName() string {

This comment has been minimized.

@mpolednik

mpolednik Feb 19, 2018

Contributor

Nit: randInterfaceName (no reason to shorten Interface to Iface).

This comment has been minimized.

@phoracek
return nil
}
const (

This comment has been minimized.

@mpolednik

mpolednik Feb 19, 2018

Contributor

Please move constants to the top of the module.

This comment has been minimized.

@phoracek
}
}
type Attachment struct {

This comment has been minimized.

@mpolednik

mpolednik Feb 19, 2018

Contributor

Move this to the top of the module.

This comment has been minimized.

@phoracek
return fmt.Sprintf("/tmp/device-plugin-network-bridge/%s/%s", bridge, nic)
}
func (nbdp *NetworkBridgeDevicePlugin) attachBridges() {

This comment has been minimized.

@mpolednik

mpolednik Feb 19, 2018

Contributor

This does seem to create a sort of "side channel" to the typical device plugins flow - the "attachment" should be part of allocate call, not done separately. Is there anything missing in the plugin API to do that?

This comment has been minimized.

@phoracek

phoracek Feb 19, 2018

Member

Pod does not exist yet when Allocate is called. Because of that we are not able to handle Pod's networking directly. Instead we mark it with mounted device and then, once it is created, handle in another thread.

phoracek added some commits Jan 30, 2018

network: bridge: introduce device plugin
Signed-off-by: Petr Horacek <phoracek@redhat.com>
network: bridge: daemon set and documentation
Signed-off-by: Petr Horacek <phoracek@redhat.com>

@phoracek phoracek merged commit 4c759a7 into kubevirt:master Feb 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment