Skip to content

Conversation

bneradt
Copy link
Contributor

@bneradt bneradt commented Oct 3, 2025

This changes the remap_acl.test.py test to use config reloads instead of
spinning up new ATS insteads for every TestRun. Now that we've reduced
our reload scanning interval via #12541, these reloads in test are
pretty quick.

Before:

./autest.sh --sandbox /tmp/sb --clean=none -f remap_acl  171.62s user 57.13s system 52% cpu 7:12.57 total

After:

./autest.sh --sandbox /tmp/sb --clean=none -f remap_acl  14.39s user 10.78s system 19% cpu 2:09.23 total

So the time to run this test goes from about 7 minutes to about 2 minutes.

@bneradt bneradt added this to the 10.2.0 milestone Oct 3, 2025
@bneradt bneradt requested a review from masaori335 October 3, 2025 20:01
@bneradt bneradt self-assigned this Oct 3, 2025
@bneradt bneradt force-pushed the remap_acl_speedup branch from 8d5e7cf to 4aee8e5 Compare October 3, 2025 21:56
@bneradt bneradt requested a review from brbzull0 October 3, 2025 22:14
@bneradt
Copy link
Contributor Author

bneradt commented Oct 3, 2025

Adding @brbzull0 for the config update logic. I ran into bugs when I updated the proxy.config.config_update_interval_ms - the wait_for_cache and some other configs didn't get set appropriately. This fixes the update logic so that both are set now. I kept this as a separate commit.

@bneradt
Copy link
Contributor Author

bneradt commented Oct 3, 2025

Adding @brbzull0 for the config update logic. I ran into bugs when I updated the proxy.config.config_update_interval_ms - the wait_for_cache and some other configs didn't get set appropriately. This fixes the update logic so that both are set now. I kept this as a separate commit.

This is getting more complicated because some tests were broken due to the longer reload interval. I will hold off on this remap_reload update until I fix the other issues in a separate PR:
#12541

In the meantime, I'll make this as a draft and rebase once that lands.

@bneradt bneradt marked this pull request as draft October 3, 2025 22:53
@bneradt bneradt force-pushed the remap_acl_speedup branch 3 times, most recently from c0e7ec8 to e92cc3f Compare October 4, 2025 02:49
@masaori335 masaori335 linked an issue Oct 6, 2025 that may be closed by this pull request
@masaori335
Copy link
Contributor

/cc @mattyw

This changes the remap_acl.test.py test to use config reloads instead of
spinning up new ATS insteads for every TestRun. Now that we've reduced
our reload scanning interval via apache#12541, these reloads in test are
pretty quick.

Before:
```
./autest.sh --sandbox /tmp/sb --clean=none -f remap_acl  171.62s user 57.13s system 52% cpu 7:12.57 total
```

After:
```
./autest.sh --sandbox /tmp/sb --clean=none -f remap_acl  14.39s user 10.78s system 19% cpu 2:09.23 total
```
@bneradt bneradt force-pushed the remap_acl_speedup branch from e92cc3f to 88b5a24 Compare October 6, 2025 19:58
@bneradt bneradt marked this pull request as ready for review October 6, 2025 19:59
Comment on lines +129 to +147
if call_reload:
#
# Update the ATS configuration.
#
tr = Test.AddTestRun("Change the ATS configuration")
p = tr.Processes.Default
p.Command = (
f'traffic_ctl config set proxy.config.http.connect_ports {server_port} && '
f'traffic_ctl config set proxy.config.url_remap.acl_behavior_policy {self._acl_behavior_policy}')

p.Env = ts.Env
tr.StillRunningAfter = ts

remap_cfg_path = os.path.join(ts.Variables.CONFIGDIR, 'remap.config')
ip_allow_path = os.path.join(ts.Variables.CONFIGDIR, 'ip_allow.yaml')
p.Setup.Lambda(
lambda: update_config_file(
remap_cfg_path, '\n'.join(remap_config_lines), ip_allow_path, '\n'.join(self._ip_allow_lines)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the heart of this patch:

  • For the first test run, start ATS via MakeATSProcess (see the else: below this).
  • For subsequent test runs, update the config and do a config reload instead of starting a new test process. Doing a config reload is faster than starting a brand new ATS process for every test run.

@maskit maskit self-requested a review October 7, 2025 21:21
Copy link
Contributor

@masaori335 masaori335 left a comment

Choose a reason for hiding this comment

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

Cool! I like this reloading idea!

@bneradt bneradt merged commit 9775c61 into apache:master Oct 8, 2025
15 checks passed
@bneradt bneradt deleted the remap_acl_speedup branch October 8, 2025 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reduce the execution time of Remap ACL AuTest

2 participants