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

Replace telnetlib with scrapli #279

Closed
wants to merge 34 commits into from

Conversation

kaelemc
Copy link

@kaelemc kaelemc commented Nov 7, 2024

This PR replaces the usage of telnetlib in vrnetlab with scrapli.

Changes

  • Telnetlib has been removed and replaced and now the Scrapli Base Driver class is used for base telnet functionality.

    • Previously telnetlib was used to connect to the qemu monitor, I didn't see any use for this anywhere so I removed this functionality as well
    • All nodes have had old telnetlib functions swapped with new scrapli-based ones.
    • All Dockerfiles except OpenWRT (and maybe one or two more i'm forgetting) should be on debian:bookworm-slim from the Amazon ECR.
    • Dockerfiles have had git and pip added, as well as installation command of Scrapli via the git repo.
  • Cisco and Juniper nodes are in their own vendor-specific subdirectory.

    • Defined a quick and dirty VENDOR_SUBDIR variable in the Makefiles of each node as well as logic in makefile.include so that files get copied one level further (due to vendor subdirectory). If VENDOR_SUBDIR is 1 then files are copied one subdirectory level further to account for the vendor subdirectory.
  • Logger for VR class (inherited by all nodes) now uses a logger under the vrnetlab instance. This is because Scrapli uses the root logging instance, and outputs all channel output as 'debug' log level.

    • The root logger is now set to only write 'info' level logs.
    • self.logger will still write 'debug' logs to stdout (docker logs).
  • Coloured the logging levels so it's easier to see, looks nicer visually too.

Functions

read_until()

telnetlib had a read_until() function which was used by wait_write. This has been re-implemented under the VR class.

Args:

  • match_str: a string of the text to wait for
  • timeout: a float of how long to wait before exiting the function even if no match is found. Defaults to None.

Returns: All console output that was read up until the match, or function timeout.

wait_write()

Adapted to scrapli, functionality should be analogous to the telnetlib version.

  • Added argument timeout (float): This has been added, and is passed to read_until, Defaults to None.

expect()

telnetlib had the expect() function which was used in the launch.py entrypoint. This new adapted version in the VR class should function the same as the telnetlib one.

Args:

  • regex_list: a list of byte-strings which are used to match via regex on the console output.
  • timeout: a float of how long before the function should just timeout and stop waiting. Defaults to None.

Returns: a tuple of:

  • List index of the matched byte-string. Defaults to -1 if there was no match. (int)
  • The regex match object. (re.match)
  • The console output up until that point (byte-string)

print()

The VR class now has a print function which is used to simply write to stdout and instantly flush. It's used so that the telnet console can output nicely on docker logs.

If the console output was printed via the logger, the formatting would make it difficult to interpret the output.

Args:

  • bytes: byte-string of what to write to stdout.

Usage

Relevant changes to make a node work:

  • Ensure git, pip and scrapli are installed in the Dockerfile

  • Replace self.tn.expect() with self.expect()

  • Use self.print() to print any console output, do not use logging for this. There is no need to decode()/convert the returned byte-string of expect() as self.print() doesn't accept strings, this would result in malformed outputs.

  • Logging levels for any log output should be changed to be more appropriate.

  • INFO is Green

  • WARNING is Yellow

  • ERROR/DEBUG is Red

Other

  • Added an XRv_qcow folder to build XRv images out of qcow2 images, existing XRv builds out of vmdks
  • XRv9k, CSR, cat8kv and cat9kv nodes use their relevant Scrapli driver.
  • XRv didn't play nicely with the XR driver, I assume due to poor performance and old code version (XRv is deprecated and only runs 6.x code, XRv9k is 7.x)
  • ASAv was not working correctly, had to make fixes for that.

I don't expect other nodes to 'plug and play' and work perfectly from the get go, I'm sure I have made errors or overlooked something so some work is required on the other nodes to ensure functionality.

  • For other nodes I think the Scrapli drivers can probably increase reliability and make it easier to send configs.

Collaboration from others is required so we can confirm all nodes work reliably (as well as to fix some other possible issues in the way i've implemented the changes). I'm open to feedback 😊.

Confirmed working platforms

subdirectory names are in brackets. Follows alphabetical/subfolder order of the directory tree.

  • Aruba AOS-CX
  • Cisco ASAv
  • Cisco Cat8kv (c8000v)
  • Cisco Cat9kv
  • Cisco CSR1kv (csr)
  • Cisco FTDv
  • Cisco Nexus 9k (n9kv)
  • Cisco NX-OS
  • Cisco IOSv (vios)
  • Cisco XRv (.vmdk)
  • Cisco XRv (.qcow2) (xrv_qcow)
  • Cisco XRv9k
  • cmglinux
  • Dell SONiC
  • Fortigate
  • FreeBSD
  • Dell OS10 (ftosv)
  • Huawei VRP
  • Juniper vJunosEvolved
  • Juniper vJunosRouter
  • Juniper vJunosSwitch
  • Juniper vMX
  • Juniper vQFX
  • Juniper vSRX
  • IPInfusion OcNOS
  • OpenBSD
  • OpenWRT
  • Palo Alto PA-VM (pan)
  • MikroTik ROS (routeros)
  • SONiC
  • Nokia SROS
  • Ubuntu
  • Arista vEOS
  • Huawei VRP (again?) (vrp)
  • HP vsr1000

- Add python3-pip, git and scrapli install via pip to all node - Dockerfiles.

- Replace all nodes telnetlib functions with new adapated scrapli functions. (wait_write, expect, read_until)

- Move Cisco and Juniper nodes to their own vendor subdirectories, fix makefiles to ensure functionality works.

- Other things, which should be outlined in the PR.
@kaelemc
Copy link
Author

kaelemc commented Nov 8, 2024

It might be worth making another scrapli branch and then I can change this PR to merge into that branch (instead of master).

This way other contributors can submit PRs for any changes to other nodes and once all/enough nodes work, that branch can be merged into master?

@hellt
Copy link
Owner

hellt commented Nov 8, 2024

sound idea @kaelemc
I have created the scrapli-dev branch

@kaelemc kaelemc changed the base branch from master to scrapli-dev November 8, 2024 09:49
…nfigurations

- Startup and bootstrap configurations use scrapli with IOSXEDriver
- Changed variable name from 'csr' to 'cat8kv' in cat8kv install function.
- Reverted change in bootstrap_spin so that console output is evaluated against empty byte-string instead of regular string.
@kaelemc
Copy link
Author

kaelemc commented Nov 9, 2024

IOS-XE nodes (CSR1000V, Cat8kv and Cat9kv) the Scrapli XE driver is now being used. Had some issues with it working with IOS-XE 16.x but it was an error on my end.. got the configs (both bootstrap and startup) to work nicely.

- existing XRv node uses vmdk image only. This one will use the qcow2 image.
@kaelemc
Copy link
Author

kaelemc commented Nov 9, 2024

Maybe this belongs as it's own PR but I've added an xrv_qemu directory. It is the same as XRv but with some modifications to make XRv work when the user provides the qcow2 image.

The existing xrv directory requires vmdk images, most users who import images from CML or other places on the web most likely will get a qcow2, this makes it easier for users so they don't have to mess around with qemu-img. (even the vmdk I have has been converted from a qcow via qemu-img)

I tried one more time to use the scrapli IOSXRDriver with XRv but it still seems unreliable, and I get some weird behavior from the XRv (don't think this is scrapli's fault).

- Alter the log levels for some logs from debug->error
- Move the VM startup log message and add information about qemu smp and startup RAM.
@kaelemc kaelemc marked this pull request as ready for review November 9, 2024 10:39
@hellt hellt mentioned this pull request Nov 9, 2024
@jbemmel
Copy link

jbemmel commented Nov 9, 2024

Not a trivial change, but I would suggest to create 1 base Dockerfile for all of vrnetlab, and then derive all platform images from that

It doesn't make sense to do 100x "install scrapli" in all those separate files - that becomes unmanageable

@hellt
Copy link
Owner

hellt commented Nov 9, 2024

yeah we can definitely create the base image hosted on ghcr.io with base pacakages that every vr system relies on

@kaelemc
Copy link
Author

kaelemc commented Nov 9, 2024

@jbemmel Excellent idea, certainly is/was unmanageable for me

@kaelemc
Copy link
Author

kaelemc commented Nov 10, 2024

So this is a difficult one to balance, in labs of different sizes the nodes take varying times to boot (depending on system specs etc.).. I'm wondering if we should bump all the scrapli timeouts to something very large to make the connection timing out a non-issue?

Currently I have XE devices on 10 minutes, I have a feeling for people with lower-spec systems might hit time-outs meaning they could never boot the device. Should we increase to something very large just to be safe?

The base Scrapli Driver (underlying telnet connection for wait_write, expect and read_until functions) uses a timeout of 1hr.

@hellt
Copy link
Owner

hellt commented Nov 10, 2024

I agree, let's have a long enough timeout so that you don't have to guess the time down to minutes.

We should leave the timeout configurable via the env var setting that can be then put in the topology file if needed to tune it up

@kaelemc
Copy link
Author

kaelemc commented Nov 10, 2024

Ok 👍, having timeouts as an env var is a good idea

- New env var SCRAPLI_TIMEOUT added, defaults to 3600 seconds (1 hour).
- It's used to enable the user to modify the operation, transport and socket timeout for the Scrapli driver used to apply the config to the CSR.
@kaelemc
Copy link
Author

kaelemc commented Nov 13, 2024

I had to remove NETCONF configuration on CSR as different versions of IOS-XE 16.x and 17.x have different behaviors when this command is entered in the config.

In 16.x a prompt will appear asking for some confirmation about the NETCONF configuration. I figured it's best to just remove this instead of adding extra complexity to handle the prompt for that single command.

Thoughts?

@hellt
Copy link
Owner

hellt commented Nov 20, 2024

@kaelemc added genisoimage and reuploaded 0.0.1 image

@kaelemc
Copy link
Author

kaelemc commented Nov 21, 2024

@hellt Thanks, I guess one more thing is if you could just change Scrapli to install from the latest changes via:

pip install git+https://github.com/carlmontanari/scrapli --break-system-packages

Otherwise everything is broken and won't work.. (seems to be a random EOF sent at some point from either qemu or the node itself and Scrapli will think the connection hasn't been opened correctly)

If/when a new release comes out with the fixes then we can pin the version to that release :)

@carlmontanari
Copy link

@kaelemc 2024.7.30.post1 just pushed to pypi 🫡

@hellt
Copy link
Owner

hellt commented Nov 21, 2024

thanks Carl
@kaelemc I have reuploaded the base image with the new version

@kaelemc
Copy link
Author

kaelemc commented Nov 21, 2024

@carlmontanari @hellt Thanks!

- Use format string in log colour formatting
- Print newlines before log messages in wait_write() to improve visual clarity of log messages.
- Hopefully this is more intuitive to users, they see they have a .qcow2 file and can use the xrv_qcow dir?
@kaelemc
Copy link
Author

kaelemc commented Nov 22, 2024

@hellt New image (& scrapli release) are all working great, i've moved Cisco nodes over so far and tested all of them. Install time is much better without having to re-install all the packages (even better when everything is cached locally of course 😄).

I have the Dockerfile changes ready for all the other nodes but I want to test as much as I can first to see if any key packages are missing, any incompatibilities etc.

In terms of Dockerfiles, since the MAINTAINER tag is now deprecated, should I migrate those over to labels or remove them entirely, it seems some of these may have been copied over from the original vrnetlab so possibly those users aren't active, or contributing to hellt/vrnetlab? I'm also seeing inconsistencies in how the maintainer labels are handled (LABEL maintainer vs LABEL org.opencontainers.image.authors) and only a few Dockerfiles have these labels anyway.

and in the recent commits i've just pushed you'll see on XRv9k I've switched it over to pull vcpu and ram from the env vars directly instead of via the flags/argparser. I figured if we want vrnetlab to be the 'source of truth' for those things (discussed in #2285.) there's no need to have the clab end to contain logic to pass vcpu/ram via the flags... do you agree with this approach, am I clouding this PR with too much other stuff, what are your thoughts?

I've also implemented it this way with the cat8kv (for some reason there was no vcpu/ram knobs implemented).

@hellt
Copy link
Owner

hellt commented Nov 22, 2024

you can remove the maintainer label/field altogether. It is not up to date anyways.

Yes, correct, the defaults for the cpu/mem should stay within the vrnetlab node definition, and containerlab can override this via QEMU

@kaelemc
Copy link
Author

kaelemc commented Nov 22, 2024

you can remove the maintainer label/field altogether. It is not up to date anyways.

👍

Yes, correct, the defaults for the cpu/mem should stay within the vrnetlab node definition, and containerlab can override this via QEMU

Alright, currently I have it set to use env vars called VCPU and RAM as that's what I've generally always used (it's what containerlab is using. It's also in the xrv9k docs in the 'resource requirements' admonition.

Didn't know about the QEMU_SMP/QEMU_MEMORY but I can easily changeover if you think this is the preferred naming?

@hellt
Copy link
Owner

hellt commented Nov 22, 2024

it is not only the preferred naming, but also is generically loaded in the vrnetlab common

The reason there are VCPU/MEM dangling in the code base is pure legacy. We should converged on the global QEMU_* env vars as they are read once by every node, without duplication

@kaelemc
Copy link
Author

kaelemc commented Nov 22, 2024

Oh perfect, got it. Will make the changes 👍

aoscx/docker/Dockerfile Outdated Show resolved Hide resolved
@kaelemc kaelemc marked this pull request as draft November 23, 2024 04:20
@kaelemc
Copy link
Author

kaelemc commented Nov 23, 2024

Will convert as many of the nodes to use the Scrapli community drivers as possible.

I've added a checklist in the original PR comment of nodes I think are 'working' for sure.

  • Unlike XRv9k, regular XRv isn't using the IOS-XR driver because it's very much legacy and doesn't want to play nice.
  • All XE nodes are using IOSXEDriver and work perfectly
  • In terms of Cisco, FTDv, ASAv, Nexus devices and vIOS I will try to move to their respective drivers (vIOS runs classic IOS but should work with the XE Driver)

Every node is using the new base vrnetlab image, will have to iron out any missing packages or incompatibilities there.

All Cisco nodes have default SMP/memory and I have tested that they are infact overriden correctly using the respective env vars as pointed out. I had to make a minor change for XRv9k as without the explicit single socket config it did nothing but kernel panic.

This means like other nodes, for XRv9k users can just set the QEMU_SMP to the number of vCPUs they want instead of QEMU_SMP: cores=x,threads=1,sockets=1. But if they want to supply it like this they still can.

common/vrnetlab.py Outdated Show resolved Hide resolved
juniper/vmx/Makefile Show resolved Hide resolved
Comment on lines 247 to 251
self.tn.write("yes\r".encode())
self.logger.trace("Read: %s" % res.decode())
self.print(res)
self.logger.debug("writing to serial console: %s" % cmd)
self.tn.write("{}\r".format(cmd).encode())

Choose a reason for hiding this comment

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

2024-11-15 02:18:02,877: launch     DEBUG writing to serial console: root
Traceback (most recent call last):
  File "/launch.py", line 483, in <module>
    vr.start()
  File "/vrnetlab.py", line 734, in start
    vm.work()
  File "/vrnetlab.py", line 524, in work
    self.bootstrap_spin()
  File "/launch.py", line 123, in bootstrap_spin
    self.wait_write("root", wait=None)
  File "/launch.py", line 250, in wait_write
    self.tn.write("{}\r".format(cmd).encode())
    ^^^^^^^^^^^^^
AttributeError: 'Driver' object has no attribute 'write'

Copy link
Author

Choose a reason for hiding this comment

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

Oops sorry about that, Still have to migrate to scrapli JunOS driver :)

Comment on lines -175 to -176
self.wait_write("restconf")
self.wait_write("netconf-yang")

Choose a reason for hiding this comment

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

Which version of CSR did you find had a problem with these? I only have csr1000v-universalk9.17.03.04a.qcow2 where it seems to work. You could use the same version condition that was in place for the ip domain name command if the NETCONF command is problematic on older.

Copy link
Author

Choose a reason for hiding this comment

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

Was testing with both XE17 and 16.09.x versions (which were the problematic ones).

Comment on lines -156 to -159
if int(self.version.split('.')[0]) >= 16:
self.wait_write("ip domain name example.com")
else:
self.wait_write("ip domain-name example.com")

Choose a reason for hiding this comment

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

This should be part of the new config string too, if older CSR are still supported.

Copy link
Author

Choose a reason for hiding this comment

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

👍 trying to get an older version to test this out with.

Comment on lines -138 to -151
# check if we are prompted to overwrite current keys
(ridx, match, res) = self.tn.expect(
[
b"How many bits in the modulus",
b"Do you really want to replace them",
b"^[^ ]+#",
],
10,
)
if match: # got a match!
if ridx == 0:
self.wait_write("2048", None)
elif ridx == 1: # press return to get started, so we press return!
self.wait_write("no", None)

Choose a reason for hiding this comment

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

If you restart a container it comes up with keys already configured:

RP/0/0/CPU0:vr-xrv#2024-11-15 13:31:22,158: vrnetlab   INFO writing to console: 'crypto key generate rsa'
2024-11-15 13:31:22,258: vrnetlab   INFO writing to console: '2048'
2024-11-15 13:31:22,258: vrnetlab   INFO waiting for '#' on console.
crypto key generate rsa
Fri Nov 15 13:31:21.519 UTC
The name for the keys will be: the_default
% You already have keys defined for the_default
Do you really want to replace them? [yes/no]: 2048
% Please answer 'yes' or 'no'.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, could you explain a little further, because I don't think I can see this behaviour?

Thanks for your review, appreciate it a lot.

I'll have to go over each platform and migrate them all to their Scrapli (and Scrapli Community) drivers. If it's ok I'll request another review when that's all good to go 👍

@kaelemc
Copy link
Author

kaelemc commented Dec 20, 2024

Replaced by #297

@kaelemc kaelemc closed this Dec 20, 2024
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.

vendor telnetlib
5 participants