Make age for reused test configurable #692

matsduf · 2021-01-04T17:20:44Z

When creating a new test through RPCAPI it will first check if there is a recent test with the same test parameters. If so, and that test is not too old, a new test will not be created, instead the test result for that recent test will be reused.

As it is now, "not too old" is hard-coded to 10 minutes. That is probably a good choice in most cases, but sometimes that is too short and sometimes too long. This PR makes that configurable.

As a side effect, this PR sorts the methods in the "[ZONEMASTER]" section from the ini file in lib/Zonemaster/Backend/Config.pm and also adds POD documentation to those methods.

Updated 2021-05-17:

How to test this PR

update the value for age_reuse_test in backend_config.ini
restart daemons
test a domain
redo the test before the new time value (it should give the previous results, check the date)
redo the test after the time limit (a new test should be created, check the dates)

* Rearranges all methods connected to the [ZONEMASTER] section in the ini file so that they come after each other. * Adds POD documentation to all methods connected to the [ZONEMASTER] section. * No changes in code or logic.

Adds a new key, "age_reuse_previous_test", to the ini file that sets the time before a new test is executed, instead of reusing the previous test (of the same domain and with the same parameters). It replaces a hard coded value.

mattias-p · 2021-01-05T12:05:29Z

docs/Configuration.md

+Positiv integer (in seconds) for how old a previous test of the same
+zone name and parameters must be before we start a new test. Internally
+the value is converted to whole minutes. If the conversion results in
+zero minutes, then the default value (600 seconds) is used.


Why is the unit seconds here, rather than minutes?

I felt that seconds was a better unit, especially since other keys take seconds. I would rather turn it around and make it seconds all the way through. In the MySQL adapter it is converted to seconds before use.

If this is merged I will create a second PR where the time unit is seconds all the way through.

But I will not insist. If minutes is better I can take that.

I see. Uniform units sure is a nice feature. Seconds it is. If we could also get rid of the rounding, that would be great!

mattias-p · 2021-01-05T12:13:59Z

lib/Zonemaster/Backend/Config.pm

+
+=head3 INPUT
+
+'max_zonemaster_execution_time' from [ZONEMASTER] section in ini file.


Maybe link to the property in Configuration.md?

Good suggestion, but those links will be broken until the updated Configuration.pm has been merged to master branch. I guess that is OK.

matsduf · 2021-03-03T12:49:02Z

@mattias-p and @PNAX, can you review?

ghost

See my comments.
I also ran some tests, and this seems to work.

ghost · 2021-03-03T17:56:24Z

lib/Zonemaster/Backend/Config.pm

+    my $val = $self->{cfg}->val( 'ZONEMASTER', 'age_reuse_previous_test' );
+    $val = ($val > 0) ? $val : 0;
+    $val = int ( ($val / 60) + 0.5); # in minutes
+    return ($val)? $val : 10;


A style remark: there is a missing space before the ?.

Fixed. Some other PR can make it consistent in the module.

ghost · 2021-03-03T18:39:51Z

share/backend_config.ini

 number_of_processes_for_batch_testing    = 20
-
+#age_reuse_previous_test=600
 #


I think the line 23 should be blank to make a clear separation with the following WARNING clause that is related to maximal_number_of_retries.

Fixed and made the warning clearer. For how long should maximal_number_of_retries be considered to be experimental?

No idea... I found PR #544 where this parameter has been added. I understand that this option has not been fully tested (as the warning says) and that there might be issues with parallelism.

matsduf · 2021-03-03T22:12:30Z

@PNAX, please re-review.

mattias-p · 2021-01-12T11:52:57Z

lib/Zonemaster/Backend/Config.pm

+
+=head3 RETURNS
+
+Integer (number of seconds)


Do you consider validation to be out of scope for this PR? Otherwise it would make sense to add validation for this to the load_config method.

It is out of scope for this PR to add validation for anything execept age_reuse_previous_test, but that is a good suggestion. For age_reuse_previous_test the validation is done such as if the value is not numeric and positive, it will be ignored. Should it croak instead?

I do not understand how to used load_config method for validation.

On second thought it's probably better to leave the validation out for now and deal with it in #685.

mattias-p · 2021-01-12T11:57:53Z

lib/Zonemaster/Backend/Config.pm

+
+=head3 RETURNS
+
+Positive integer (default 600).


This default doesn't look right. Maybe you meant to add it to MaxZonemasterExecutionTime?

Good catch. I will fix.

mattias-p · 2021-05-21T10:04:15Z

Release testing for v2021.1

I verified this using the commands below on CentOS 7 and CentOS 8.

sudo sed -i '/\bage_reuse_previous_test\b/ s/=.*/= 20/' /etc/zonemaster/backend_config.ini
sudo systemctl restart zm-rpcapi
sudo systemctl restart zm-testagent
zmtest example > /dev/null
sleep 10
zmtest example > /dev/null
sleep 20
zmtest example > /dev/null
# Verify that the two first zmtest commands report the same testid, but the third one reports a different one

matsduf added 2 commits January 1, 2021 15:45

Documents and rearranges

c7c8077

* Rearranges all methods connected to the [ZONEMASTER] section in the ini file so that they come after each other. * Adds POD documentation to all methods connected to the [ZONEMASTER] section. * No changes in code or logic.

Make age for reused test configurable

70fd39d

Adds a new key, "age_reuse_previous_test", to the ini file that sets the time before a new test is executed, instead of reusing the previous test (of the same domain and with the same parameters). It replaces a hard coded value.

matsduf added the A-Code label Jan 4, 2021

matsduf added this to the v2021.1 milestone Jan 4, 2021

matsduf requested review from a user and mattias-p January 4, 2021 17:20

mattias-p reviewed Jan 5, 2021

View reviewed changes

Updates documentation with links to Configuration.md

1a69fe1

matsduf requested a review from mattias-p January 5, 2021 14:25

ghost reviewed Mar 3, 2021

View reviewed changes

Editorial updates

1b29784

matsduf requested a review from a user March 3, 2021 22:11

ghost previously approved these changes Mar 4, 2021

View reviewed changes

mattias-p reviewed Mar 4, 2021

View reviewed changes

Corrects documentation

93b37f6

matsduf dismissed ghost ’s stale review via 93b37f6 March 4, 2021 10:50

mattias-p approved these changes Mar 4, 2021

View reviewed changes

matsduf added the T-Feature Type: New feature in software or test case description label Mar 4, 2021

matsduf merged commit c2a2608 into zonemaster:develop Mar 4, 2021

matsduf deleted the add-reuse-test-configuration branch March 4, 2021 13:25

ghost mentioned this pull request Mar 16, 2021

Fix unitialized value #720

Merged

mattias-p mentioned this pull request May 3, 2021

Stop rounding test shelflife to whole minutes #756

Merged


		=head3 INPUT

		'max_zonemaster_execution_time' from [ZONEMASTER] section in ini file.

Make age for reused test configurable #692

Make age for reused test configurable #692

Uh oh!

Conversation

matsduf commented Jan 4, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to test this PR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matsduf commented Mar 3, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Mar 3, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matsduf commented Mar 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattias-p commented May 21, 2021

Release testing for v2021.1

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

matsduf commented Jan 4, 2021 •

edited by ghost

Loading

ghost Mar 3, 2021 •

edited by ghost

Loading