[WIP] Add initial utilities for link and namespace actions #5
Conversation
|
This is a first pass, with a lot of shameless copy/paste. Looking mostly for comments on the approach in general at this point. |
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
There was a problem hiding this comment.
A lot of shameless copy/paste in this file, and a lot of unused code here too.
There was a problem hiding this comment.
Don’t forget to carry forward the original copyright in addition to the SUSE copyright for copied code
|
@rktidwell I've done something similar already for the SRIOV PR https://github.com/networkservicemesh/networkservicemesh/blob/27ba3cffa9f809d1481d975d904b52dd6008258d/forwarder/sriov-forwarder/pkg/sriovforwarder/link.go There's also an attempt to provide a bit of the idempotency there, for instance, trying to assign the same IP address probably shouldn't cause an error or bringing down a link that's already down shouldn't throw any errors. Beyond that (based on my experiences from other projects) it's usually good to have an interface defined for link operations that wrap netlink library around, so that you can mock/fake/stub it later on for the unit testing purposes, as this may often be a pain to have correct permissions, resources, kernel dependencies in the CI env for example. |
| cDISCONNECT = false | ||
| ) | ||
|
|
||
| type connectionConfig struct { |
There was a problem hiding this comment.
Why define a new struct here rather than simply using the stuff already in the Connection struct from the api?
There was a problem hiding this comment.
I saw this laying around, really a relic of copy/paste just to get some momentum built. I have to confess I'm not familiar with the API's available to me, I'm learning this as I go. Keep pointing out places where there are structs and things that can be re-used.
There was a problem hiding this comment.
All good... one of the things I am seeking to encourage is to keep things simple :) If this isn't buying you anything, might be a good idea to simplify it out :)
| } | ||
|
|
||
| // newVETH returns a VETH interface instance | ||
| func NewVETH(srcName, dstName string) *netlink.Veth { |
There was a problem hiding this comment.
Why do this with a func rather than just declaring the &netling.Veth inline where folks can see how its filled in?
There was a problem hiding this comment.
Just a relic of copy/paste, learning the netlink API, etc.
To be honest, I am finding that I don't like how a lot of the code I've come across wraps netlink, I agree it would be nicer to not bury so much of this.
There was a problem hiding this comment.
@rktidwell With the caveat that everything is a balance, I tend to like simpler things that are more compatible with human cognition. So the way I would personally (note: not claiming only right way to think here) evaluate whether or not to have a function here would be: Does it encapsulate some complexity that would otherwise break up the train of thought of the person reading the code that calls it or does it simply require the human to change context from the place its called to where it's defined to know for sure what it's doing. For me, those considerations would argue for not having it as a function.
Thanks for the pointer. I think idempotency is very important here, and it's something I'm trying to pay close attention to. I've been thinking about how to test this, because as you point out having the correct permissions in place during execution is very painful. Test cases that assert this are key, and I definitely plan on developing them. I'm struggling with the value of mocking calls into netlink for unit testing purposes, at the moment I'm not envisioning the complexity of these utilities being great enough for unit testing with mocks to add value. It may be that we have to deal with some of this complexity, I'm really of the opinion that end-to-end tests are going to add value and that heavy use of mocking is simply generating coverage for the sake of having coverage. |
This change introduces basic utilities for managing interfaces, routes, and namespaces. See networkservicemesh#2 Signed-off-by: Ryan Tidwell <[email protected]>
This change introduces basic utilities for managing interfaces,
routes, and namespaces. See
#2
Signed-off-by: Ryan Tidwell [email protected]