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

MTR tests fail when built without WSREP

Details

    Description

      When building with -DWITH_WSREP=OFF, files required for MTR tests are excluded and several tests fail. This is cause by a recent commit 7b44d0ba which attempted to solve MDEV-23230.

      Failures can be seen in GitLab CI, or by building/installing/testing in the fedora:latest Docker container with the -DWITH_WSREP=OFF CMake flag enabled.

      It appears this issue happened before (see !2196 and MDEV-28782) where WITH_WSREP=0 cause MTR tests to break.

      Perhaps there should be a CI build with WSREP/Galera disabled to catch this mistake sooner.

      Attachments

        Issue Links

          Activity

            robinnew Robin Newhouse created issue -

            I suggest the following change in the CMake install macros which will avoid excluding the MTR test .inc files. This fixes the (relevant) failing tests. I'll make a PR if this change is appropriate.

            diff --git a/cmake/install_macros.cmake b/cmake/install_macros.cmake
            index 7814f1dbeac..fba0bd03dac 100644
            --- a/cmake/install_macros.cmake
            +++ b/cmake/install_macros.cmake
            @@ -266,7 +266,7 @@ SET(DEBUGBUILDDIR "${BINARY_PARENTDIR}/debug" CACHE INTERNAL "Directory of debug
             FUNCTION(INSTALL_MYSQL_TEST from to)
               IF(INSTALL_MYSQLTESTDIR)
                 IF(NOT WITH_WSREP)
            -      SET(EXCL_GALERA "(suite/(galera|wsrep|sys_vars/[rt]/(sysvars_)?wsrep).*|include/((w.*)?wsrep.*|.*galera.*)\\.inc|std_data/(galera|wsrep).*)")
            +      SET(EXCL_GALERA "(suite/(galera|wsrep|sys_vars/[rt]/(sysvars_)?wsrep).*|std_data/(galera|wsrep).*)")
                 ELSE()
                   SET(EXCL_GALERA "^DOES_NOT_EXIST$")
                 ENDIF()
            

            robinnew Robin Newhouse added a comment - I suggest the following change in the CMake install macros which will avoid excluding the MTR test .inc files. This fixes the (relevant) failing tests. I'll make a PR if this change is appropriate. diff --git a/cmake/install_macros.cmake b/cmake/install_macros.cmake index 7814f1dbeac..fba0bd03dac 100644 --- a/cmake/install_macros.cmake +++ b/cmake/install_macros.cmake @@ -266,7 +266,7 @@ SET(DEBUGBUILDDIR "${BINARY_PARENTDIR}/debug" CACHE INTERNAL "Directory of debug FUNCTION(INSTALL_MYSQL_TEST from to) IF(INSTALL_MYSQLTESTDIR) IF(NOT WITH_WSREP) - SET(EXCL_GALERA "(suite/(galera|wsrep|sys_vars/[rt]/(sysvars_)?wsrep).*|include/((w.*)?wsrep.*|.*galera.*)\\.inc|std_data/(galera|wsrep).*)") + SET(EXCL_GALERA "(suite/(galera|wsrep|sys_vars/[rt]/(sysvars_)?wsrep).*|std_data/(galera|wsrep).*)") ELSE() SET(EXCL_GALERA "^DOES_NOT_EXIST$") ENDIF()
            robinnew Robin Newhouse made changes -
            Field Original Value New Value
            Summary MTR tests fail when build without WSREP MTR tests fail when built without WSREP
            dlenski Daniel Lenski (Inactive) added a comment - - edited

            When building with -DWITH_WSREP=OFF, files required for MTR tests are excluded and several tests fail. This is cause by a recent commit 7b44d0ba which attempted to solve MDEV-23230.

            Specifically, it is this specific change which causes the failure, right robinnew?

            Perhaps there should be a CI build with WSREP/Galera disabled to catch this mistake sooner.

            Agreed.

            dlenski Daniel Lenski (Inactive) added a comment - - edited When building with -DWITH_WSREP=OFF, files required for MTR tests are excluded and several tests fail. This is cause by a recent commit 7b44d0ba which attempted to solve MDEV-23230 . Specifically, it is this specific change which causes the failure, right robinnew ? Perhaps there should be a CI build with WSREP/Galera disabled to catch this mistake sooner. Agreed.
            danblack Daniel Black made changes -
            Assignee Daniel Black [ danblack ]

            Specifically, it is this specific change which causes the failure

            @dlenski Correct

            robinnew Robin Newhouse added a comment - Specifically, it is this specific change which causes the failure @dlenski Correct
            danblack Daniel Black added a comment - - edited

            Robin, I think you patch is right for the fault observed in gitlab, please do a PR.

            Daniel, there might be different problem here. wsrep includes in sql/*.h (for non-wsrep files are):

            sql/item_strfunc.h:#include "wsrep_api.h" - under a   #ifdef WITH_WSREP
            sql/sql_class.h:#include "wsrep.h" - no def prot
            sql/sql_class.h:#include "wsrep_on.h" - no def prot
            sql/sql_class.h:#include "wsrep_client_service.h" - this and below under #ifdef WITH_WSREP
            sql/sql_class.h:#include "wsrep_client_state.h"
            sql/sql_class.h:#include "wsrep_mutex.h"
            sql/sql_class.h:#include "wsrep_condition_variable.h"
            

            So remove EXCL_WSREP parts of include/CMakeLists.txt for wsrep.h to be there.

            and in sql/CMakeList -

            SET(EXCL_WSREP "wsrep_[a-np-z]*.h")
            

            maybe? to keep the wsrep_on.h, which happens to be the only wsrep_o*.h file.

            On CI, seems to be getting more common so a build at least so probably soon.

            Do you know of a test that links against the server headers?

            Happy to take this as one or two PRs, whatever is easiest.

            danblack Daniel Black added a comment - - edited Robin, I think you patch is right for the fault observed in gitlab, please do a PR. Daniel, there might be different problem here. wsrep includes in sql/*.h (for non-wsrep files are): sql/item_strfunc.h:#include "wsrep_api.h" - under a #ifdef WITH_WSREP sql/sql_class.h:#include "wsrep.h" - no def prot sql/sql_class.h:#include "wsrep_on.h" - no def prot sql/sql_class.h:#include "wsrep_client_service.h" - this and below under #ifdef WITH_WSREP sql/sql_class.h:#include "wsrep_client_state.h" sql/sql_class.h:#include "wsrep_mutex.h" sql/sql_class.h:#include "wsrep_condition_variable.h" So remove EXCL_WSREP parts of include/CMakeLists.txt for wsrep.h to be there. and in sql/CMakeList - SET(EXCL_WSREP "wsrep_[a-np-z]*.h") maybe? to keep the wsrep_on.h, which happens to be the only wsrep_o*.h file. On CI, seems to be getting more common so a build at least so probably soon. Do you know of a test that links against the server headers? Happy to take this as one or two PRs, whatever is easiest.
            danblack Daniel Black made changes -
            Fix Version/s 10.5 [ 23123 ]
            danblack Daniel Black made changes -

            Thanks Daniel. PR created here: https://github.com/MariaDB/server/pull/2411

            robinnew Robin Newhouse added a comment - Thanks Daniel. PR created here: https://github.com/MariaDB/server/pull/2411
            cross Chris Ross added a comment - - edited

            I have no issues with the change if it's right. I'm not sure what the MTR tests are, and why they need the wsrep/galera includes if wsrep isn't to be used. Shouldn't the tests instead be changed to not use need them when building without WSREP, similar to what was done to fix MDEV-25625?

            cross Chris Ross added a comment - - edited I have no issues with the change if it's right. I'm not sure what the MTR tests are, and why they need the wsrep/galera includes if wsrep isn't to be used. Shouldn't the tests instead be changed to not use need them when building without WSREP, similar to what was done to fix MDEV-25625 ?

            cross wrote:

            Shouldn't the tests instead be changed to not use need them when building without WSREP, similar to what was done to fix MDEV-25625?

            Yes, agreed. If tests aren't actually using Galera/WSREP (and clearly if they're passing when it's not built… they're not ) then they shouldn't be including Galera/WSREP include files.

            dlenski Daniel Lenski (Inactive) added a comment - cross wrote: Shouldn't the tests instead be changed to not use need them when building without WSREP, similar to what was done to fix MDEV-25625 ? Yes, agreed. If tests aren't actually using Galera/WSREP (and clearly if they're passing when it's not built… they're not ) then they shouldn't be including Galera/WSREP include files.
            danblack Daniel Black added a comment -

            The test Robin tripped on Gitlab was missing include/wait_wsrep_ready.inc called from ./include/start_mysqld.inc (more widely used on many tests that restart the server) which has inbuilt tests which detects galera missing, which results in it being largely a no-op, but mtr can't tell that unless the file is there.

            Both wsrep.h and wsrep_on.h contain no-op macros needed in the without wsrep case. so hence https://github.com/MariaDB/server/pull/2417 which adds the two files to the Devel package. Thanks dlenski

            danblack Daniel Black added a comment - The test Robin tripped on Gitlab was missing include/wait_wsrep_ready.inc called from ./include/start_mysqld.inc (more widely used on many tests that restart the server) which has inbuilt tests which detects galera missing, which results in it being largely a no-op, but mtr can't tell that unless the file is there. Both wsrep.h and wsrep_on.h contain no-op macros needed in the without wsrep case. so hence https://github.com/MariaDB/server/pull/2417 which adds the two files to the Devel package. Thanks dlenski
            cross Chris Ross added a comment -

            Okay. Just wanted to make sure the earlier removal of that one pattern from `INSTALL_MYSQL_TEST`s `EXCL_GALERA` was still correct.

            cross Chris Ross added a comment - Okay. Just wanted to make sure the earlier removal of that one pattern from `INSTALL_MYSQL_TEST`s `EXCL_GALERA` was still correct.
            danblack Daniel Black added a comment -

            Yes. Resulting wsrep files (including both PRs) is per https://github.com/MariaDB/server/pull/2417#issue-1521525371. Two or three files might be excess to absolute minimal requirements but its better than going too far.

            danblack Daniel Black added a comment - Yes. Resulting wsrep files (including both PRs) is per https://github.com/MariaDB/server/pull/2417#issue-1521525371 . Two or three files might be excess to absolute minimal requirements but its better than going too far.
            danblack Daniel Black made changes -
            issue.field.resolutiondate 2023-01-06 00:13:23.0 2023-01-06 00:13:23.495
            danblack Daniel Black made changes -
            Component/s Packaging [ 10700 ]
            Fix Version/s 10.5.19 [ 28511 ]
            Fix Version/s 10.6.12 [ 28513 ]
            Fix Version/s 10.7.8 [ 28515 ]
            Fix Version/s 10.8.7 [ 28517 ]
            Fix Version/s 10.9.5 [ 28519 ]
            Fix Version/s 10.10.3 [ 28521 ]
            Fix Version/s 10.11.2 [ 28523 ]
            Fix Version/s 11.0.1 [ 28548 ]
            Fix Version/s 10.5 [ 23123 ]
            Resolution Fixed [ 1 ]
            Status Open [ 1 ] Closed [ 6 ]

            People

              danblack Daniel Black
              robinnew Robin Newhouse
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

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