spec: add extras for adding additional info in result.

[Colstuwjx]

This PR is going to fix #582 .

It was above 4 months since we proposed a solution for adding additional information inside result struct, therefore, I'd like to give a try, propose this PR for pushing this feature online.

Feel free to reject this PR if we have another idea about how to fix this issue.
Thanks.

[mars1024]

This will be helpful in CNI DEL for cleaning something like iptable rules/chains ..., but I'm worrying about whether this is a break change in SPEC, which means libcni and plugins both will need a version upgrade.

[Colstuwjx]

It's an optional field, so I think it should keep the backward compatibility, although the plugins which introduced this field MUST create a new version for it, I think, since it's not easy to check whether a struct has one field in golang.

TBH, I have no idea about this part, maybe those plugins could check the field existence by reflect? Any suggestions?

Besides, the word capabilityArgs looks not good to me, I keep this word just as it's a conclusion from this. But I think it's not intuitive, personally, I'd like to use something like extras.

[bboreham]

I found it surprising that capabilityArgs would come specifically from the IPAM plugin. Is the background to this described somewhere?

[Colstuwjx]

TBH, I'm not sure about this, I just see this comment. Personally, I'd prefer Args. The original intention about adding this optional args is that I want to consume the extra args returned by IPAM plugin result, for now, it doesn't support carry extra args.

[bboreham]

I don't see where either of your links mention IPAM.

[Colstuwjx]

So what's your suggestion? How about make this field not specifically to ipam plugin? I think this field is also useful for those chained plugins.

[bboreham]

Yes, I cannot see any reason to make this specific to IPAM, so I would change the wording so it doesn't say that.

[bboreham]

I too would prefer extras over capabilityArgs.

Maybe extraResults ?

[Colstuwjx]

I have been changed the wording according to @bboreham 's suggestion, personally, I choosed extras, just thought Results in extraResults seems little redundant.

BTW, currently, CNI DEL carries the ADD operation result within prevResult, I wonder whether it is guaranteed. If it did NOT, we may lost the plugin extra results.

[Colstuwjx]

Any updates?

[bboreham]

@Colstuwjx we discussed at length in the maintainers meeting today.

What you've done in the spec looks fine, however I think we should dig a little further into what the libcni code will do.

Three cases come to mind:

1. you want to pass something back to the calling runtime
2. you want to pass something down to a later plugin in the chain.
3. a plugin wants some extra information created in ADD to be available later in DEL

Case 1 just needs the extras field.
Case 2 we can use the capability mechanism to say which fields to pick out.
(I now understand this is what @squeed meant on the original issue, and I missed this earlier)
Case 3 is unclear. Libcni will cache the data (once we've added the field), but do we want to mandate that all runtimes have this feature?

And we should have tests for all of this.

[Colstuwjx]

I couldn't find the capability detail usage in the spec, it just mentioned but even didn't talk the details. Let me talk my understanding:

If the downstream plugin called the IPAM, it would be sth like:

	r, err := ipam.ExecAdd(plugin, stdin)

the downstream plugin could extract extras from the r, a.k.a the ipam result, and it might also extract capabilities from the stdin, which means the runtime args, and it could consume the args, extract the capabilities and do sth, they could also extract the extras in the previous result to do some custom configurations.

The capability is similar to extras, the difference between them is that capability comes from the runtime args, and extras comes from the result, I think both of them are meaningful, since the capability should be the designed feature spec, and extras could carries the real execution result info. With you said, the extras helps the case 1, and capability helps the case 2.

And for the last case, I don't think it's reasonable, we just cache the result while ADD, and it maybe NOT guaranteed, if we suggest this way, plugin developers may abuse this feature. I suggest each plugin should handle DELETE by an idempotent way rather than relying on the cached result, the spec also talked about this part.

[bboreham]

That line in the SPEC tells you to look in CONVENTIONS.md to find out more about capabilities.

[dcbw]

@Colstuwjx The suggestion here is to ensure that any extras information is defined by a runtimeConfig capability as described in CONVENTIONS.md. And that plugins later in the chain would not receive information that it does not declare it supports.

Proposal:

1. Add a runtimeConfig field to the Result object (instead of extras). That field follows the same format and conventions as the runtimeConfig field of the config JSON that the runtime adds. In other words, the keys in this map and their value types should be defined by an item in CONVENTIONS.md
2. The Result runtimeConfig field is merged into the config JSON runtimeConfig field for each plugin in the chain, and filtered as it currently is based on declared plugin capabilities. Fields from the Result override fields sent from the runtime if there is a conflict.
3. The Result runtimeConfig field is also passed back to the runtime itself.

Justification for this approach rather than something similar is to strongly encourage plugins to declare the data they wish to exchange with runtimes and other plugins via CONVENTIONS.md and not have it be a random dumping ground for data.

[Colstuwjx]

That line in the SPEC tells you to look in CONVENTIONS.md to find out more about capabilities.

Okay, it would be better for reading if we add link to that CONVENTIONS.md.

@dcbw , the extras in result, should be different with capabilities in runtimeConfig, it may NOT be one of the well-known capabilities, and just carry some execution result from the plugin, and need to be returned back to the calling runtime.

Justification for this approach rather than something similar is to strongly encourage plugins to declare the data they wish to exchange with runtimes and other plugins via CONVENTIONS.md and not have it be a random dumping ground for data.

I agree with you, but the fact is, some CNI plugins, for example, IPAM plugins, need to pass such extra execution result inside the Result, these cases were not been covered by capability.

I'd like to close this PR, since we didn't make a conclusion about this.