-
Notifications
You must be signed in to change notification settings - Fork 275
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
Moved docker behind a new runtime interface #416
Conversation
thanks @networkop I wonder how you imagine the runtime selection/sensing needs to be handled? |
@hellt there's a new CLI argument |
latest commit was tested on docker-ce 19.03.15. |
clab/clab.go
Outdated
} | ||
// init docker network mtu | ||
if c.Config.Mgmt.MTU == "" { | ||
m, err := getDefaultDockerMTU() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likely this func needs to be runtime-scoped? i.e. with containerd/podman the logic should be runtime-specific as docker0
interface won't be there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so should we move this whole function under the runtime interface? or do you want to just skip MTU here and set it in the runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh, I don't fully understand the need to initMgmtNetwork. why can't we just defer to the runtime to set it all up (after all it's managing it's own IPAM). maybe you can elaborate @hellt ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can start with a small change of adding a new runtime-specific function named DefaultMgmtNetMTU
which will return the default management network MTU using the runtime-specific methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@networkop the initMgmtNetwork
was created to set the default configuration values for mgmtNetName (clab) and default subnets for ipv4/6
It might be that this can be removed/changed with the default values to be passed over to the respective runtime func that creates the network
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at this stage, i think the only right solution (I think) is to leave MTU unchanged and do a check (if empty, then lookup) inside the CreateNet
function, which is the only place where MTU is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the runtime name is always provided via rt
value, and has a docker
default, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, there's always a runtime set. The problem is that runtime is currently optional and most of the config_test.go
functions are set up just WithTopoFile
. It's not a problem adding a WithRuntime
to them as well, it'll work, but it is an indication of leaking dependencies -- your config test code should not really rely on a runtime being set.
But if you're ok with this, I'm happy to add runtime to every test in config_test.go
, so they'll all look like:
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
opts := []ClabOption{
WithTopoFile(tc.got),
+ WithRuntime(...),
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should defer this till we sort the other things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another issue with getting default MTU is determining which bridge to use. Some runtimes may defer to CNI plugins to setup the networking, which means we'd have to parse all CNI configuration files in order to extract the name of the bridge, and it may not even exist in case of a P2P CNI plugin. So maybe we should just have the magic getDefaultMTU
function that picks the lowest MTU off several well-known bridge names, and, if none found, falls back to some safe default, e.g. 1400. wdyt @hellt
@networkop can we address this deprecation issue as well? https://github.com/srl-labs/containerlab/pull/416/checks?check_run_id=2545109898#step:4:26 |
This is a transitive dependency, I'll just add this to go.mod, i think that should fix it?
|
This is some quick prototyping I've done to kick-start the discussion raised in #415. Most of the code logic is still the same, the only difference is all docker api interactions are hidden behind a generic container runtime interface.
There are 3 new folders:
./runtime
- contains interface definition and docker runtime./types
- generic types that are required by both clab and runtimes (moved some types fromconfig.go
to avoid circular imports)./utils
- again some generic util functionsHave a look, let me know what you think 😄