Skip to content

Add cJunosEvolved #2617

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

Merged
merged 2 commits into from
Jul 15, 2025
Merged

Add cJunosEvolved #2617

merged 2 commits into from
Jul 15, 2025

Conversation

kmo7
Copy link
Contributor

@kmo7 kmo7 commented Jun 11, 2025

  • adding cJunosEvolved to containerlab
  • adding the cJunosEvolved lab example and it's doc
  • adding the cJunosEvolved description manual

@hellt
Copy link
Member

hellt commented Jun 12, 2025

thanks @kmo7

@vista- could you give this a try when you have time given you are my Juniper buddy 👯

Copy link
Contributor

@vista- vista- left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution @kmo7! I left some comments and suggestions. Since I don't think this is a publicly available product yet (or is it? some clarification would be helpful :)), I think it would be best to hold off on merging this until its released, just for the sake of the maintainers' sanity ;)

Unless the release is imminent, of course, we'd love to help you launch with Clab support built-in!

@kmo7
Copy link
Contributor Author

kmo7 commented Jun 12, 2025 via email

@hellt
Copy link
Member

hellt commented Jun 12, 2025

Thanks @kmo7 for the comments. It is great seeing contributions for new systems coming ahead of the release time, love it.

Given our reliance on you doing the testing, I'd say the path forward is to address existing comments and then re-checking that the same MVP reproduction with srl-cjunos works


Juniper cJunosEvolved nodes launched with containerlab can be provisioned to enable SSH, SNMP, NETCONF and gNMI services.

## How to obtain the image
Copy link
Member

Choose a reason for hiding this comment

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

@kmo7 ping us here when the image becomes available, @vista- and myself will do some hands-on testing and then we merge this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great. Will do. You can both can ping me on here if you need to discuss something after you start testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can also be reached via Discord if need be :)

@vista- vista- self-requested a review June 19, 2025 08:48
@vista-
Copy link
Contributor

vista- commented Jun 19, 2025

@kmo7 I rebased your branch onto latest main to add a recent commit that fixes the CI/CD pipeline, just as a heads-up

@kmo7
Copy link
Contributor Author

kmo7 commented Jun 19, 2025

@kmo7 I rebased your branch onto latest main to add a recent commit that fixes the CI/CD pipeline, just as a heads-up

Thanks Vista, i don't see this commit reflected in containerlab.dev yet, e.g. on the front page where vendor notes are listed.
https://containerlab.dev/ or in the nodes directory: https://github.com/srl-labs/containerlab/tree/main/nodes

Are you waiting on the image from us or the rebase operation to be done>

@nleiva
Copy link
Contributor

nleiva commented Jul 11, 2025

Hi all,

Might be too early, but would it be possible to add a pre-config hook to the cJunosEvolved router kind in Containerlab to execute a couple of operational commands that generate TLS certificates directly on the container?

The commands are:

request security pki generate-key-pair certificate-id myname
request security pki local-certificate generate-self-signed certificate-id myname subject CN=abc domain-name clab.com ip-address 1.2.3.4 email [email protected]

This would enable the configuration of TLS-based services like RESTCONF in the startup-config:

services {
    rest {
        https {
            port 3000;
            server-certificate myname;
        }
    }
}

Let me know if that’s feasible or if there’s a preferred approach.

Thanks!
Nicolas

@vista-
Copy link
Contributor

vista- commented Jul 14, 2025

Hi @kmo7,

Congrats on launching cJunosEvo! Thank you again for your hard work with preparing this PR.

I just tried out the cJunosEvo image, just a few observations:

The way the startup configuration to the node is provided does not really match the way most other Containerlab kinds do it: they just append the generated config required for management connectivity to the provided config.

cJunosEvolved, on the other hand, expects the provided startup config to contain the mgmt config to template into.

This essentially means a user would have to manually edit the configuration saved from the node in order for it to be usable in the next deploy (which might not assign the same management IP address to the node).

Rather than your current approach for generating an initial startup configuration, perhaps it would be better to put this logic (rendering a default config template and merging it with the user-provided startup config) into Containerlab? Check out how it's done with cRPD, it's fairly easy to template in runtime variables: https://github.com/srl-labs/containerlab/blob/main/nodes/crpd/crpd.go#L155

Some further notes:

  • The default config loaded on the node is a bit lackluster. I would suggest adding the following:
    • Hostname should be included for easy identification of nodes in the topology
    • NETCONF and gNMI should be pre-enabled in the default config
    • system { management-instance; } helps avoid issues down the line where you might kill your own management connectivity
  • I would add "Node can take 5+ minutes to boot" to the Kind documentation, since some users won't look in the container logs to find out if it's actually booting or not.
  • Perhaps define a health check command to give feedback to the user/containerlab when the node has finished booting?

@kmo7
Copy link
Contributor Author

kmo7 commented Jul 15, 2025

Hi @kmo7,

Congrats on launching cJunosEvo! Thank you again for your hard work with preparing this PR.

I just tried out the cJunosEvo image, just a few observations:

The way the startup configuration to the node is provided does not really match the way most other Containerlab kinds do it: they just append the generated config required for management connectivity to the provided config.

cJunosEvolved, on the other hand, expects the provided startup config to contain the mgmt config to template into.

This essentially means a user would have to manually edit the configuration saved from the node in order for it to be usable in the next deploy (which might not assign the same management IP address to the node).

Rather than your current approach for generating an initial startup configuration, perhaps it would be better to put this logic (rendering a default config template and merging it with the user-provided startup config) into Containerlab? Check out how it's done with cRPD, it's fairly easy to template in runtime variables: https://github.com/srl-labs/containerlab/blob/main/nodes/crpd/crpd.go#L155

Some further notes:

* The default config loaded on the node is a bit lackluster. I would suggest adding the following:
  
  * Hostname should be included for easy identification of nodes in the topology
  * NETCONF and gNMI should be pre-enabled in the default config
  * `system { management-instance; }` helps avoid issues down the line where you might kill your own management connectivity

* I would add "Node can take 5+ minutes to boot" to the Kind documentation, since some users won't look in the container logs to find out if it's actually booting or not.

* Perhaps define a health check command to give feedback to the user/containerlab when the node has finished booting?

Reponse:
Hi Vista, thanks for the review. Have incorporated some of the comments and we will look at the rest for the next commit. Since cJunosEvolved has now been also verified by you to be working, can you please merge the pull request into containerlab so the many others that are expressing interest to use it, can do so, and will get back to you on the rest? Please see below responses to the comments.

  • With respect to the longer comment up front, this is the key point "This essentially means a user would have to manually edit the configuration saved from the node in order for it to be usable in the next deploy (which might not assign the same management IP address to the node)."
    3 points on this:
    1- The current FXP0ADDR token scheme our code uses, does correctly configure the management IP according to what containerlab passes in to the container, even when the cevo instance is restarted and a new management port IP is assigned by the containerlab.
    2- As you know, the save command has not been working in general for many containers in containerlab and you are fixing that but it is not yet committed:
    nodes: Add generic implementation for VM-based node startup config saving #2659
    Once that is committed, for vr-net lab based containers, could you also fix it for cjunosevo?
    3- With the combination of the above 2 points, even though cjunosevolved does this differently than some other containers in clab, it will still function correctly.

wrt to these comments:
o Hostname should be included for easy identification of nodes in the topology. Yes, agreed. We will address this in a subsequent commit. This feature works in our docker compose deployements, but has one line missing to make it work in containerlab. Either by adding that line or by pushing the hostname from the go code, we will fix this, but do not want to delay the initial commit, so people can bringup cjunosevo and use it initially.
o NETCONF and gNMI should be pre-enabled in the default config

  • netconf already is enabled in the following config file: lab-examples/srlcjunosevo/cjunosevo.cfg. Users can do same for their deployments (will show the test log for this at the end).
  • GNMI was not enabled for our vJunosEvolved, vJunos-switch and vJunos-router also. This needs a bit more work and we will enable it in a subsequent commit.
    o system { management-instance; Added this to the above config file to separate the mgmt port into its own routing instance.
    "• I would add "Node can take 5+ minutes to boot" to the Kind documentation,". This was already documented in this kind doc file: docs/manual/kinds/cjunosevolved.md, line 28. If you think it is needed elsewhere too, can you please add it?

"Perhaps define a health check command to give feedback to the user/containerlab when the node has finished booting?" I have added this info the kind documentation file. Besides the docker log command, the " # docker exec -ti clab-srlcjunosevo-cevo cli" indicates bootup progress periodically, so the user is not blind to that.

Thanks for the careful review Vista. I hope above answers the immediate comments so you can commit the code into clab and I will follow up w/ the rest.

Here is a sample netconf log:

ssh admin@clab-srlcjunosevo-cevo -p 830 -s netconf

Warning: Permanently added '[clab-srlcjunosevo-cevo]:830' (ED25519) to the list of known hosts.
(admin@clab-srlcjunosevo-cevo) Password:

urn:ietf:params:netconf:base:1.0 urn:ietf:params:netconf:capability:candidate:1.0 urn:ietf:params:netconf:capability:confirmed-commit:1.0 urn:ietf:params:netconf:capability:validate:1.0 urn:ietf:params:netconf:capability:url:1.0?scheme=http,ftp,file urn:ietf:params:xml:ns:netconf:base:1.0 urn:ietf:params:xml:ns:netconf:capability:candidate:1.0 urn:ietf:params:xml:ns:netconf:capability:confirmed-commit:1.0 urn:ietf:params:xml:ns:netconf:capability:validate:1.0 urn:ietf:params:xml:ns:netconf:capability:url:1.0?scheme=http,ftp,file urn:ietf:params:xml:ns:yang:ietf-yang-metadata?module=ietf-yang-metadata&revision=2016-08-05 urn:ietf:params:xml:ns:yang:ietf-netconf-monitoring http://xml.juniper.net/netconf/junos/1.0 http://xml.juniper.net/dmi/system/1.0 http://yang.juniper.net/junos/jcmd?module=junos-configuration-metadata&revision=2021-09-01 31674 ]]>]]>

Copy link

codecov bot commented Jul 15, 2025

Codecov Report

Attention: Patch coverage is 17.64706% with 42 lines in your changes missing coverage. Please review.

Project coverage is 54.83%. Comparing base (02513f6) to head (69291f6).
Report is 25 commits behind head on main.

Files with missing lines Patch % Lines
nodes/cjunosevolved/cjunosevolved.go 16.00% 41 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2617      +/-   ##
==========================================
- Coverage   54.90%   54.83%   -0.07%     
==========================================
  Files         184      185       +1     
  Lines       20151    20202      +51     
==========================================
+ Hits        11063    11078      +15     
- Misses       7948     7985      +37     
+ Partials     1140     1139       -1     
Files with missing lines Coverage Δ
clab/register.go 100.00% <100.00%> (ø)
nodes/cjunosevolved/cjunosevolved.go 16.00% <16.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

* adding cJunosEvolved to containerlab
* adding the cJunosEvolved lab example and it's doc
* adding the cJunosEvolved description manual
Copy link
Contributor

@vista- vista- left a comment

Choose a reason for hiding this comment

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

lgtm

@hellt hellt merged commit 60dd97e into srl-labs:main Jul 15, 2025
73 of 75 checks passed
benjoe1126 pushed a commit to benjoe1126/containerlab that referenced this pull request Jul 19, 2025
* Add cJunosEvolved

* adding cJunosEvolved to containerlab
* adding the cJunosEvolved lab example and it's doc
* adding the cJunosEvolved description manual

* cjunosevo: gofmt + golintci

---------

Co-authored-by: vista <[email protected]>
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.

4 participants