Skip to content

Conversation

@HollyGurza
Copy link
Contributor

@HollyGurza HollyGurza commented Apr 9, 2024

related task: T5660: add marker for last element of priority queues

Problem: In the priority queue we always have one empty root element after _get_commit_prio_queue therefore minimum length is 1 and we can't mark as last any deleted node, even when we try to delete only one node e.g. del vpn
also when we try to add\change one node, in the queue we have two element: one empty and one that we use.

This root element has state as COMMIT_STATE_UNCHANGED probably will be better to filter this element in _get_commit_prio_queue but i don't know how it affect feature not related with configdep.
also, all other unchanged nodes filtered before doCommit.

Change Summary

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)

Related PR(s)

Component(s) name

Proposed changes

How to test

/usr/libexec/vyos/tests/smoke/cli/test_vpn_l2tp.py

or try to delete node with a dependency e.g del vpn l2tp

set interfaces dummy dum0 address 203.0.113.1/32
set vpn ipsec interface dum0
commit
sudo -S swanctl -L

set vpn l2tp remote-access authentication local-users username foo password bar
set vpn l2tp remote-access authentication mode local
set vpn l2tp remote-access authentication protocols chap
set vpn l2tp remote-access client-ip-pool first range 10.200.100.100-10.200.100.110
set vpn l2tp remote-access description VPN - REMOTE
set vpn l2tp remote-access name-server 1.1.1.1
set vpn l2tp remote-access ipsec-settings authentication mode pre-shared-secret
set vpn l2tp remote-access ipsec-settings authentication pre-shared-secret SeCret
set vpn l2tp remote-access ipsec-settings ike-lifetime 8600
set vpn l2tp remote-access ipsec-settings lifetime 3600
set vpn l2tp remote-access outside-address 203.0.113.1
set vpn l2tp remote-access gateway-address 203.0.113.1
commit
sudo -S swanctl -L

del vpn l2tp
commit

sudo -S swanctl -L

Smoketest result

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

related task: T5660: add marker for last element of priority queues
@vyosbot vyosbot requested review from a team, c-po, dmbaturin, jestabro, sarthurdev, sever-sever and zdc and removed request for a team April 9, 2024 12:10
@jestabro
Copy link
Contributor

jestabro commented Apr 9, 2024

I agree that this is the better fix, as to not modify exiting legacy behavior; thanks for debugging this, @HollyGurza !

@c-po c-po merged commit ecc7310 into vyos:current Apr 9, 2024
@sever-sever
Copy link
Member

Do we need a backport?

@sever-sever
Copy link
Member

@Mergifyio backport sagitta

@mergify
Copy link

mergify bot commented Apr 29, 2024

backport sagitta

✅ Backports have been created

Details
  • Backport to branch sagitta not needed, change already in branch sagitta

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

4 participants