[SERVER-71467] Dont run feature flag specific tests for multiversion testing Created: 17/Nov/22  Updated: 29/Oct/23  Resolved: 10/Jan/23

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 6.0.4, 6.3.0-rc0

Type: Bug Priority: Major - P3
Reporter: Gregory Noma Assignee: Tausif Rahman (Inactive)
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Backports
Depends
is depended on by SERVER-67268 Investigate simplifying usages of fea... Closed
is depended on by SERVER-71898 Multiversion unreleased feature flagg... Closed
Related
related to SERVER-69883 Investigate improving JS test tags fo... Open
Assigned Teams:
Server Development Platform
Backwards Compatibility: Fully Compatible
Operating System: ALL
Backport Requested:
v6.2, v6.0
Participants:
Linked BF Score: 157

 Description   

In all-feature-flag multiversion testing, in the global_index_rollback.js test the featureFlagGlobalIndexes feature flag is not enabled on the 6.2 nodes which causes the test to fail due to "no such command: '_shardsvrCreateGlobalIndex'".


The above is how this issue was surfaced. We shouldn't run feature flag specific tests during multiversion testing for 2 reasons.

(1) The feature flag c++ code is gated by FeatureFlag::isEnabled(serverGlobalParams.featureCompatibility) & multiversion testing's featureCompatibilityVersion will always be lower than the required FCV for a non-default feature flag. Therefore, the feature flag code can never run with the given FCV. This will always result in failures for featureFlag specific tests.
(2) The previous version is not guaranteed to have the featureFlag feature. So there's a good chance, featureFlag specific tests will fail.

There is a resmoke argument specifically for this use case: --runNoFeatureFlagTests. We should automatically add this when we are running multiversion testing in CI.



 Comments   
Comment by Githook User [ 14/Dec/22 ]

Author:

{'name': 'Tausif Rahman', 'email': 'tausif.rahman@mongodb.com', 'username': 'trahman1318'}

Message: SERVER-71467 Dont run feature flag specific tests for multiversion testing

(cherry picked from commit 58818ad6cc1445841f43e02df4e2b4866e591281)
Branch: v6.0
https://github.com/mongodb/mongo/commit/d4ab3fcbfd696c615012f38f0f004cea19d88cc7

Comment by Tausif Rahman (Inactive) [ 13/Dec/22 ]

need to do backports

Comment by Githook User [ 12/Dec/22 ]

Author:

{'name': 'Tausif Rahman', 'email': 'tausif.rahman@mongodb.com', 'username': 'trahman1318'}

Message: SERVER-71467 Dont run feature flag specific tests for multiversion testing
Branch: master
https://github.com/mongodb/mongo/commit/58818ad6cc1445841f43e02df4e2b4866e591281

Comment by Tausif Rahman (Inactive) [ 07/Dec/22 ]

Ah okay, that makes sense. Thanks for clarifying max.hirschhorn@mongodb.com. I think this is best suited for --excludeWithAnyTags=featureFlagXX,featureFlagYY, etc. 

Comment by Max Hirschhorn [ 06/Dec/22 ]

Thanks for the question tausif.rahman@mongodb.com. The multiversion testing story we have didn't get a holistic design so I think it is fair to question some of the utility.

I would say it is important to continue running the multiversion tests on the "all feature flags" build variants. The purpose of the "all feature flags" build variant is to reflect what would happen if all in-development feature flags were released (defaulted to on) today. The multiversion tests run in a lower FCV and so even with --setParameter featureFlagXX, C++ code which is appropriately using FeatureFlag::isEnabled(serverGlobalParams.featureCompatibility) will continue to not execute. The C++ code would only execute after {setFeatureCompatibilityVersion: kLatest} is run. And hence there is the odd detail that none of the JavaScript tests tagged with featureFlagXX are valid to run because the explicit tests for the feature would be expected to fail. However, if a feature flag is ignoring the feature compatibility version (e.g. perhaps it is a feature independent from replication or other cases found by SERVER-66587) then that C++ would execute only in the presence of --setParameter featureFlagXX.

That is to say, running the multiversion tests on the "all feature flags" build variants is the only way I see to catch a feature inappropriately not consulting the FCV value prior to the commit which attempts to enable the feature by default. And the primary objective of our multiversion testing is to ensure enabling the feature by default is a non-event where we have been doing all of the required testing commit-over-commit the whole time.

Comment by Tausif Rahman (Inactive) [ 06/Dec/22 ]

max.hirschhorn@mongodb.com I'm looking at this again & I'm curious if we should even be running multiversion testing with feature flags enabled. "All feature flags" enabled will enable all feature flags that do not yet have a default version. These features are not available in past versions & I don't think it serves any purpose to enable these on the latest version just to disable any tests that use those feature flags (since those test will definitely fail on previous versions).

By not running this in an "all feature flags" variant, the tests will be naturally excluded/included.

If we take featureFlagGlobalIndexes as an example – since featureFlagGlobalIndexes does not have a default version it will not be enabled & any tests with the featureFlagGlobalIndexes tag will be excluded from running. This allows multiversion testing to successfully exclude unsupported feature flag specific testing. When featureFlagGlobalIndexes eventually gets a default version – say 6.3 – all featureFlagGlobalIndexes tags will be updated to requires_fcv_63. If master is 6.3 this feature flag is enabled by default & any old featureFlagGlobalIndexes tests would be fair game, but they are blocked from running in multiversion testing because of the requires_fcv_63 tag.

Do you think not running multiversion tests in "all feature flags" variants is a better solution?

Comment by Tausif Rahman (Inactive) [ 06/Dec/22 ]

huayu.ouyang@mongodb.com tagging with requires_fcv_63 will prevent the test from running in multiversion configurations until 6.3 is released. That's a sufficient workaround for now, but as discussed in Slack, it is technically "wrong" since that feature flag does not yet have a version. Long term, the above plan will make it so that we don't need to use requires_fcv_63 in a hacky way.

Comment by Max Hirschhorn [ 06/Dec/22 ]

Yes, I believe using requires_fcv_63 on the master branch in the interim is also a solution and is being pursued in SERVER-71856 to address some sharding_multiversion Evergreen failures.

huayu.ouyang@mongodb.com, I had (wishfully) interpreted SERVER-67268 to be about ensuring featureFlagXX tags can be used to skip running tests in certain scenarios while a feature is in-development and to replace the featureFlagXX tags with the appropriate requires_fcv_yy tag when the feature is defaulted to being on in the 'yy' version.

Comment by Huayu Ouyang [ 06/Dec/22 ]

tausif.rahman@mongodb.com I was also under the impression that tagging the test with featureFlagGlobalIndexes and requires_fcv_63 should have excluded the test from being run in multiversion configurations. Does that not work?

max.hirschhorn@mongodb.com Also, sorry I believe the title/description of SERVER-67268 is slightly out of date in that with the current test infrastructure we cannot get rid of using requires_fcv_yy, but I can repurpose the ticket to move away from using FeatureFlagUtil.isEnabled.

Comment by Max Hirschhorn [ 06/Dec/22 ]

I don't think server_feature_flags.idl should be sourced directly for how resmoke learns of which feature flags exist and which are not enabled by default. That metadata should probably instead be generated by buildscripts/idl/gen_all_feature_flag_list.py and persisted in the all_feature_flags.txt file.

tausif.rahman@mongodb.com, what you describe about adding --excludeWithAnyTags for each featureFlagXX, featureFlagYY, etc. makes sense to me. I was under the impression that a combination of the work done under SERVER-51883 and SERVER-54420 had already achieved this.

CC huayu.ouyang@mongodb.com because it looks like addressing SERVER-71467 is a prerequisite to doing SERVER-67268. I would hope the end state is to see very few usages of FeatureFlagUtil.isEnabled() and certainly none when the entire test is being skipped. It is wasteful to repeatedly start a sharded cluster only to tear it down.

Comment by Tausif Rahman (Inactive) [ 06/Dec/22 ]

On the testing infra side, we can probably parse the server_feature_flags.idl file for any feature flags that are not enabled by default and automatically add an --excludeWithAnyTags=featureFlag1,featureFlag2,featureFlag3 for multiversion testing. Whenever the feature flag is eventually enabled by default on a server version, the tests will no longer be excluded.

In the interim, we can add checks in the tests as is done here. This is supported & described in the 3rd bullet in the readme: https://github.com/mongodb/mongo/blob/085aa724abcba8481860109593c70620915069e2/jstests/sharding/txn_addingParticipantParameter.js#L53-L59

Any concerns with that approach max.hirschhorn@mongodb.com?

Comment by Max Hirschhorn [ 05/Dec/22 ]

All feature flags are not enabled on prior versions in the multiversion testing. Why?

The featureFlagGlobalIndexes feature flag is not enabled by default in any server version still. MongoDB 6.2, which is a production version, must therefore not support running the _shardsvrCreateGlobalIndex command. Speaking for an arbitrary feature, because the feature flag is disabled, there is no reason to expect the MongoDB 6.2 behavior is consistent with the MongoDB 6.3 behavior. It could just as well be that the feature is only partially implemented in MongoDB 6.2.

Atlas isn't ever going to specify --setParameter featureFlagGlobalIndexes=true for the 6.2 release and so it would misleading for our testing infrastructure to specify it. It would be most appropriate for the testing infrastructure to skip any tests which are dependent on the featureFlagGlobalIndexes feature flag being enabled.

Comment by Matt Kneiser [ 22/Nov/22 ]

All feature flags are not enabled on prior versions in the multiversion testing. Why?
If the test isn't respecting the version tag, contact alexander.neben@mongodb.com 

Generated at Thu Feb 08 06:19:05 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.