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.

Sign up for GitHub

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

Jump to bottom

op-mode: T6601: move "reset ip(v6) arp|neighbor" to "clear ip(v6) arp|neighbor" #3885

Closed
wants to merge 1 commit into from
Closed

op-mode: T6601: move "reset ip(v6) arp|neighbor" to "clear ip(v6) arp|neighbor" #3885

wants to merge 1 commit into from

Conversation

c-po
Copy link
Member

@c-po c-po commented Jul 27, 2024 •
edited
Loading

Change Summary

Move reset ip(v6) arp|neighbor to clear ip(v6) arp|neighbor

In addition create a build-time symbolic link for:
show ip arp -> show ip neighbors

This change also introduces new-style op-mode for IPv6 ND operations rather then directly calling iproute2

Parts should be manually backported to 1.4 sagitta to also provide the new CLI commands but keep the old ones for CLI stability reasons!

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): Op-mode CLI change

Related Task(s)

Related PR(s)

How to test

cpo@LR2.wue3:~$ clear ip arp
Possible completions:
  address               Reset ARP cache for an IPv4 address
  interface             Reset ARP cache for interface
  table                 Flush entire ARP cache

cpo@LR2.wue3:~$ clear ipv6 neighbors
Possible completions:
  address               Reset ND cache for an IPv6 address
  interface             Reset IPv6 ND cache for interface
  table                 Flush entire IPv6 ND cache

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

@c-po
op-mode: T6601: move "reset ip(v6) arp|neighbor" to "clear ip(v6) arp…
55f7e07 
…|neighbor"

In addition create a build-time symbolic link for:
  "show ip arp" -> "show ip neighbors"
@c-po c-po requested a review from a team as a code owner July 27, 2024 08:03
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 27, 2024 •
edited
Loading

👍
No issues in PR Title / Commit Title

@github-actions GitHub Actions
Copy link

❌
warning: Unused import os in smoketest/scripts/cli/test_interfaces_l2tpv3.py:17.

@c-po
Copy link
Member Author

c-po commented Jul 27, 2024

@Mergifyio backport circinus

@c-po c-po enabled auto-merge July 27, 2024 08:04
@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 27, 2024

backport circinus

🟠 Waiting for conditions to match

  • merged [📌 backport requirement]

@c-po c-po mentioned this pull request Jul 27, 2024
Closed
1 task
@github-actions GitHub Actions
Copy link

CI integration 👍 passed!

Details

CI logs

  • CLI Smoketests 👍 passed
  • Config tests 👍 passed
  • RAID1 tests 👍 passed

sever-sever
sever-sever approved these changes Jul 28, 2024
View reviewed changes
op-mode-definitions/clear-ip.xml.in
<children>
<tagNode name="address">
<properties>
<help>Reset ARP cache for an IPv4 address</help>
Copy link
Member

Choose a reason for hiding this comment

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

Should there also be a clear ARP cache?

op-mode-definitions/clear-ip.xml.in
<script>${vyos_completion_dir}/list_interfaces</script>
</completionHelp>
</properties>
<command>sudo ${vyos_op_scripts_dir}/neighbor.py reset --family inet --interface "$5"</command>
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a clear option instead of reset

dmbaturin
dmbaturin reviewed Jul 29, 2024
View reviewed changes
Copy link
Member

@dmbaturin dmbaturin left a comment

Choose a reason for hiding this comment

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

Top level op mode words have specific meanings in VyOS. clear is reserved for completely non-disruptive operations: clearing the screen, clearing rule counters... reset is for locally-disruptive operations.

Since removing a neighbor table entry can lead to lost packets or increased latencies for new connections, it doesn't qualify for clear, which is why it was called reset.

I'd prefer to keep that terminology consistent. BGP neighbors in Cisco IOS are also under clear, but we don't plan to mimic that, do we? ;)

@c-po c-po closed this Jul 30, 2024
auto-merge was automatically disabled July 30, 2024 06:11

Pull request was closed

@c-po c-po deleted the op-mode-clear-ip branch August 11, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmbaturin dmbaturin dmbaturin left review comments

@sever-sever sever-sever sever-sever approved these changes

@sarthurdev sarthurdev Awaiting requested review from sarthurdev sarthurdev was automatically assigned from vyos/reviewers

@zdc zdc Awaiting requested review from zdc zdc was automatically assigned from vyos/reviewers

@jestabro jestabro Awaiting requested review from jestabro jestabro was automatically assigned from vyos/reviewers

@nicolas-fort nicolas-fort Awaiting requested review from nicolas-fort nicolas-fort was automatically assigned from vyos/reviewers

Assignees

@c-po c-po

Labels
backport current
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@c-po @dmbaturin @sever-sever