Skip to content

CDRIVER-3620 Add SASL, CSE, and Sanitizers matrices #1201

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

Merged
merged 53 commits into from
Feb 14, 2023

Conversation

eramongodb
Copy link
Contributor

Description

This PR is part of CDRIVER-3620.

Please refer to this patch to verify the proposed Evergreen matrix (at the time of PR creation). New patches will be posted as changes are made to the matrices as part of this PR review.

The objective of this PR is to evaluate the set of compile and test tasks currently captured by the new config generator under components.sasl, components.cse, components.sanitizers, and components.std. At the time of PR creation, the matrices of test tasks are the same as the current set of tasks and mapping to distros in order to facilitate their review (status quo) and subsequent modifications (desired matrices).

Note: with the exception of the macos-1100-arm64 distro, compile tasks that do not have a dependent test task have been removed in an effort to avoid redundancies. Compile coverage can be added back in via modifications to appropriate COMPILE_MATRIX tables as part of this PR.

Matrix Design

There are several notable aspects and/or goals to the proposed matrices.

One Tag for All Tasks per Variant

The current Evergreen config (mostly) defines build variants that maps to a given distro (i.e. ubuntu1804-large). A common compile task (i.e. debug-compile-sasl-openssl) is defined and included in multiple variants, and a common test task (i.e. test-latest-server-auth-sasl-openssl) is separately defined and separately included in multiple variants accordingly. This makes it difficult to track what test task is being run on which variant, especially given the liberal use of generic tags to specify included tasks (e.g. what tasks are included by .latest .nossl?).

Furthermore, it is difficult to verify that a required compile tasks is being included in the same variant, as Evergreen does not enforce a dependent task is included in the same variant. It is also difficult to confirm whether a given set of tasks is being included in any variant, as Evergreen does not enforce a defined task is included in at least one variant (e.g. none of the test-.*-sharded-.*-cse tasks defined in the current Evergreen configuration are included in any variant: there is no test coverage of CSE with sharded clusters!).

The new matrix components under sasl, cse, sanitizers, and c11 define a single build variant for each logical matrix (i.e. by SSL library) and includes all tasks in the given variant using a single tag unique to each matrix (the TAG variable defined at the top of each matrix component; typically the same name as the variant). For example, in components.sasl.openssl:

SSL = 'openssl'
TAG = f'sasl-matrix-{SSL}'

# ...

def variants():
    expansions = {
        'DEBUG': 'ON'
    }

    return [
        BuildVariant(
            name=TAG,
            display_name=TAG,
            tasks=[EvgTaskRef(name=f'.{TAG}')], # <-- !!
            expansions=expansions,
        ),
    ]

The new matrix components also ensures that every task has a unique name across all variants to ensure any dependent task required by the variant (i.e. sasl-auto-openssl-ubuntu1804-gcc-test-latest-server-auth depends on sasl-auto-openssl-ubuntu1804-gcc-compile) will trigger Evergreen validation failure if missing.

Distro-Specific Tasks

Instead of defining the distro to be executed on in the build variant definition (requiring N build variants for N distros as in the current Evergreen configuration), the distro is defined at the task level. Not only does this allow for better logical grouping of features being tested by each task (similar tasks are in the same variant instead of split across multiple variants), but the task names are also refactored to better express logical groupings. For example, in the sasl-matrix-openssl variant, tasks that test SASL=AUTO + SSL=OPENSSL on Ubuntu 18.04 are lexicographically grouped as follows:

sasl-auto-openssl-ubuntu1804-gcc-compile
sasl-auto-openssl-ubuntu1804-gcc-test-4.0-replica-auth
sasl-auto-openssl-ubuntu1804-gcc-test-4.0-replica-noauth
sasl-auto-openssl-ubuntu1804-gcc-test-4.0-server-auth
sasl-auto-openssl-ubuntu1804-gcc-test-4.0-server-noauth
...

An effort is made to use higher-level categories (i.e. cse -> sasl -> ssl`) as prefixes and lower-level categories (i.e. server version -> topology -> authentication) to faciliate nice lexicographical sorting and grouping of task names.

Sanitizers

The sanitizers-matrix-asan variant enables UBSAN alongside ASAN:

expansions = {
    'ASAN': 'on',
    # ...
    'EXTRA_CONFIGURE_FLAGS': '-DENABLE_EXTRA_ALIGNMENT=OFF',
    'SANITIZE': 'address,undefined',
}

This is a discrepancy from the current Evergreen configuration. This makes the debug-compile-ubsan task redundant. The debug-compile-ubsan-with-extra-alignment task has been dropped due lack of runtime testing after f4a83c0 and false-positive misaligned address failures being observed during execution of the extended test suite.

The DEBUG: ON expansion has also been dropped when running compile tasks in sanitizers matrices. This means CMake is configured with a build type of RelWithDebInfo instead of Debug to offset the performance penalty of compiling with ASAN+UBSAN or TSAN.

Expansions

Expansions (that are treated as environment variables when a subprocess command is executed with add_expansions_to_env) that are common to all tasks in a variant are defined by the variant. Expansions that are specific to a given task (i.e. CC for compiler selection) are defined by the task. An effort is made to define environment variables required by a given task in the Evergreen configuration as expansions via update_expansions() or env= to reduce complexity of the executed script itself.

A notable exception are expansions that are expected to be prepended/appended to. For example, EXTRA_CONFIGURE_FLAGS in components.sanitizers.asan_cse is defined both by the variant:

'EXTRA_CONFIGURE_FLAGS': '-DENABLE_EXTRA_ALIGNMENT=OFF'

and by the function:

bash_exec(
    # ...
    script='EXTRA_CONFIGURE_FLAGS="-DENABLE_PIC=ON -DENABLE_CLIENT_SIDE_ENCRYPTION=ON ${EXTRA_CONFIGURE_FLAGS}" .evergreen/scripts/compile.sh',
    # ...

env= does not play well with add_expansions_to_env when a given expansion is already defined: the existing expansion takes precedence over the definition provided by env=. The variables is therefore defined/expanded in the script instead.

Evergreen Project Settings

Once this PR is merged, the Evergreen validator will emit the following warning:

WARNING: Github PR alias matching variant regexp 'asan-ubuntu$' has no matching variants
WARNING: Patch alias matching variant regexp 'asan-ubuntu$' has no matching variants

The Evergreen project settings will have to be updated to match against the new sanitizers-matrix-asan variant instead.

Followup Work

Unfortunately not many current by-distro variants could be removed yet due to tasks still assigned to them. In particular, relocating release-compile, IPv4/IPv6, and authentication tasks should allow for several by-distro variants to be removed. However, this work is deferred to a later date due to priorities and scheduling.

@eramongodb eramongodb requested a review from kevinAlbs February 3, 2023 22:55
@eramongodb eramongodb self-assigned this Feb 3, 2023
@kevinAlbs kevinAlbs requested a review from rcsanchez97 February 6, 2023 16:51
Copy link
Contributor

@rcsanchez97 rcsanchez97 left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2023

Codecov Report

Merging #1201 (8b5a14e) into master (8b5a14e) will not change coverage.
The diff coverage is n/a.

❗ Current head 8b5a14e differs from pull request most recent head bf90754. Consider uploading reports for the commit bf90754 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #1201   +/-   ##
=======================================
  Coverage   63.31%   63.31%           
=======================================
  Files         328      328           
  Lines       84382    84382           
=======================================
  Hits        53426    53426           
  Misses      30956    30956           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@rcsanchez97 rcsanchez97 left a comment

Choose a reason for hiding this comment

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

One additional comment/suggestion, but otherwise still LGTM

Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Fantastic work. The concise matrix form makes it significantly easier to audit the tasks being run.

Most suggestions are based on these general opinions:

  • It is OK to have compile tasks that do not have dependent test tasks. That may validate some of the many ways to compile the C driver without paying the cost of running the full test suite.
  • Running all tests with multiple Linux distros on the same architecture does not add much value.
  • Running all tests with multiple Visual Studio versions does not add much value.
  • Testing with nossl and noauth provides little value. I expect the vast majority of users are using TLS and auth. TLS and auth are required in MongoDB Atlas.
  • Testing with sasl=off provides no value. Disabling sasl does not appear to provide any additional test coverage. Checks for sasl=off in test-libmongoc.c and run-auth-test.sh remove tests when sasl=off
  • Tests with 5.0 and latest should include 6.0. Not including 6.0 may have been an oversight of CDRIVER-4370.
  • Testing replica adds value. A significant amount of tests requiring an oplog are skipped on the server topology (e.g. Transactions and Queryable Encryption). Testing on all topologies is suggested in the drafted CI Matrix Policy. I expect adding tests for sharded may involve more investigation.

@eramongodb
Copy link
Contributor Author

eramongodb commented Feb 10, 2023

Latest changes verified by this patch. Note: OCSP tests are now excluded from patch builds after 04d975b.

Notes:

@eramongodb eramongodb requested a review from kevinAlbs February 10, 2023 21:25
@eramongodb
Copy link
Contributor Author

SASL was defaulting to "SSPI" in compile-windows.sh if not defined. Added explicit sasl='OFF' for all SaslOff compile functions. Verified by this patch.

@eramongodb
Copy link
Contributor Author

Latest changes (prior to merge with upstream/master) verified by this patch.

# pylint: disable=line-too-long
# fmt: off
COMPILE_MATRIX = [
('macos-1015', 'clang', None, ['cyrus']),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like this task may be blocked by macos-1015 not yet(?) providing the CoreFoundation library (aka Secure Transport) required by both libmongocrypt and libmongoc when Client Side Encryption is enabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I forgot I found the same problem in libmongocrypt before and filed MONGOCRYPT-443. I suggest commenting out cse tasks on macOS until MONGOCRYPT-443 is resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather than commenting out the tasks, I added disable: true to the tasks instead and renamed EvgTaskWithRunOn to simply Task accordingly. Per docs:

This behaves similarly to commenting out the task but will not trigger any validation errors.

@eramongodb eramongodb requested a review from kevinAlbs February 13, 2023 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants