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

[Feature request]Include an additional field in Result struct, analogous to args in the config struct. #582

Open
amshinde opened this issue Aug 25, 2018 · 21 comments
Milestone

Comments

@amshinde
Copy link

This will allow plugins to return arbitrary data that is specific to the plugin. I am currently looking at a OVS-DPDK plugin, that would create vhost-user sockets for virtualized containers. For this to work, the runtime should be able to get the path of the socket created by the plugin. To accommodate such use-cases, it will be useful to create an additional field+interface for the plugin to return additional data.

This has been discussed previously here:
#513 (comment)

@amshinde
Copy link
Author

cc @rosenhouse

@amshinde amshinde changed the title Include an additional field in Result struct, analogous to args in the config struct. [Feature request]Include an additional field in Result struct, analogous to args in the config struct. Aug 28, 2018
@amshinde
Copy link
Author

amshinde commented Sep 5, 2018

Any feedback on this? I am willing to work on this if the maintainers agree on this feature.

@jellonek
Copy link
Member

@dcbw @squeed ping. WDYT about that?

@amshinde
Copy link
Author

Any feedback on this @rosenhouse @dcbw @squeed ?

@squeed
Copy link
Member

squeed commented Sep 25, 2018

In general, we've been reluctant to consider this in the past, since it decreases the value of CNI as a specification. If there's an attribute that it makes sense to export, then it should be part of the spec.

However, this sort of rigid viewpoint is easy to dictate from up on high.

What sort of attributes were you thinking?

@amshinde
Copy link
Author

@squeed I am currently looking at a CNI plugin that uses Open vSwitch. This plugin would return a vhost-user interface for use with a hypervisor-based containers. The vhost-user socket is a path on the file-system, that will be used as a network interface by the container.

So, essentially, the plugin needs to send this path as an additional attribute vhost_user_path.

@dcbw
Copy link
Member

dcbw commented Sep 26, 2018

@amshinde what would the runtime do with the vhost-user socket, and how would it pass that to the workload to enable the workload to load the DPDK libraries and start passing traffic on the VF?

@amshinde
Copy link
Author

@dcbw The runtime would check for the vhost-user-path attribute and pass this socket to our hypervisor based container (as a backend for virtio-net) The container would then see this as just another network interface. The socket just acts as shared memory based interface.
The container workload does not need to be customized in any way. The loading of the the DPDK libraries on the host is upto the CNI plugin itself.

@kmala
Copy link

kmala commented Sep 13, 2019

Any update on this? This feature will be help especially in the scenario's of chained plugins where one plugin wan't to pass some data to the next one.

@squeed
Copy link
Member

squeed commented Oct 2, 2019

I have a plan for this: allow plugins to inject additional capabilityArgs to plugins down the chain. Thoughts? I'm going to get someone to write it up shortly.

@squeed
Copy link
Member

squeed commented Oct 9, 2019

@dougbtv has agreed to take some time and prepare a SPEC.md PR for this (in due time).

@oilbeater
Copy link
Contributor

Any update on this? We are still struggling to find a way to pass extra data from one plugin to another.

@dcbw
Copy link
Member

dcbw commented Feb 12, 2020

@oilbeater @Colstuwjx is the main use-case still vhost-user? Can you elaborate on the control flow here? eg plugin A adds the vhost-user-path + data to capabilityArgs and then what plugin consumes that item later in the chain? Or is it just to preserve the path in the cached result for DEL? Or is it just to return some random data to the runtime that would use it somehow?

@Colstuwjx
Copy link
Contributor

It's similar to this, but not the same one.

In my case, I just have two plugins as a chain, and I want the upstream plugin could return a result with some extra vars, if the extra vars are set, the downstream plugin should do some tasks, such as setting up the firewall rules and network policies, but it's optional, if no extra vars passed, the downstream plugin may set the defaults.

@oilbeater
Copy link
Contributor

In my case, we want to build a cluster wide IPAM to different CNI and the IPAM can manage some plugin specific options like vlanID, paraentNic, deviceID etc. In that way, the main plugin (maybe a modified vlan, macvlan or host-device) can use the extra options to config the nic. Otherwise the main plugins have to use a static config or request another configserver to get the options.

@bboreham
Copy link
Contributor

I'd like to know why it is called capabilityArgs. Does it relate to "capabilities" described in CONVENTIONS.md?
And why "args" when it is part of the result?

@Colstuwjx
Copy link
Contributor

Yes, I also don't like the word capabilityArgs, maybe something like Args should be better. For me and @oilbeater , both of cases are, we could NOT pass extra args inside IPAM plugin result, since its type didn't support carry extra args, that's the reason for adding this "args".

Thanks.

@mars1024
Copy link
Member

I have to say that Result is just result, even some data inside Result will be used in other plugins, but it should not be called as args.

What we are talking is actually the extendablility of Result, should it carry user-defined datas, just like metadata.annotations in Pod.

@amshinde
Copy link
Author

Agree with @mars1024. Its been a while since I created this feature request, but what I had in mind was the ability for a plugin to return additional set of data in the Result structure, essentially being able to extend the Result structure.

@Colstuwjx
Copy link
Contributor

Yes, whatever the terminology is, we just want to have this extendablility .

@MikeZappa87
Copy link
Contributor

@amshinde hello we were just talking about this as apart of #1050 does this help?

@squeed squeed added this to the CNI v1.2 milestone Jun 10, 2024
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 a pull request may close this issue.

10 participants