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

T6007: revise migration system #3692

Merged
merged 19 commits into from
Jun 27, 2024
Merged

T6007: revise migration system #3692

merged 19 commits into from
Jun 27, 2024

Conversation

jestabro
Copy link
Contributor

@jestabro jestabro commented Jun 20, 2024

Change Summary

This is a simplification of the migration system that provides a dramatic speed up of config migration, and requires little adjustment on the part of authors of migrations scripts.

Taking examples from the configtest directories, running time improvement is, for example:

basic-vyos:
old:

vyos@vyos:~$ time /usr/libexec/vyos/run-config-migration.py basic-vyos

real    0m12.416s
user    0m9.211s
sys     0m3.018s

new:

vyos@vyos:~$ time /usr/libexec/vyos/run-config-migrate.py basic-vyos 

real    0m0.796s
user    0m0.547s
sys     0m0.133s

firewall-big:
old:

vyos@vyos:~$ time /usr/libexec/vyos/run-config-migration.py firewall-big

real    37m55.363s
user    37m49.713s
sys     0m5.257s

new:

vyos@vyos:~$ time /usr/libexec/vyos/run-config-migrate.py config.test

real    0m27.387s
user    0m27.153s
sys     0m0.141s

The difference is impressive, but not surprising, as we eliminate the parse/write step traditionally required for each migration script, instead operating on a resident configtree under modifications by the body of the migration script. The scripts have been stripped of the boiler plate read/parse/write; the migrate.py module provides a wrapper to run a single 'script' for testing, as one is accustomed to.

Example of the changes for a recent migration script, ignoring white space:

> git diff -w current..revise-migration -- 1-to-2 
diff --git a/src/migration-scripts/openvpn/1-to-2 b/src/migration-scripts/openvpn/1-to-2
old mode 100755
new mode 100644
index 1f82a2128..b7b7d4c77
--- a/src/migration-scripts/openvpn/1-to-2
+++ b/src/migration-scripts/openvpn/1-to-2
@@ -17,25 +17,13 @@
 # Removes --cipher option (deprecated) from OpenVPN configs
 # and moves it to --data-ciphers for server and client modes
 
-import sys
-
 from vyos.configtree import ConfigTree
 
-if len(sys.argv) < 2:
-    print("Must specify file name!")
-    sys.exit(1)
-
-file_name = sys.argv[1]
-
-with open(file_name, 'r') as f:
-    config_file = f.read()
-
-config = ConfigTree(config_file)
-
+def migrate(config: ConfigTree) -> None:
     if not config.exists(['interfaces', 'openvpn']):
         # Nothing to do
-    sys.exit(0)
-else:
+        return
+
     ovpn_intfs = config.list_nodes(['interfaces', 'openvpn'])
     for        i in ovpn_intfs:
         # Remove 'encryption cipher' and add this value to 'encryption ncp-ciphers'
@@ -65,10 +53,3 @@ else:
 
             for c in ncp_ciphers:
                 config.set(ncp_cipher_path, value=c, replace=False)
-
-    try:
-        with open(file_name, 'w') as f:
-            f.write(config.to_string())
-    except OSError as e:
-        print("Failed to save the modified config: {}".format(e))
-        sys.exit(1)

The only subtlety in writing the form of migration module is that one now has to be aware of local scope, so if a traditionally global variable is updated in the migrate function, one will encounter the standard error.
UnboundLocalError: cannot access local variable ...
I found only ~10 scripts that had this issue, due to the nature of the translation, but don't expect it be an issue for authors of the new format.

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):
    Simplification, improved error reporting, and performance improvement.

Related Task(s)

Related PR(s)

Component(s) name

Proposed changes

How to test

Smoketest result

make testc:

DEBUG - ----------------------------------------------------------------------
DEBUG - Ran 37 tests in 1897.605s
DEBUG -
DEBUG - OK
DEBUG - vyos@vyos:~$ echo EXITCODE:$^V?
DEBUG - echo EXITCODE:$?
 INFO - Configtest finished successfully!

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 Jun 20, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

@jestabro jestabro force-pushed the revise-migration branch 2 times, most recently from cfec08a to 7e2f9fe Compare June 24, 2024 03:37
@jestabro jestabro marked this pull request as ready for review June 24, 2024 03:56
@jestabro jestabro requested a review from a team as a code owner June 24, 2024 03:56
@c-po
Copy link
Member

c-po commented Jun 24, 2024

https://github.com/vyos/vyos-1x/actions/runs/9639183334

PR smoketests report an error during HTTP API testing

@jestabro
Copy link
Contributor Author

jestabro commented Jun 24, 2024

Fixed: update to vyos-save-config.py had more restrictive perms when saving to file other than
/opt/vyatta/etc/config/config.boot
Original behavior restored.

@jestabro
Copy link
Contributor Author

This will need a rebase after:
#3724

@jestabro
Copy link
Contributor Author

jestabro commented Jun 26, 2024

Failure was for:
test_ospf_14_segment_routing_configuration
which we have seen intermittently fail; running local smoketests to confirm; configtests pass in local tests after rebase.

@c-po
Copy link
Member

c-po commented Jun 26, 2024

Failure was for: test_ospf_14_segment_routing_configuration which we have seen intermittently fail; running local smoketests to confirm; configtests pass in local tests after rebase.

This is a ghost issue which comes and goes and I have not yet identified the root cause - you may omit that error

Copy link

👍 VyOS CLI smoketests finished successfully!

Makefile Outdated Show resolved Hide resolved
@c-po c-po merged commit da1515c into vyos:current Jun 27, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants