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 juniper_junos_show_system_processes_brief.textfsm support #1922

Conversation

jnicholson56
Copy link
Contributor

adding juniper_junos_show_system_processes_brief.textfsm support

Copy link
Contributor

@jmcgill298 jmcgill298 left a comment

Choose a reason for hiding this comment

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

Thanks @jnicholson56 just a couple of minor adjustments.

@mjbear
Copy link
Collaborator

mjbear commented Jan 3, 2025

@jnicholson56
If you're just getting back from winter break I totally understand the email backlog. 😁 🫣
Similar to PR #1921, there are a few suggestions on this junos show system processes brief template that are pending your review.

Thank you!

@jnicholson56
Copy link
Contributor Author

jnicholson56 commented Jan 3, 2025 via email

@mjbear
Copy link
Collaborator

mjbear commented Jan 4, 2025

Thanks for the reminder. Having some motivation issues during the holidays. I will try to get to it soon.

All good, I understand. ☺️

@jnicholson56
Copy link
Contributor Author

Updated the files based on the suggested changes

@mjbear
Copy link
Collaborator

mjbear commented Jan 4, 2025

@jnicholson56
Apologies for all the messages.
(I need to get some Juniper images or gear so I can be more helpful with JunOS.) 😉

Since this is a template that didn't exist before, index file changes need to be made per the docs.

Edit: It might feel like an uphill battle, but if the template is useful to you then it probably is to someone else. Persevere and you'll this ready to merge!

^Tasks:\s*
^\%Cpu
^${MEMORY_FORMAT}i*B\s*MEMORY\s*:\s*${MEMORY_ACTIVE}\s*total,\s*${MEMORY_FREE}\s*free,\s*${MEMORY_USED}\s*used,\s*${MEMORY_BUFFER}\s*buff/cache\s*
^${SWAP_FORMAT}i*B\s*Swap:\s*${SWAP_TOTAL}\s*total,\s*${SWAP_FREE}\s*free,\s*${SWAP_USED}\s*used\.\s*${SWAP_AVAIL}\s*avail\s*MEMORY\s* -> Record
Copy link
Collaborator

@mjbear mjbear Jan 4, 2025

Choose a reason for hiding this comment

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

The suggestions below:

  • Make the i optional - I'm guessing there's a reason it was i* despite there being no test data showing that (which I've requested for the "memory" line too).
  • Change many \s* to \s+ since we expect one or more spaces (instead of zero or more).
  • MEMORY becomes Mem in the swap memory line
    • (If there truly is a line that has MEMORY, we can work on that regex further)

🎯 (Seeing as how the Memory line "used" is followed by a comma ...) On the swap line, the used. seems like a typo (could also be present per Juniper) that maybe we need to account for with a regex so we're not fiddling with this template later (if that's ever fixed).

Suggested change
^${SWAP_FORMAT}i*B\s*Swap:\s*${SWAP_TOTAL}\s*total,\s*${SWAP_FREE}\s*free,\s*${SWAP_USED}\s*used\.\s*${SWAP_AVAIL}\s*avail\s*MEMORY\s* -> Record
^${SWAP_FORMAT}i?B\s+Swap:\s+${SWAP_TOTAL}\s+total,\s+${SWAP_FREE}\s+free,\s+${SWAP_USED}\s+used\.\s+${SWAP_AVAIL}\s+avail\s+Mem\s* -> Record

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The i* was a suggestion from the other PR that I am working on. I am unsure of why, but i just added it here too. There was no explanation as to why it was suggested there. I have reverted it back to just MiB and will leave it. As far as I have seen across all of the versions I have worked with, that has stayed consistent.

I updated the \s* to \s+

MEMORY changing to Mem is a new output thing. We had just upgraded to a new version of Junos and I had not realized the output had changed. Maybe we should have MEMORY or Mem, to keep it backwards compatible?

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 added a either or for MEMORY and ME to the lastest commit. Let know if that is not correct. It seems to work for me when I test it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The i* was a suggestion from the other PR that I am working on. I am unsure of why, but i just added it here too. There was no explanation as to why it was suggested there. I have reverted it back to just MiB and will leave it. As far as I have seen across all of the versions I have worked with, that has stayed consistent.

Good.

MEMORY changing to Mem is a new output thing. We had just upgraded to a new version of Junos and I had not realized the output had changed. Maybe we should have MEMORY or Mem, to keep it backwards compatible?

I agree with making it backwards compatible.

Might you have a spare device that you can get the old MEMORY output from?
Ideally we should include both types of test data so we have better test coverage. Otherwise if we need to change something later and don't have the test data we risk breaking the template.

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 work for an SP and we use Juniper as our primary routers for core and edge. I have access to quite a few router models and SW versions (We use Junos Classic and EVO). I can downgrade a test router as well if needed. Let me see what I can come up with tomorrow as far as examples across versions.

In the past I have had issues with Juniper adding hidden spaces into output. The \s+ changes should take care of that. The reason I went with \s* as heavily as I did was out of frustration with templates breaking when we upgraded because of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjbear

I have a large cross section of Junos Versions ~14.x to 23.x and platforms (ex, qfx, ptx, mx, vrr) to look at. I found the output I am using in this PR is for EVO. The classic output is pretty different.

This is the classic output which appears constant regardless of version I look at.

last pid: 95940;  load averages:  0.81,  0.77,  0.70  up 103+12:35:31    14:17:19
290 processes: 3 running, 286 sleeping, 1 waiting
CPU:  5.6% user,  0.0% nice,  3.0% system,  0.3% interrupt, 91.1% idle
Mem: 2421M Active, 16G Inact, 1403M Wired, 409M Buf, 27G Free
Swap: 3072M Total, 3072M Free

It's been a while since I worked on this template and I forgot the context of the work I was doing on it. Classic and EVO use two different underlying OSes (FreeBSD vs Linux). The output for this command is from the underlying OS. EVO also displays the output per RE and per FPC hence the multiple copies of the output as opposed to Classic only show the current RE the command is being ran from.

How is Classic versus EVO being handled in textfsm? The OS name remains the same as far as I know and I do not recall see any templates that differentiate between the two. Or do we write one template to handle both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fwiw I can try to work on a single template, but I wonder if that won't be too confusing for others when they try to read it later.

@jnicholson56
Copy link
Contributor Author

@jnicholson56 Apologies for all the messages. (I need to get some Juniper images or gear so I can be more helpful with JunOS.) 😉

Since this is a template that didn't exist before, index file changes need to be made per the docs.

Edit: It might feel like an uphill battle, but if the template is useful to you then it probably is to someone else. Persevere and you'll this ready to merge!

Thanks for the reminder. I had a bunch of templates submitted in a single PR which had the index file filled out. It was suggested I separate them all into individual PR's and I forgot to transfer the Index file changes.

@jnicholson56
Copy link
Contributor Author

@mjbear

Edit: It might feel like an uphill battle, but if the template is useful to you then it probably is to someone else. Persevere and you'll this ready to merge!

I started a path about 4-5 years ago to collect operational data off the network. Not a single command I wanted had a template. I went through a lot of pain to make the templates that worked and really dislike textfsm due to that process. A few months ago at AutoCon I made the decision to submit them to the community in hopes that I can improve them with feedback and as a form of therapy by releasing them to the community.

@mjbear
Copy link
Collaborator

mjbear commented Jan 7, 2025

@mjbear

Edit: It might feel like an uphill battle, but if the template is useful to you then it probably is to someone else. Persevere and you'll this ready to merge!

I started a path about 4-5 years ago to collect operational data off the network. Not a single command I wanted had a template. I went through a lot of pain to make the templates that worked and really dislike textfsm due to that process. A few months ago at AutoCon I made the decision to submit them to the community in hopes that I can improve them with feedback and as a form of therapy by releasing them to the community.

Yeah, there are certain situations where TextFSM can be challenging.

I'm glad you are choosing to share them back with the community. 🎊

My suggestion is to take the templates one or two at a time for the PR submission. It's going to be a bit of extra time/work to gather test data, but it is well worth it. If we didn't have the test data we would have created other "bugs" (ex: broken backwards compatibility with older output) when those of us went in to fix a single bug.

@jmcgill298 jmcgill298 merged commit fe4752d into networktocode:master Jan 7, 2025
10 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.

3 participants