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

Adding support for Cumulus with Ignite #482

Merged
merged 20 commits into from
Jul 6, 2021
Merged

Adding support for Cumulus with Ignite #482

merged 20 commits into from
Jul 6, 2021

Conversation

networkop
Copy link
Contributor

@networkop networkop commented Jun 29, 2021

I think the title pretty much sums it up but since the PR is quite big, I'll try to break it down into multiple bullet points to better explain everything that has been affected:

  • The main addition is the new runtime and node kind -- these are completely new files and don't change any of the existing interfaces or types
  • Ability to specify runtime per node -- all nodes structs now have a private runtime field. This field is set during node.Init and defaults to the global runtime (as configured by the --runtime flag). However for cvx it defaults to a pre-defined ignite runtime with the ability to fall back to any other supported runtime via the NodeConfig.Runtime.
  • As a result of the above, all of the calls to runtime (e.g. ListContainers) are now executed via a proxy functions defined for CLab, e.g. it is now c.ListContainers(ctx, labels) instead of c.Runtime(ctx, labels). CLab always knows about all of the configured runtimes (since it knows about all of the nodes).
  • Topology parsing is moved to CLab.Init function -- this way it's safer to reason about CLab since we know that it'll always have Nodes and Runtimes parsed and initialized.

I think overall the last 3 points help enforce the separation between different packages and prevent direct calls between, e.g. cmd and runtime. It's much easier to read and troubleshoot code that relies on p2p interaction instead, i.e. cmd <-> clab <-> node <-> runtime.

AFAICT, none of the existing behavior has been affected, I've tried to make sure of that but happy to fix if there anything is off.

clab/config.go Outdated Show resolved Hide resolved
@networkop networkop marked this pull request as ready for review July 5, 2021 13:19
@networkop networkop requested a review from hellt July 5, 2021 13:19
@hellt hellt requested a review from karimra July 5, 2021 13:27
clab/clab.go Outdated Show resolved Hide resolved
cmd/deploy.go Outdated Show resolved Hide resolved
cmd/deploy.go Outdated Show resolved Hide resolved
cmd/destroy.go Outdated Show resolved Hide resolved
cmd/graph.go Outdated Show resolved Hide resolved
runtime/ignite/iginite.go Outdated Show resolved Hide resolved
runtime/ignite/iginite.go Outdated Show resolved Hide resolved
clab/clab.go Outdated Show resolved Hide resolved
clab/config.go Outdated Show resolved Hide resolved
clab/config.go Show resolved Hide resolved
@karimra
Copy link
Member

karimra commented Jul 5, 2021

@networkop , did a first review, let me know if anything doesn't make sense to you.

If I got it right, cumulus uses ignite as a default runtime, but it supports docker ?
This could be confusing for users who will have to set the runtime explicitly to docker just for cvx nodes.

@networkop
Copy link
Contributor Author

thanks @karima, will get the changes in shortly.

regarding the default behavior, with ignite you get 100% of cumulus features, which is why I made it a default behaviour. Ultimately, from user's pov it shouldn't be noticeable what runtime is used under the hood, they would still be able to interact with it the same exact way.

types/types.go Outdated Show resolved Hide resolved
@networkop
Copy link
Contributor Author

@karimra I've done multiple changes to address your comments. let me know what you think

@networkop networkop requested review from karimra and hellt July 6, 2021 06:45
clab/config.go Outdated Show resolved Hide resolved
cmd/deploy.go Outdated Show resolved Hide resolved
cmd/deploy.go Outdated Show resolved Hide resolved
cmd/deploy.go Outdated Show resolved Hide resolved
nodes/cvx/cvx.go Outdated Show resolved Hide resolved
nodes/cvx/cvx.go Outdated Show resolved Hide resolved
cmd/destroy.go Outdated Show resolved Hide resolved
cmd/disableTxOffload.go Outdated Show resolved Hide resolved
nodes/crpd/crpd.go Outdated Show resolved Hide resolved
nodes/host/host.go Outdated Show resolved Hide resolved
nodes/ovs/ovs.go Outdated Show resolved Hide resolved
@networkop networkop requested review from hellt and karimra July 6, 2021 13:32
@networkop
Copy link
Contributor Author

@karimra @hellt all fixes have been pushed.

@karimra
Copy link
Member

karimra commented Jul 6, 2021

@networkop thanks for addressing all my comments, lgtm.

@networkop
Copy link
Contributor Author

thanks @karimra . Can you check the latest commit? I think it would make sure runtimes are initialized only once.

@karimra
Copy link
Member

karimra commented Jul 6, 2021

Yes, even better, a single runtime passed to the node Init. Thanks

clab/config.go Outdated Show resolved Hide resolved
nodes/cvx/cvx.go Show resolved Hide resolved
@hellt
Copy link
Member

hellt commented Jul 6, 2021

@networkop In 362dd1b I removed a few leftovers of log.Fatal

@networkop
Copy link
Contributor Author

networkop commented Jul 6, 2021

thanks @hellt . I also pushed a fix for the shortname parsing logic.

Copy link
Member

@hellt hellt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solid work @networkop
I hope you had fun time ;) that is one of the biggest PRs in this repo

@hellt hellt merged commit 4d5938e into srl-labs:master Jul 6, 2021
@hellt hellt mentioned this pull request Jul 6, 2021
@networkop
Copy link
Contributor Author

awesome! thanks for reviewing it @karimra @hellt

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

3 participants