Create new Source Based Routing plugin #212
Conversation
|
I have updated the description to reflect tidy up behaviour, and removed the rogue binaries. Code is still pending tests, and adding of deletion tidy up (I'm going to restructure slightly to make tidy up easier, so doubly not worth detailed code review yet, though I think the README description is correct). |
plwhite
changed the title
|
I think this is now a decent candidate for a merge. I should squash commits before doing so, but deliberately haven't done that yet so you can see what's new. |
plugins/meta/sbr/README.md
Outdated
| { "type": "flannel" }, | ||
| { | ||
| "type": "sbr", | ||
| "logfile": "/var/log/sbr.log" |
There was a problem hiding this comment.
Generally, plugins just log to stderr.
There was a problem hiding this comment.
True, but I'd like to appeal on this one. By default the plugin just logs to stderr, but can be configured to log to file. I actually found the option of setting that debug option was very useful when testing.
Hence I can remove the logging to file if you want, but it felt like a good thing to have available when I needed it. What do you think?
There was a problem hiding this comment.
@plwhite we don't have any general objection to logging elsewhere than stderr, but we believe plugins should always log to stderr. Any other logging should be additional to stderr. But we also want to figure out a "standard" way to tell plugins what level to log at and what the additional method to use, since we don't think that should be different for every plugin.
(eg, you would just want to tell Kubernetes or CRIO or Mesos to "turn on CNI debugging" and it would work for all plugins in a chain)
There was a problem hiding this comment.
@plwhite so for now, if it's OK, we'd like to remove the logging and add it back later when we have a better plan for logging in general.
There was a problem hiding this comment.
Having thought this over, I agree that every plugin logging in its own way is insane, and strictly it's way more sensible that K8s should be trapping stderr and saving it somewhere.
So fair enough, I have removed it in my latest commit.
|
@dcbw Do you reckon this is ready for merge? It obviously needs commits squashed down to one, but I won't do that until we are ready. |
|
@plwhite could you 'git rm plugins/meta/sbr/cnitest-c18c58aa-d341-2759-1fcd-2bbc1d1f2d01' and update? looks like that leftover test output snuck in or something. |
There was a problem hiding this comment.
There's also a spurious file https://github.com/containernetworking/plugins/pull/212/files#diff-462604326ddd9332b797c273d9b07fce
|
I think I fixed everything required - happy to squash all the commits ready for merge now if you reckon it's ready to go? |
|
Seems good to me. If later users file an issue asking for logging to be a bit less chatty, I would agree with them - but not enough to stop this PR now. LGTM pending squash. |
This creates a new plugin (sbr) which sets up source based routing, for use as a chained plugin for multi-network environments.
|
I've squashed commits, ready for merge. |
This creates a new plugin (sbr) which sets up source based routing, for use
as a chained plugin for multi-network environments.
Note to maintainers - this is now updated with tests etc. so should be ready to merge. I think I have addressed all comments so far.