Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-27602

Automatic creation of working directories in mtr

Details

    Description

      Now we have problems with automatically checking the correct operation of the Galera cluster when changing various paths to innodb files, to binary logs, and so on. Checking each such option requires the development of complex tests that stop the entire cluster, manually duplicate the bootstrap process with non-standard paths, then destroy everything and restart the cluster from a .test file with previous settings, which is very inconvenient and potentially masks failures that could be detected on the final stage of the check (if it was done inside mtr).

      To solve this problem and create such tests without significant alteration of .test files, we need automatic creation of working directories in mtr - at least for testing a cluster with non-standard path settings. It can also be useful for testing mariabackup and even innodb and maybe other subsystems.

      Attachments

        Issue Links

          Activity

            sysprg Julius Goryavsky added a comment - Proposed solution: https://github.com/MariaDB/server/commit/6777c260c22e16fb9a76a60f124c2d9337d04585

            I don't see much of a point. It doesn't "greatly simplify" anything. The patch is

             mysql-test/lib/mtr_cases.pm  |   9 ++++
             mysql-test/mysql-test-run.pl | 126 ++++++++++++++++++++++++++++++++++++++++---
             2 files changed, 129 insertions(+), 6 deletions(-)
            

            And it allows to write

            --- /dev/null
            +++ b/mysql-test/suite/galera/t/galera_sst_mariabackup3.dirs
            @@ -0,0 +1,2 @@
            +${MYSQLTEST_VARDIR}/mysqld.1/node1
            +${MYSQLTEST_VARDIR}/mysqld.2/node2
            

            instead of

            --- /dev/null
            +++ b/mysql-test/suite/galera/t/galera_sst_mariabackup3.sh
            @@ -0,0 +1,2 @@
            +mkdir -p ${MYSQLTEST_VARDIR}/mysqld.1/node1 
            +mkdir -p ${MYSQLTEST_VARDIR}/mysqld.2/node2
            

            which doesn't strike me as a great simplification worth that much new code in the mtr.
            You can even do

            rm -rf ${MYSQLTEST_VARDIR}/mysqld.1/node1  ${MYSQLTEST_VARDIR}/mysqld.2/node2
            mkdir -p ${MYSQLTEST_VARDIR}/mysqld.1/node1  ${MYSQLTEST_VARDIR}/mysqld.2/node2
            

            or

            rm -rf ${MYSQLTEST_VARDIR}/mysqld.1/node1  ${MYSQLTEST_VARDIR}/mysqld.2/node2
            cp -rf ${MYSQLTEST_VARDIR}/mysqld.1/node1  ${MYSQLTEST_VARDIR}/mysqld.2/node2
            

            whatever you need. There are tests that already do that. And there are only few of them, again, showing that this feature isn't needed in the mtr.

            serg Sergei Golubchik added a comment - I don't see much of a point. It doesn't "greatly simplify" anything. The patch is mysql-test/lib/mtr_cases.pm | 9 ++++ mysql-test/mysql-test-run.pl | 126 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 129 insertions(+), 6 deletions(-) And it allows to write --- /dev/null +++ b/mysql-test/suite/galera/t/galera_sst_mariabackup3.dirs @@ -0,0 +1,2 @@ +${MYSQLTEST_VARDIR}/mysqld.1/node1 +${MYSQLTEST_VARDIR}/mysqld.2/node2 instead of --- /dev/null +++ b/mysql-test/suite/galera/t/galera_sst_mariabackup3.sh @@ -0,0 +1,2 @@ +mkdir -p ${MYSQLTEST_VARDIR}/mysqld.1/node1 +mkdir -p ${MYSQLTEST_VARDIR}/mysqld.2/node2 which doesn't strike me as a great simplification worth that much new code in the mtr. You can even do rm -rf ${MYSQLTEST_VARDIR} /mysqld .1 /node1 ${MYSQLTEST_VARDIR} /mysqld .2 /node2 mkdir -p ${MYSQLTEST_VARDIR} /mysqld .1 /node1 ${MYSQLTEST_VARDIR} /mysqld .2 /node2 or rm -rf ${MYSQLTEST_VARDIR} /mysqld .1 /node1 ${MYSQLTEST_VARDIR} /mysqld .2 /node2 cp -rf ${MYSQLTEST_VARDIR} /mysqld .1 /node1 ${MYSQLTEST_VARDIR} /mysqld .2 /node2 whatever you need. There are tests that already do that. And there are only few of them, again, showing that this feature isn't needed in the mtr.
            sysprg Julius Goryavsky added a comment - - edited

            serg Thank you very much for the review, but let me disagree with the possibility of using shell scripts without writing dozens of lines of additional code in test files, for the following reasons:

            1) Creating directories from shell scripts will do nothing to ensure that their paths would be substituted in the bootstrap process parameters (see changes around the mysql_install_db() call in the mysql_server_start code, especially in the second commit);

            2) Currently we do not have the ability to pass parameters to the bootstrap stage from .cnf files;

            3) Taking into account (1) and (2) if the base for the test should be created in such a way that some of the innodb files would be in non-standard directories, then shell scripts will not help us - they will be executed with standard parameters in the bootstrap procedure and the created database will not be located in non-standard directories, even if the directories themselves have been created in advance;

            4) Because of (3) in the case of Galera, we don't just need to create directories, but we need to:

            • Sequentially stop all cluster nodes;
            • Erase working files like gcache & etc sequentially for each of them;
            • In the test code, create configuration files with a non-standard directory location and restart the server for each of the nodes by passing it the paths to these configuration files;
            • Run tests;
            • Stop the nodes sequentially again;
            • Restart them again on the standard configuration (which was at the time of the start of the test) - otherwise the verification process in mtr will fail due to a state mismatch at the beginning and at the end of the test;
            • Well, create / destroy non-standard directories using shell scripts;
              As a result, these are dozens of lines of code in a .test file.

            5) Paragraph (4) actually cancels all mechanisms for checking state discrepancy in mtr (state difference after test completion), this masks errors and assigns this check to the test developer;

            6) Using shell scripts immediately ties us to Linux, but we also officially support Windows (not for Galera, but, for example, this is true for mariabackup).

            sysprg Julius Goryavsky added a comment - - edited serg Thank you very much for the review, but let me disagree with the possibility of using shell scripts without writing dozens of lines of additional code in test files, for the following reasons: 1) Creating directories from shell scripts will do nothing to ensure that their paths would be substituted in the bootstrap process parameters (see changes around the mysql_install_db() call in the mysql_server_start code, especially in the second commit); 2) Currently we do not have the ability to pass parameters to the bootstrap stage from .cnf files; 3) Taking into account (1) and (2) if the base for the test should be created in such a way that some of the innodb files would be in non-standard directories, then shell scripts will not help us - they will be executed with standard parameters in the bootstrap procedure and the created database will not be located in non-standard directories, even if the directories themselves have been created in advance ; 4) Because of (3) in the case of Galera, we don't just need to create directories, but we need to: Sequentially stop all cluster nodes; Erase working files like gcache & etc sequentially for each of them; In the test code, create configuration files with a non-standard directory location and restart the server for each of the nodes by passing it the paths to these configuration files; Run tests; Stop the nodes sequentially again; Restart them again on the standard configuration (which was at the time of the start of the test) - otherwise the verification process in mtr will fail due to a state mismatch at the beginning and at the end of the test; Well, create / destroy non-standard directories using shell scripts; As a result, these are dozens of lines of code in a .test file. 5) Paragraph (4) actually cancels all mechanisms for checking state discrepancy in mtr (state difference after test completion), this masks errors and assigns this check to the test developer; 6) Using shell scripts immediately ties us to Linux, but we also officially support Windows (not for Galera, but, for example, this is true for mariabackup).

            serg P.S. I also forgot to say:

            (7) All manually made lines for restarting servers on nodes will need to be constantly supported as the system develops - if some new files are added to innodb, the names of old ones change, etc. - tests for Galera will be broken in a way that is not obvious to anyone (after a some time) - while with the proposed changes, the bootstrap process through mysql_install_db() will do everything correctly after innodb changes - and it will not be necessary to change a single line in the .test files for Galera.

            sysprg Julius Goryavsky added a comment - serg P.S. I also forgot to say: (7) All manually made lines for restarting servers on nodes will need to be constantly supported as the system develops - if some new files are added to innodb, the names of old ones change, etc. - tests for Galera will be broken in a way that is not obvious to anyone (after a some time) - while with the proposed changes, the bootstrap process through mysql_install_db() will do everything correctly after innodb changes - and it will not be necessary to change a single line in the .test files for Galera.

            You're right that shell scripts are invoked too late for bootstrap. This is easily fixed by moving their invocation few lines up.

            After doing that and converting all dirs files to shell scripts all your tests from the commit 4e2da16802f (fix for MDEV-27524) pass.

            $ git diff --shortstat @
             23 files changed, 50 insertions(+), 172 deletions(-)
            

            So, all tests (that used to use .dirs) pass, mtr is ~120 lines smaller. Looks like a good thing to me.

            serg Sergei Golubchik added a comment - You're right that shell scripts are invoked too late for bootstrap. This is easily fixed by moving their invocation few lines up. After doing that and converting all dirs files to shell scripts all your tests from the commit 4e2da16802f (fix for MDEV-27524 ) pass. $ git diff --shortstat @ 23 files changed, 50 insertions(+), 172 deletions(-) So, all tests (that used to use .dirs) pass, mtr is ~120 lines smaller. Looks like a good thing to me.

            It's good to improve the mtr! But every improvement or a bug fix must be accompanied by a test case. And then there's a question whether the goal can be achieved simpler.

            The improvement related to .dirs files — I believe it's not needed, because -master.sh files achieve the same goal, but simpler.

            The bug, that they're run too late — you are right, this is a bug, and it needs to be fixed. By moving the rebootstrap code down.

            The bug that mysql_install_db() function in mtr does not notice that command line option overrides the value from the cnf file — you are right again, this is a bug, needs to be fixed too. For example, like this:

            --- a/mysql-test/mysql-test-run.pl
            +++ b/mysql-test/mysql-test-run.pl
            @@ -3166,6 +3166,7 @@ sub mysql_install_db {
                 {
                   my $sql_dir= dirname($path_sql);
                   # Use the mysql database for system tables
            +      mtr_tofile($bootstrap_sql_file, "create database if not exists mysql;\n");
                   mtr_tofile($bootstrap_sql_file, "use mysql;\n");
             
                   # Add the offical mysql system tables
            

            After fixing these two bugs all your new galera test cases pass.

            If you know of any more mtr bugs, please, show test cases that trigger them and we can discuss how to fix them.

            serg Sergei Golubchik added a comment - It's good to improve the mtr! But every improvement or a bug fix must be accompanied by a test case. And then there's a question whether the goal can be achieved simpler. The improvement related to .dirs files — I believe it's not needed, because -master.sh files achieve the same goal, but simpler. The bug, that they're run too late — you are right, this is a bug, and it needs to be fixed. By moving the rebootstrap code down. The bug that mysql_install_db() function in mtr does not notice that command line option overrides the value from the cnf file — you are right again, this is a bug, needs to be fixed too. For example, like this: --- a/mysql-test/mysql-test-run.pl +++ b/mysql-test/mysql-test-run.pl @@ -3166,6 +3166,7 @@ sub mysql_install_db { { my $sql_dir= dirname($path_sql); # Use the mysql database for system tables + mtr_tofile($bootstrap_sql_file, "create database if not exists mysql;\n"); mtr_tofile($bootstrap_sql_file, "use mysql;\n"); # Add the offical mysql system tables After fixing these two bugs all your new galera test cases pass. If you know of any more mtr bugs, please, show test cases that trigger them and we can discuss how to fix them.

            People

              sysprg Julius Goryavsky
              sysprg Julius Goryavsky
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.