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

mtr: T5658: Add VRF support for mtr (+ op_mode wrapper) #2435

Merged
merged 5 commits into from Nov 12, 2023

Conversation

bbabich
Copy link
Contributor

@bbabich bbabich commented Nov 4, 2023

Change Summary

  • Adds op_mode wrapper for mtr and all the good things that come along with it (like VRF support).
  • Re-introduces 'monitor traceroute' as requested by @c-po

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Component(s) name

  • mtr
  • monitor traceroute

Proposed changes

  • Add op_mode wrapper for mtr to enable ease of use + vrf functionality
  • Re-introduce 'monitor traceroute' back to op_mode as requested by @c-po

How to test

  • monitor traceroute 1.1.1.1
  • monitor traceroute vrf mgmt 1.1.1.1
  • mtr vrf blah 1.1.1.1

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@vyosbot vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, jestabro, sever-sever and c-po and removed request for a team November 4, 2023 11:40
src/op_mode/mtr.py Outdated Show resolved Hide resolved
src/op_mode/mtr.py Outdated Show resolved Hide resolved
src/op_mode/mtr.py Outdated Show resolved Hide resolved
src/op_mode/mtr.py Outdated Show resolved Hide resolved
@c-po
Copy link
Member

c-po commented Nov 4, 2023

In the past (VyOS 1.2) we had already a CLI for mtr under monitor traceroute which was removed in by 55d2ee8.

I have no strong bounds with monitor traceroute or mtr on the op-mode tree, but we probably should keep the well known monitor traceroute CLI tree as this was already there.

Original Implementation https://github.com/vyos/vyatta-op/blob/crux/templates/monitor/traceroute/node.tag/node.def
which was moved to XML in vyos-1x with f16633f but later got removed in 55d2ee8

Happy to re-add this with the nice CLI helper extension for 1.4 and 1.5 after the stray double whitespaces have been fixed and the CLI was changed to monitor traceroute

@bbabich
Copy link
Contributor Author

bbabich commented Nov 5, 2023

What you suggest makes sense to me too @c-po - I believe all you've requested is now complete.
Seems the doco doesn't even need updating :)
https://github.com/vyos/vyos-documentation/blob/master/docs/troubleshooting/index.rst#advanced-connectivity-tests

@bbabich bbabich requested a review from c-po November 5, 2023 10:14
@bbabich bbabich requested a review from c-po November 6, 2023 09:38
@c-po
Copy link
Member

c-po commented Nov 6, 2023

Possible completions:
  <hostname>            Traceroute route with mtr
  <x.x.x.x>
  <h:h:h:h:h:h:h:h>


cpo@LR1.wue3:~$ monitor traceroute 1.1.1.1

 Invalid option: 1.1.1.1


Possible completions:
  <Enter>               Execute the current command
  <nocomps>             mtr options


cpo@LR1.wue3:~$ monitor traceroute 1.1.1.1
mtr: Unknown host: traceroute

Was this tested locally?

@c-po
Copy link
Member

c-po commented Nov 9, 2023

monitor traceroute 1.1.1.1 now works with 709c578 - CLI completion helper is still broken.

…ls.network

Reduce amount of duplicated (3 times) code in op-mode scripts for ping,
traceroute and mtr.
Example: we should focus on JSON output and not expose XML and CSV.
@c-po c-po merged commit 45ac050 into vyos:current Nov 12, 2023
5 checks passed
@c-po
Copy link
Member

c-po commented Nov 12, 2023

@Mergifyio backport sagitta

Copy link

mergify bot commented Nov 12, 2023

backport sagitta

✅ Backports have been created

@bbabich
Copy link
Contributor Author

bbabich commented Nov 12, 2023

monitor traceroute 1.1.1.1 now works with 709c578 - CLI completion helper is still broken.

Thanks for finishing this off and apologies for not sorting it out myself. A better outcome with your additional cleanup/fixes too anyhow.

I'll do better next time...

c-po added a commit that referenced this pull request Nov 15, 2023
mtr: T5658: Add VRF support for mtr (+ op_mode wrapper) (backport #2435)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants