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

Implement --keep-mgmt-net for destroy subcmd #462

Merged
merged 14 commits into from
Jul 8, 2021
Merged

Implement --keep-mgmt-net for destroy subcmd #462

merged 14 commits into from
Jul 8, 2021

Conversation

steiler
Copy link
Collaborator

@steiler steiler commented Jun 18, 2021

Implementing #271.

This is based upon changes introduced in #460.

At the same time it adds an implementation of the DeleteNet() for containerd.
Here we are listing all the Host interfaces to see if the bridge is still the master to any existing device, if not, and not --keep-mgmt-net is set, the deletion of the cni based mgmt bridge is executed.

cmd/destroy.go Show resolved Hide resolved
runtime/docker/docker.go Outdated Show resolved Hide resolved
clab/clab.go Outdated Show resolved Hide resolved
cmd/destroy.go Outdated Show resolved Hide resolved
cmd/destroy.go Outdated Show resolved Hide resolved
@steiler
Copy link
Collaborator Author

steiler commented Jun 21, 2021

Alright then, I'd consider this done now.

@steiler
Copy link
Collaborator Author

steiler commented Jun 30, 2021

Resolved merge conflicts.
We're ready to merge here, aren't we?

utils/netlink.go Outdated Show resolved Hide resolved
@hellt
Copy link
Member

hellt commented Jun 30, 2021

We're ready to merge here, aren't we?

I think it is time to add a test for this case

@steiler
Copy link
Collaborator Author

steiler commented Jul 1, 2021

@hellt are you fine with the tests

@hellt
Copy link
Member

hellt commented Jul 1, 2021

@steiler can you please also add a doc entry about this flag of destroy command?

@hellt
Copy link
Member

hellt commented Jul 1, 2021

@steiler also I think you wanted to add the schema entry for bridge under mgmt?

@steiler
Copy link
Collaborator Author

steiler commented Jul 6, 2021

@hellt That is not related to this flag, so it should go into a new PR.

@steiler steiler mentioned this pull request Jul 8, 2021
@steiler
Copy link
Collaborator Author

steiler commented Jul 8, 2021

Since we've gone downa different path now, with karims work on refactoring the runtime interface, should the withKeepMgmtNet() func now be replaced by jsut the field in the withConfig piece?
https://github.com/srl-labs/containerlab/pull/462/files#diff-bb8ad382f59edfae9297e446895a161036f39115189e67da9d9f377a684a8f82R28

@hellt
Copy link
Member

hellt commented Jul 8, 2021

@steiler

Since we've gone downa different path now, with karims work on refactoring the runtime interface, should the withKeepMgmtNet() func now be replaced by jsut the field in the withConfig piece?

can you create a commit with that approach? Looks more clean to me

@steiler
Copy link
Collaborator Author

steiler commented Jul 8, 2021

As discussed, we go with this approach for now and tackle a potential change in a different PR.

@hellt hellt merged commit fa9c2da into master Jul 8, 2021
@hellt hellt deleted the keepmgmtnet branch July 8, 2021 11:15
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 this pull request may close these issues.

None yet

4 participants