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

configdep: T5839: remove trivially redundant config dependency calls #2659

Merged
merged 2 commits into from Feb 29, 2024

Conversation

jestabro
Copy link
Contributor

@jestabro jestabro commented Dec 19, 2023

Change Summary

NOTE: this requires vyos/vyatta-cfg#74

NOTE: this should not be backported until it has had a bit of time in 1.5

Remove redundant calls to config dependencies in two steps:
(1) remove trivial redundancies produced by multiple calls to set_dependents within a given config-mode script
(2) add a global list of dependencies under vyos-configd and execute in that context, when script is called with vyos-configd support

The case (1) is necessary as set_dependents can be called conditionally; a simple example, used below for demonstration, is that of firewall.py wherein dependents ["system_conntrack", "nat", "policy_route"] are added if 'group' is present (group-resync), and ["system_conntrack"] is added unconditionally.

An important consideration is that adjusting the script logic to avoid redundancies is fragile and prone to error and NOT recommended, in favor of a proper solution, provided here.

The case (2) is necessary as otherwise, scripts process local dependency lists without knowledge of other script calls, for examples, firewall 'group-resync' will call ["system_conntrack", "nat", "policy_route"], which will incur nat.py calling ["system_conntrack"] subsequently. Again, one should not need to handle the logic of execution from with the logic of dependency.

Example below:

set firewall group address-group AG address '198.122.100.137'
set nat source rule 10 outbound-interface name 'eth1'
set nat source rule 10 source group address-group 'AG'
set nat source rule 10 translation address 'masquerade'

Without any redundancy pruning:

--- including set_dependents calls:

vyos@vyos# commit
set_dependents: caller nat, dependents ['system_conntrack']
calling: system_conntrack
set_dependents: caller firewall, dependents ['system_conntrack', 'nat', 'policy_route']
set_dependents: caller firewall, dependents ['system_conntrack', 'nat', 'policy_route', 'system_conntrack']
calling: system_conntrack
calling: nat
set_dependents: caller nat, dependents ['system_conntrack']
calling: system_conntrack
calling: policy_route
calling: system_conntrack

--- limiting to the resulting script calls:

calling: system_conntrack
calling: system_conntrack
calling: nat
calling: system_conntrack
calling: policy_route
calling: system_conntrack

With intra-script dependency pruning:

--- including set_dependents calls

vyos@vyos# commit
set_dependents: caller nat, dependents ['system_conntrack']
calling: system_conntrack
set_dependents: caller firewall, dependents ['system_conntrack', 'nat', 'policy_route']
set_dependents: caller firewall, dependents ['nat', 'policy_route', 'system_conntrack']
calling: nat
set_dependents: caller nat, dependents ['system_conntrack']
calling: system_conntrack
calling: policy_route
calling: system_conntrack

--- with result:

calling: system_conntrack
calling: nat
calling: system_conntrack
calling: policy_route
calling: system_conntrack

With global dependency list:

--- scripts set dependencies as before:

vyos@vyos# commit
set_dependents: caller nat, dependents ['system_conntrack']
set_dependents: caller firewall, dependents ['system_conntrack', 'nat', 'policy_route']
set_dependents: caller firewall, dependents ['nat', 'policy_route', 'system_conntrack']

--- dependents are called under vyos-configd:

journalctl -u vyos-configd|grep calling
...: calling: nat
...: calling: policy_route
...: calling: system_conntrack

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)

vyos/vyatta-cfg#74

Component(s) name

Proposed changes

How to test

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

@jestabro jestabro self-assigned this Dec 19, 2023
@vyosbot vyosbot requested review from a team, dmbaturin, sarthurdev, zdc, sever-sever and c-po and removed request for a team December 19, 2023 20:04
@jestabro jestabro force-pushed the remove-trivial-redundancies branch 2 times, most recently from b64ae91 to faaf6dd Compare February 28, 2024 13:58
@jestabro jestabro marked this pull request as ready for review February 28, 2024 17:24
@vyosbot vyosbot requested a review from a team February 28, 2024 17:24
@jestabro jestabro force-pushed the remove-trivial-redundancies branch 2 times, most recently from 172b33a to 348a58f Compare February 28, 2024 20:04
@jestabro jestabro merged commit a2e1c85 into vyos:current Feb 29, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants