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

Add vJunos-Router kind based on existing vJunos-switch implementation #2063

Merged
merged 7 commits into from
Jun 6, 2024

Conversation

vista-
Copy link
Contributor

@vista- vista- commented Jun 3, 2024

This PR adds a vJunos-Router kind to match the recently added vrnetlab image: hellt/vrnetlab#196

Since both vJunos-Switch and vJunos-Router images are handled identically by containerlab, alternatively, the two kinds could be merged into a "vJunos" or "vJunos-Classic" kind that handles both vJunos-Switch and vJunos-Router nodes; however, this difference in naming might be confusing to users.

This PR also contains some minor fixes to syntactic fixes to the documentation and adds short name support for other vJunos kinds.

@vista-
Copy link
Contributor Author

vista- commented Jun 3, 2024

Adding the new node type to https://github.com/srl-labs/containerlab/blob/main/docs/manual/vrnetlab.md will require a new release to be carved both for vrnetlab and containerlab.

@hellt
Copy link
Member

hellt commented Jun 5, 2024

Hi @vista-
thanks for your PR

I wonder, could you try and add the new kind name to this list https://github.com/srl-labs/containerlab/blob/main/nodes/vr_vjunosswitch/vr-vjunosswitch.go#L20

kindnames          = []string{"juniper_vjunosswitch","juniper_vjunosrouter"}

and then remove the duplicated code? I suppose this should work nicely but I have no vrouter image to test this myself, so if you give it a spin that might be very helpful and should reduce the duplication

@vista-
Copy link
Contributor Author

vista- commented Jun 5, 2024

@hellt merged the two kinds into one at your suggestion. For sake of simplicity, I left the vJunos-switch and vJunos-router's documentation separate.

I also updated the documentation based on your feedback, pointing out that all vJunos images can be downloaded without a Juniper account. :)

docs/index.md Outdated
@@ -68,38 +69,38 @@ This short clip briefly demonstrates containerlab features and explains its purp

## Features

* **IaaC approach**
Copy link
Member

Choose a reason for hiding this comment

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

@vista-
ah, you will have to bring those changes back =)
they are newlines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what got deleted here at the end of the line -- just two spaces at the end of the line? I don't think it was CRLF based on the line encodings in other files.

I honestly don't know what introduced these line ending changes (using VS code with default settings on Mac).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

microsoft/vscode#1679 (comment)

mystery solved 🤷


## Managing Juniper vJunosEvolved nodes

!!!note
Containers with vJunosEvolved inside will take ~15min to fully boot.
Copy link
Member

Choose a reason for hiding this comment

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

we will have to bring this back


## Managing Juniper vJunos-switch nodes

!!!note
Containers with vJunos-switch inside will take ~15min to fully boot.
Copy link
Member

Choose a reason for hiding this comment

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

we will have to bring this back =)

@@ -53,8 +53,8 @@ The following table provides a link between the version combinations:
| `0.54.0` | [`0.16.0`](https://github.com/hellt/vrnetlab/releases/tag/v0.16.0) | Added support for Cisco c8000v |

???note "how to understand version inter-dependency between containerlab and vrnetlab?"
When new VM-based platform support is added to vrnetlab, it is usually accompanied by a new containerlab version. In this case the table row will have both containerlab and vrnetlab versions.
Copy link
Member

Choose a reason for hiding this comment

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

we will need those newlines added back

@@ -117,7 +118,7 @@ Containerlab offers several ways of connecting VM-based routers with the rest of
<div class="mxgraph" style="max-width:100%;border:1px solid transparent;margin:0 auto; display:block;" data-mxgraph="{&quot;page&quot;:6,&quot;zoom&quot;:1.5,&quot;highlight&quot;:&quot;#0000ff&quot;,&quot;nav&quot;:true,&quot;check-visible-state&quot;:true,&quot;resize&quot;:true,&quot;url&quot;:&quot;https://raw.githubusercontent.com/srl-labs/containerlab/diagrams/vrnetlab.drawio&quot;}"></div>

??? "Any other datapaths?"
Although `tc` based datapath should cover all the needed connectivity requirements, if other bridge-like datapaths are needed, Containerlab offers OpenvSwitch and Linux bridge modes.
Copy link
Member

Choose a reason for hiding this comment

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

we will need those newlines added back

@@ -17,7 +17,7 @@ import (
)

var (
kindnames = []string{"juniper_vjunosevolved"}
Copy link
Member

Choose a reason for hiding this comment

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

we also are getting away from vr- named kinds, so keep only juniper_vjunos... in the kind names

Comment on lines 910 to 915
"vr-vjunosrouter": {
"$ref": "#/definitions/node-config"
},
"vr-juniper_vjunosrouter": {
"$ref": "#/definitions/node-config"
},
Copy link
Member

Choose a reason for hiding this comment

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

when you remove the vr- kind names from the go code we should also remove it from the schema

@hellt
Copy link
Member

hellt commented Jun 5, 2024

thanks @vista-
I left a bunch of NIT comments mostly about the kind names and removed soft newlines

thank you for your help!

@vista-
Copy link
Contributor Author

vista- commented Jun 5, 2024

@hellt Just pushed some changes removing the vr-prefixed short names and fixing the newlines!

Thanks for your quick review :)

@hellt
Copy link
Member

hellt commented Jun 6, 2024

great, thank you! Merging

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.73%. Comparing base (2b735d9) to head (797523e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2063      +/-   ##
==========================================
- Coverage   53.82%   53.73%   -0.10%     
==========================================
  Files         160      160              
  Lines       11524    11524              
==========================================
- Hits         6203     6192      -11     
- Misses       4458     4465       +7     
- Partials      863      867       +4     
Files Coverage Δ
cmd/generate.go 68.30% <ø> (ø)
nodes/vr_vjunosswitch/vr-vjunosswitch.go 2.43% <ø> (ø)

... and 3 files with indirect coverage changes

@hellt hellt merged commit bad598d into srl-labs:main Jun 6, 2024
62 of 63 checks passed
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

2 participants