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

decouple container creation and container details retrieval #80

Merged
merged 16 commits into from
Oct 5, 2020

Conversation

karimra
Copy link
Member

@karimra karimra commented Oct 4, 2020

This PR decouples containers creation/starting and container inspect to get the IPs and PID.
This allows to run the creation and starting functions concurrently.

This PR also rewrites the hosts file generation function, as well as moving the inspect cmd table output to a separate function to allow its reuse in deploy command.

Got rid of a few log.Fatal as well, replaced by returning an error on the cobra command level

@karimra karimra requested a review from hellt October 4, 2020 05:43
@karimra
Copy link
Member Author

karimra commented Oct 4, 2020

@hellt I will add the "/" trimming to this PR after the other PR is merged.

@hellt
Copy link
Member

hellt commented Oct 4, 2020

@karimra in a615950 I added a condition to create symlinks only if links are present

links section is absent when we want to create a node with just a management interface

@karimra
Copy link
Member Author

karimra commented Oct 4, 2020

Creating the symlink doesn't hurt, it just creates a network namespace with the docker container name.

In the future we can add a connect command that creates a link between 2 already existing docker containers.

@hellt
Copy link
Member

hellt commented Oct 4, 2020

I conditioned this func, which creates a symlink https://github.com/srl-wim/container-lab/blob/a615950fe4de0b846e53ad8c30bdae610985894c/clab/docker.go#L339
I dont think its ok to create a symlink in case no links are needed, because the destroy command then doesn't clean it up (since no c.Links is present in the topo)

cmd/deploy.go Show resolved Hide resolved
@hellt
Copy link
Member

hellt commented Oct 4, 2020

LGTM

@karimra
Copy link
Member Author

karimra commented Oct 4, 2020

I still think that change in cLab.CreateContainer is not right, it checks if there are links in the topology to create a symlink to the container PID, not if that specific container has link or not.
If a topology has both containers with and without links, those without links will get their symlink created ( because len(c.Links >0) )but never deleted because no link ( in c.Links ) references them.

What you are trying to fix here is better fixed in cLab.DeleteVirtualWiring, not during the container creation.

It's better to create the symlink whether links are needed or not, and fix the destroy procedure.

@hellt
Copy link
Member

hellt commented Oct 4, 2020

reverted the commit
is it ok to merge the PR from your pov?

@karimra
Copy link
Member Author

karimra commented Oct 5, 2020

Should be ok now, the destroy behavior can be resolved in another PR

@hellt hellt merged commit 590d1f7 into master Oct 5, 2020
@karimra karimra deleted the docker-create-conc branch October 19, 2020 07:16
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.

2 participants