Update expected prompt #3332
Update expected prompt #3332
Conversation
This is to fix issue ytti#3330 so multiline output won't break the module.
|
This seems like a bad risk. What characters the #3320 issue has in front of the prompt that make it not match? Is it \r? If so, perhaps ^\r? instead of removing the ^. Another option is removing screen scraping and turning on exec mode. |
|
Hi @ytti , I posted what shows when doing interactive SSH in the issue #3330 : With Please note that I was checking this via https://regex101.com/ and the above is not matched when |
|
Most prompts in oxidized match the line begin ('^') and line end ('$'). |
As the model has four different modes, I've added four cmfsets.
|
The cumulus model is not very nice, and I don't check what the different modes are, so I provided four command sets. Instructions: https://github.com/alchemyx/oxidized/blob/patch-1/docs/Issues.md#sumbit-a-yaml-simulation-file Command line (choose your cmdset filename according to the mode): |
|
Here's heavily redacted file -
https://github.com/alchemyx/oxidized/blob/patch-1/examples/device-simulation/yaml/cumulus_with-motd.yaml
It turns out there are some control chars between start of the line and
prompt.
czw., 5 gru 2024 o 17:37 Robert Chéramy ***@***.***>
napisał(a):
… The cumulus model is not very nice, and I don't check what the different
modes are, so I provided four command sets.
Could you provide a YAML simulation file in this PR for your mode / model,
with motd on and motd off?
Instructions:
https://github.com/alchemyx/oxidized/blob/patch-1/docs/Issues.md#sumbit-a-yaml-simulation-file
Command line (choose your cmdset filename according to the mode):
./device2yaml.rb ***@***.*** -c cmdsets/cumulus_<mode> -o yaml/cumulus_<with or without>-motd.yaml
—
Reply to this email directly, view it on GitHub
<#3332 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYNRMMFNDBFWVJZXGQCKIL2EB6MFAVCNFSM6AAAAABS5UART6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRQHA3DGNJQGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Michał Margula, ***@***.***
"W życiu piękne są tylko chwile" [Ryszard Riedel]
|
|
The codes (2004h, 2004l) you are seeing is 'bracketed paste', that could come also if you did copy+paste yourself, instead of actually being in the output. In case those are actually in the output, for prompt maybe Did you have chance to test of 'exec mode' works, this would remove the need for prompt detection. |
|
Nah, it was all created by the script. I have updated prompt to And tested, it works fine. I have updated my pull request and renamed yaml filename to something more meaningful (unless we want to remove the simulation output which is fine). |
You can't actuaklly use that, as you need to make the whole code part optional i.e. enclose it in ()? |
- Make the ANSI Escape Code optional for reverse compatibility - Removed unecessary parenthesis - Match the whole prompt line - Add a unit test to validate the prompt and the YAML Simulation File - Renamed the YAML Simulation File to match the hardware model - add "exit" to the command sets & the YAML Simulation File
| exit: |- | ||
| exit | ||
| oxidized_output: | | ||
| \e[?2004l\r- header: |
There was a problem hiding this comment.
Note: not nice - escape code in the saved config.
|
The I've updated the prompt to make the ANSI Escape Code optional for backward compatibility. There is still one thing wich is not very nice - Cumulus produces an ANSI Escape code in the configuration. Should we remove it or live it alone? |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3332 +/- ##
==========================================
+ Coverage 70.42% 71.41% +0.98%
==========================================
Files 77 82 +5
Lines 3202 3372 +170
==========================================
+ Hits 2255 2408 +153
- Misses 947 964 +17 ☔ View full report in Codecov by Sentry. |
We probably should. Procurve has this: # replace all used vt100 control sequences
expect /\e\[\??\d+(;\d+)*[A-Za-z]/ do |data, re|
data.gsub re, ''
end |
- Updated the garderos model, as we have unit tests for it an it uses ANSI ESC codes. - This will help me to improve PR #3332 with an universal approach. - No documentation to CHANGELOG.md, as documentation / change in Garderos covered by unit tests
- now the output looks better - added an entry in CHANGELOG.md
|
@alchemyx - if the new code works for you, I would merge it into master. |
Lessons learned from the cumuls model (PR #3332) - Adds an optional "\r" at the end of the regexp - Tests the change on the garderos model unit test - Updates the garderos model unit test so that it can tests prompts with ESC Codes
|
Unfortunately this version stops for me at prompt This is diff between working and not working version: |
|
Colleague at work recommended using \b and I tested, this also works: |
|
Interesting problem, as it works with the unit tests. Could you try following prompt (on last commit)? |
|
Okay, found one problem: (cumulus in normal mode): The file ports.conf does not end with a new line, so the prompt gets polluted with the last line of ports.conf ( So we are back to the initial commit to this PR, removing the match on a newline in the prompt, which @ytti did not like. With the following commits in the PR, the prompt at least got better and matches the end of the prompt, which I think is a good compensation for not matching the newline in the prompt. |
|
How about |
Some cumulus commands results do not end with \n, so the prompt gets polluted by the trailing line of the previous command. This is the case of `cat /etc/cumulus/ports.conf` in the new example attached in this commit.
This is an interesting option and I tested it: it works 👍 But the cumulus model has ~20 commands, and It makes the model somewhat unaesthetic to add About 20% of our models don't match on the line beginning, so I think it should be OK, as long as the prompt is specific enough. The new prompt ( @alchemyx - could you check if the new prompt works for you? If yes I'll make it more specific and let you try again ;-) |
|
Yes, it works! Looking forward to another test :)
pt., 13 gru 2024 o 07:09 Robert Chéramy ***@***.***>
napisał(a):
… How about cat /etc/cumulus/ports.conf; echo
This is an interesting option and I tested it: it works 👍
But the cumulus model has ~20 commands, and It makes the model somewhat
unaesthetic to add ;echo everywhere
About 20% of our models don't match on the line beginning, so I think it
should be OK, as long as the prompt is specific enough. The new prompt (prompt
***@***.***+:.*[#$] $/) could be more specific (.+ for the hostname and .*
for the unix path), so I will adapt it further.
@alchemyx <https://github.com/alchemyx> - could you check if the new
prompt works for you? If yes I'll make it more specific and let you try
again ;-)
—
Reply to this email directly, view it on GitHub
<#3332 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYNRMPYM5A4GWWFRWT7OY32FJ2Y5AVCNFSM6AAAAABS5UART6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBQGYZDINZTHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Michał Margula, ***@***.***
"W życiu piękne są tylko chwile" [Ryszard Riedel]
|
Making it more specific to avoid side effects as we cannot match on ^
Lot of the models are really low quality, but priority is that it was useful to someone, that someone maybe didn't have good idea what they are doing, but they got results that were sufficient to them. Prompt detection is incredibly fragile fundamentally, because we don't parse any data and data has lot of free form field, like descriptions, MOTD etc. The more relaxed our prompt is, the more often we get people misdiagnosing their problems like 'oxidized can't handle large config', because large config happened to have false positive prompt detection, causing desync between issuing command and assuming command output has ended. The order of preference should be.
We don't know if Cumulus guarantees new line at end of some files, and not others. It is very very likely, that any cumulus customer could open ticket and have them fix this one file so that Cumulus guarantees it has new line. I'm not against any solution, including poor prompt or ;echo after every command, but perosnally I'd just put ;echo after poorly generated file, and work on assumption not all of them are poorly generated. |
|
I can raise a ticket with Cumulus (Nvidia now) and ask them to fix that but I would need specifics. If we are tallking about My understanding is also that exec mode should also work. They run modified Debian 12 after all. |
|
Please try exec mode. I think your production device having new line in the file is more support that the example where we do not see new line would be accepted as bug. But agreed, we need to know more about it, like are we sure that case wasn't operator error? At any rate, I'm still perfectly OK with any solution. While my preferred solution is 'exec mode' and if not possible, strong prompt and ';echo' where needed. |
|
Exec mode will only work if there is no 2-step login (enable mode), so I think this is not an option that will work for every cumulus user. |
|
Usually including |
|
Hi, While I am happy to help not sure how can I migrate that module over to exec mode? In our setup we don't have 2-step login, we just create dedicated oxidized account and add it to groups like frr and netshow. |
Yes @robertcheramy explained this as well, we cannot migrate if we need |
... and add an echo to /etc/cumulus/ports.conf because in some cases, it has no new line at EOF.
|
@alchemyx - I've updated the prompt, and it will not work for you. ---
debug: true
input:
debug: trueI need the logs output of oxidized containing the last command sent (
On my devices, it looks like this: And the debug output file, including tail telling me (in German) that the file was overwritten: |
|
This time it worked, I got the full config output without any issues, /etc/oxidized/logs/ip-ssh: And there was no execution of cat /etc/cumulus/ports.conf, I assume because we have this in our config Which is further confirmed by oxidized.log snippet |
|
Great! So, to sum up:
|
This is to fix issue #3330 so multiline output won't break the module.
Pre-Request Checklist
rubocop --auto-correct)rake test)Description
Closes issue #3330