From e38772165366061ef6aa21477f9f352564c48cc9 Mon Sep 17 00:00:00 2001 From: Mark Benvenuto Date: Fri, 16 Oct 2020 18:10:40 -0400 Subject: SERVER-51110 Feature Flag with FCV checks --- buildscripts/idl/idl/ast.py | 1 + buildscripts/idl/idl/binder.py | 26 +++- buildscripts/idl/idl/errors.py | 17 +++ buildscripts/idl/idl/generator.py | 16 ++- buildscripts/idl/idl/parser.py | 12 +- buildscripts/idl/idl/syntax.py | 1 + buildscripts/idl/tests/test_binder.py | 46 +++++++ buildscripts/idl/tests/test_parser.py | 13 ++ jstests/core/profile_operation_metrics.js | 2 +- src/mongo/db/stats/SConscript | 1 + .../db/stats/operation_resource_consumption.idl | 2 +- .../db/stats/resource_consumption_metrics.cpp | 5 +- .../db/stats/resource_consumption_metrics_test.cpp | 15 ++- src/mongo/db/storage/SConscript | 1 + src/mongo/db/storage/storage_parameters.idl | 2 +- src/mongo/idl/SConscript | 18 +++ src/mongo/idl/feature_flag.cpp | 123 +++++++++++++++++++ src/mongo/idl/feature_flag.h | 119 ++++++++++++++++++ src/mongo/idl/feature_flag_test.cpp | 133 ++++++++++++++++++++- src/mongo/idl/feature_flag_test.idl | 14 +++ 20 files changed, 545 insertions(+), 22 deletions(-) create mode 100644 src/mongo/idl/feature_flag.cpp create mode 100644 src/mongo/idl/feature_flag.h diff --git a/buildscripts/idl/idl/ast.py b/buildscripts/idl/idl/ast.py index 6af6ca4e94a..3a8880f2bfd 100644 --- a/buildscripts/idl/idl/ast.py +++ b/buildscripts/idl/idl/ast.py @@ -296,6 +296,7 @@ class ServerParameter(common.SourceLocation): self.test_only = False # type: bool self.deprecated_name = [] # type: List[str] self.default = None # type: Expression + self.feature_flag = False # type : bool # Only valid if cpp_varname is specified. self.validator = None # type: Validator diff --git a/buildscripts/idl/idl/binder.py b/buildscripts/idl/idl/binder.py index 8e01fe83b02..3d8055e0763 100644 --- a/buildscripts/idl/idl/binder.py +++ b/buildscripts/idl/idl/binder.py @@ -1005,8 +1005,8 @@ def _bind_server_parameter(ctxt, param): return None -def _bind_feature_flags(param): - # type: (syntax.FeatureFlag) -> ast.ServerParameter +def _bind_feature_flags(ctxt, param): + # type: (errors.ParserContext, syntax.FeatureFlag) -> ast.ServerParameter """Bind a FeatureFlag as a serverParameter setting.""" ast_param = ast.ServerParameter(param.file_name, param.line, param.column) ast_param.name = param.name @@ -1014,9 +1014,25 @@ def _bind_feature_flags(param): ast_param.set_at = "ServerParameterType::kStartupOnly" - ast_param.cpp_vartype = "bool" - ast_param.default = _bind_expression(param.default) + ast_param.cpp_vartype = "::mongo::FeatureFlag" + + # Feature flags that default to false must not have a version + if param.default.literal == "false" and param.version: + ctxt.add_feature_flag_default_false_has_version(param) + return None + + # Feature flags that default to true are required to have a version + if param.default.literal == "true" and not param.version: + ctxt.add_feature_flag_default_true_missing_version(param) + return None + + expr = syntax.Expression(param.default.file_name, param.default.line, param.default.column) + expr.expr = '%s, "%s"_sd' % (param.default.literal, param.version if param.version else '') + + ast_param.default = _bind_expression(expr) + ast_param.default.export = False ast_param.cpp_varname = param.cpp_varname + ast_param.feature_flag = True return ast_param @@ -1185,7 +1201,7 @@ def bind(parsed_spec): bound_spec.structs.append(_bind_struct(ctxt, parsed_spec, struct)) for feature_flag in parsed_spec.feature_flags: - bound_spec.server_parameters.append(_bind_feature_flags(feature_flag)) + bound_spec.server_parameters.append(_bind_feature_flags(ctxt, feature_flag)) for server_parameter in parsed_spec.server_parameters: bound_spec.server_parameters.append(_bind_server_parameter(ctxt, server_parameter)) diff --git a/buildscripts/idl/idl/errors.py b/buildscripts/idl/idl/errors.py index fc3ecef8ff8..4548b5ac505 100644 --- a/buildscripts/idl/idl/errors.py +++ b/buildscripts/idl/idl/errors.py @@ -110,6 +110,8 @@ ERROR_ID_SERVER_PARAMETER_INVALID_ATTR = "ID0066" ERROR_ID_SERVER_PARAMETER_REQUIRED_ATTR = "ID0067" ERROR_ID_SERVER_PARAMETER_INVALID_METHOD_OVERRIDE = "ID0068" ERROR_ID_NON_CONST_GETTER_IN_IMMUTABLE_STRUCT = "ID0069" +ERROR_ID_FEATURE_FLAG_DEFAULT_TRUE_MISSING_VERSION = "ID0070" +ERROR_ID_FEATURE_FLAG_DEFAULT_FALSE_HAS_VERSION = "ID0071" class IDLError(Exception): @@ -804,6 +806,21 @@ class ParserContext(object): self._add_error(location, ERROR_ID_MISSING_SHORT_NAME_WITH_SINGLE_NAME, ("Missing 'short_name' required with 'single_name' value '%s'") % (name)) + def add_feature_flag_default_true_missing_version(self, location): + # type: (common.SourceLocation) -> None + """Add an error about a default flag with a default value of true but no version.""" + # pylint: disable=invalid-name + self._add_error(location, ERROR_ID_FEATURE_FLAG_DEFAULT_TRUE_MISSING_VERSION, + ("Missing 'version' required for feature flag that defaults to true")) + + def add_feature_flag_default_false_has_version(self, location): + # type: (common.SourceLocation) -> None + """Add an error about a default flag with a default value of false but has a version.""" + # pylint: disable=invalid-name + self._add_error( + location, ERROR_ID_FEATURE_FLAG_DEFAULT_FALSE_HAS_VERSION, + ("The 'version' attribute is not allowed for feature flag that defaults to false")) + def _assert_unique_error_messages(): # type: () -> None diff --git a/buildscripts/idl/idl/generator.py b/buildscripts/idl/idl/generator.py index c4cea8d1697..81cec424345 100644 --- a/buildscripts/idl/idl/generator.py +++ b/buildscripts/idl/idl/generator.py @@ -944,6 +944,8 @@ class _CppHeaderFileWriter(_CppFileWriterBase): header_list.append('mongo/util/options_parser/environment.h') if spec.server_parameters: + if [param for param in spec.server_parameters if param.feature_flag]: + header_list.append('mongo/idl/feature_flag.h') header_list.append('mongo/idl/server_parameter.h') header_list.append('mongo/idl/server_parameter_with_storage.h') @@ -1963,10 +1965,16 @@ class _CppSourceFileWriter(_CppFileWriterBase): def _gen_server_parameter_with_storage(self, param): # type: (ast.ServerParameter) -> None """Generate a single IDLServerParameterWithStorage.""" - self._writer.write_line( - common.template_args( - 'auto* ret = makeIDLServerParameterWithStorage<${spt}>(${name}, ${storage});', - storage=param.cpp_varname, spt=param.set_at, name=_encaps(param.name))) + if param.feature_flag: + self._writer.write_line( + common.template_args( + 'auto* ret = new FeatureFlagServerParameter(${name}, ${storage});', + storage=param.cpp_varname, name=_encaps(param.name))) + else: + self._writer.write_line( + common.template_args( + 'auto* ret = makeIDLServerParameterWithStorage<${spt}>(${name}, ${storage});', + storage=param.cpp_varname, spt=param.set_at, name=_encaps(param.name))) if param.on_update is not None: self._writer.write_line('ret->setOnUpdate(%s);' % (param.on_update)) diff --git a/buildscripts/idl/idl/parser.py b/buildscripts/idl/idl/parser.py index 08f76bf0273..4b9f40ee325 100644 --- a/buildscripts/idl/idl/parser.py +++ b/buildscripts/idl/idl/parser.py @@ -619,9 +619,15 @@ def _parse_feature_flag(ctxt, spec, name, node): _generic_parser( ctxt, node, "feature_flags", param, { - "description": _RuleDesc('scalar', _RuleDesc.REQUIRED), - "cpp_varname": _RuleDesc('scalar'), - "default": _RuleDesc('scalar_or_mapping', mapping_parser_func=_parse_expression), + "description": + _RuleDesc('scalar', _RuleDesc.REQUIRED), + "cpp_varname": + _RuleDesc('scalar'), + "default": + _RuleDesc('scalar_or_mapping', _RuleDesc.REQUIRED, + mapping_parser_func=_parse_expression), + "version": + _RuleDesc('scalar'), }) spec.feature_flags.append(param) diff --git a/buildscripts/idl/idl/syntax.py b/buildscripts/idl/idl/syntax.py index 9b3530a7db7..d1781be492c 100644 --- a/buildscripts/idl/idl/syntax.py +++ b/buildscripts/idl/idl/syntax.py @@ -540,6 +540,7 @@ class FeatureFlag(common.SourceLocation): self.description = None # type: str self.cpp_varname = None # type: str self.default = None # type: Expression + self.version = None # type: str super(FeatureFlag, self).__init__(file_name, line, column) diff --git a/buildscripts/idl/tests/test_binder.py b/buildscripts/idl/tests/test_binder.py index d433145a169..e0c621317bb 100644 --- a/buildscripts/idl/tests/test_binder.py +++ b/buildscripts/idl/tests/test_binder.py @@ -1985,6 +1985,52 @@ class TestBinder(testcase.IDLTestcase): source: cli """), idl.errors.ERROR_ID_MISSING_SHORT_NAME_WITH_SINGLE_NAME) + def test_feature_flag(self): + # type: () -> None + """Test feature flag checks around version.""" + + # feature flag can default to false without a version + self.assert_bind( + textwrap.dedent(""" + feature_flags: + featureFlagToaster: + description: "Make toast" + cpp_varname: gToaster + default: false + """)) + + # feature flag can default to true with a version + self.assert_bind( + textwrap.dedent(""" + feature_flags: + featureFlagToaster: + description: "Make toast" + cpp_varname: gToaster + default: true + version: 123 + """)) + + # true is only allowed with a version + self.assert_bind_fail( + textwrap.dedent(""" + feature_flags: + featureFlagToaster: + description: "Make toast" + cpp_varname: gToaster + default: true + """), idl.errors.ERROR_ID_FEATURE_FLAG_DEFAULT_TRUE_MISSING_VERSION) + + # false is not allowed with a version + self.assert_bind_fail( + textwrap.dedent(""" + feature_flags: + featureFlagToaster: + description: "Make toast" + cpp_varname: gToaster + default: false + version: 123 + """), idl.errors.ERROR_ID_FEATURE_FLAG_DEFAULT_FALSE_HAS_VERSION) + if __name__ == '__main__': diff --git a/buildscripts/idl/tests/test_parser.py b/buildscripts/idl/tests/test_parser.py index d204d88d440..47d84582538 100644 --- a/buildscripts/idl/tests/test_parser.py +++ b/buildscripts/idl/tests/test_parser.py @@ -1123,6 +1123,19 @@ class TestParser(testcase.IDLTestcase): - two """), idl.errors.ERROR_ID_IS_NODE_TYPE_SCALAR_OR_MAPPING) + def test_feature_flag(self): + # type: () -> None + """Test feature flag.""" + + # Missing default + self.assert_parse_fail( + textwrap.dedent(""" + feature_flags: + featureFlagToaster: + description: "Make toast" + cpp_varname: gToaster + """), idl.errors.ERROR_ID_MISSING_REQUIRED_FIELD) + if __name__ == '__main__': diff --git a/jstests/core/profile_operation_metrics.js b/jstests/core/profile_operation_metrics.js index 7e5112f3e26..44a0b7d1386 100644 --- a/jstests/core/profile_operation_metrics.js +++ b/jstests/core/profile_operation_metrics.js @@ -18,7 +18,7 @@ load("jstests/libs/fixture_helpers.js"); // For isReplSet(). let res = assert.commandWorked( db.adminCommand({getParameter: 1, measureOperationResourceConsumption: 1})); -if (!res.measureOperationResourceConsumption) { +if (!res.measureOperationResourceConsumption.value) { jsTestLog("Skipping test because the 'measureOperationResourceConsumption' flag is disabled"); return; } diff --git a/src/mongo/db/stats/SConscript b/src/mongo/db/stats/SConscript index 331d77d85b7..5b9d036e170 100644 --- a/src/mongo/db/stats/SConscript +++ b/src/mongo/db/stats/SConscript @@ -84,6 +84,7 @@ env.Library( ], LIBDEPS_PRIVATE=[ '$BUILD_DIR/mongo/db/repl/repl_coordinator_interface', + '$BUILD_DIR/mongo/idl/feature_flag', '$BUILD_DIR/mongo/idl/server_parameter', ], LIBDEPS_TYPEINFO=[ diff --git a/src/mongo/db/stats/operation_resource_consumption.idl b/src/mongo/db/stats/operation_resource_consumption.idl index 796e9c065b5..c6c95891493 100644 --- a/src/mongo/db/stats/operation_resource_consumption.idl +++ b/src/mongo/db/stats/operation_resource_consumption.idl @@ -31,7 +31,7 @@ global: feature_flags: measureOperationResourceConsumption: - description: "When true, records and reports per-operation resource consumption" + description: "When enabled, records and reports per-operation resource consumption" cpp_varname: gMeasureOperationResourceConsumption default: false diff --git a/src/mongo/db/stats/resource_consumption_metrics.cpp b/src/mongo/db/stats/resource_consumption_metrics.cpp index 4244b845590..5d15e8cd68e 100644 --- a/src/mongo/db/stats/resource_consumption_metrics.cpp +++ b/src/mongo/db/stats/resource_consumption_metrics.cpp @@ -61,7 +61,7 @@ inline void appendNonZeroMetric(BSONObjBuilder* builder, const char* name, long } // namespace bool ResourceConsumption::isMetricsCollectionEnabled() { - return gMeasureOperationResourceConsumption; + return gMeasureOperationResourceConsumption.isEnabledAndIgnoreFCV(); } bool ResourceConsumption::isMetricsAggregationEnabled() { @@ -69,7 +69,8 @@ bool ResourceConsumption::isMetricsAggregationEnabled() { } ResourceConsumption::ResourceConsumption() { - if (gAggregateOperationResourceConsumptionMetrics && !gMeasureOperationResourceConsumption) { + if (gAggregateOperationResourceConsumptionMetrics && + !gMeasureOperationResourceConsumption.isEnabledAndIgnoreFCV()) { LOGV2_FATAL_NOTRACE( 5091600, "measureOperationResourceConsumption feature flag must be enabled to use " diff --git a/src/mongo/db/stats/resource_consumption_metrics_test.cpp b/src/mongo/db/stats/resource_consumption_metrics_test.cpp index fbd83f88226..2f88f0072cc 100644 --- a/src/mongo/db/stats/resource_consumption_metrics_test.cpp +++ b/src/mongo/db/stats/resource_consumption_metrics_test.cpp @@ -36,12 +36,24 @@ #include "mongo/unittest/unittest.h" namespace mongo { +namespace { + +ServerParameter* getServerParameter(const std::string& name) { + const auto& spMap = ServerParameterSet::getGlobal()->getMap(); + const auto& spIt = spMap.find(name); + ASSERT(spIt != spMap.end()); + + auto* sp = spIt->second; + ASSERT(sp); + return sp; +} + class ResourceConsumptionMetricsTest : public ServiceContextTest { public: void setUp() { _opCtx = makeOperationContext(); - gMeasureOperationResourceConsumption = true; + ASSERT_OK(getServerParameter("measureOperationResourceConsumption")->setFromString("true")); gAggregateOperationResourceConsumptionMetrics = true; auto svcCtx = getServiceContext(); @@ -277,4 +289,5 @@ TEST_F(ResourceConsumptionMetricsTest, IncrementReadMetricsSecondary) { ASSERT_EQ(metricsCopy["db1"].secondaryMetrics.keysSorted, 16 + 256); } +} // namespace } // namespace mongo diff --git a/src/mongo/db/storage/SConscript b/src/mongo/db/storage/SConscript index 46dfc0ecc9b..c582d8f78a4 100644 --- a/src/mongo/db/storage/SConscript +++ b/src/mongo/db/storage/SConscript @@ -177,6 +177,7 @@ env.Library( '$BUILD_DIR/mongo/base', ], LIBDEPS_PRIVATE=[ + '$BUILD_DIR/mongo/idl/feature_flag', '$BUILD_DIR/mongo/idl/server_parameter', ], ) diff --git a/src/mongo/db/storage/storage_parameters.idl b/src/mongo/db/storage/storage_parameters.idl index fdf9686dce8..8aeab1a6ba0 100644 --- a/src/mongo/db/storage/storage_parameters.idl +++ b/src/mongo/db/storage/storage_parameters.idl @@ -86,6 +86,6 @@ server_parameters: feature_flags: featureFlagTimeSeriesCollection: - description: "Support for time-series collections" + description: "When enabled, support for time-series collections" cpp_varname: feature_flags::gTimeSeriesCollection default: false diff --git a/src/mongo/idl/SConscript b/src/mongo/idl/SConscript index dd13816c007..99cad300ea8 100644 --- a/src/mongo/idl/SConscript +++ b/src/mongo/idl/SConscript @@ -28,6 +28,22 @@ env.Library( ], ) +env.Library( + target='feature_flag', + source=[ + 'feature_flag.cpp', + ], + LIBDEPS=[ + '$BUILD_DIR/mongo/base', + 'server_parameter', + ], + LIBDEPS_PRIVATE=[ + '$BUILD_DIR/mongo/db/commands/feature_compatibility_parsers', + '$BUILD_DIR/mongo/util/options_parser/options_parser', + ], +) + + env.CppUnitTest( target='idl_test', source=[ @@ -47,9 +63,11 @@ env.CppUnitTest( LIBDEPS=[ '$BUILD_DIR/mongo/base', '$BUILD_DIR/mongo/db/namespace_string', + '$BUILD_DIR/mongo/db/server_options_core', '$BUILD_DIR/mongo/idl/idl_parser', '$BUILD_DIR/mongo/util/cmdline_utils/cmdline_utils', '$BUILD_DIR/mongo/util/options_parser/options_parser', + 'feature_flag', 'server_parameter', ], ) diff --git a/src/mongo/idl/feature_flag.cpp b/src/mongo/idl/feature_flag.cpp new file mode 100644 index 00000000000..6c1ca689e26 --- /dev/null +++ b/src/mongo/idl/feature_flag.cpp @@ -0,0 +1,123 @@ +/** + * Copyright (C) 2020-present MongoDB, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + * + * As a special exception, the copyright holders give permission to link the + * code of portions of this program with the OpenSSL library under certain + * conditions as described in each individual source file and distribute + * linked combinations including the program with the OpenSSL library. You + * must comply with the Server Side Public License in all respects for + * all of the code used other than as permitted herein. If you modify file(s) + * with this exception, you may extend this exception to your version of the + * file(s), but you are not obligated to do so. If you do not wish to do so, + * delete this exception statement from your version. If you delete this + * exception statement from all source files in the program, then also delete + * it in the license file. + */ + +#include "mongo/idl/feature_flag.h" +#include "mongo/db/commands/feature_compatibility_version_parser.h" +#include "mongo/util/debug_util.h" + +namespace mongo { + +// (Generic FCV reference): feature flag support +FeatureFlag::FeatureFlag(bool enabled, StringData versionString) + : _enabled(enabled), _version(ServerGlobalParams::FeatureCompatibility::kLatest) { + + // Verify the feature flag invariants. IDL binder verifies these hold but we add these checks to + // prevent incorrect direct instantiation. + // + // If default is true, then version should be present. + // If default is false, then no version is allowed. + if (kDebugBuild) { + if (enabled) { + dassert(!versionString.empty()); + } else { + dassert(versionString.empty()); + } + } + + if (!versionString.empty()) { + _version = FeatureCompatibilityVersionParser::parseVersion(versionString); + } +} + +bool FeatureFlag::isEnabled(const ServerGlobalParams::FeatureCompatibility& fcv) const { + if (!_enabled) { + return false; + } + + return fcv.isGreaterThanOrEqualTo(_version); +} + +bool FeatureFlag::isEnabledAndIgnoreFCV() const { + return _enabled; +} + +ServerGlobalParams::FeatureCompatibility::Version FeatureFlag::getVersion() const { + uassert(5111001, "Feature Flag is not enabled, cannot retrieve version", _enabled); + + return _version; +} + +void FeatureFlag::set(bool enabled) { + _enabled = enabled; +} + +FeatureFlagServerParameter::FeatureFlagServerParameter(StringData name, FeatureFlag& storage) + : ServerParameter(ServerParameterSet::getGlobal(), name, true, false), _storage(storage) {} + +void FeatureFlagServerParameter::append(OperationContext* opCtx, + BSONObjBuilder& b, + const std::string& name) { + bool enabled = _storage.isEnabledAndIgnoreFCV(); + + { + auto sub = BSONObjBuilder(b.subobjStart(name)); + sub.append("value"_sd, enabled); + + if (enabled) { + sub.append("version", + FeatureCompatibilityVersionParser::serializeVersion(_storage.getVersion())); + } + } +} + +Status FeatureFlagServerParameter::set(const BSONElement& newValueElement) { + bool newValue; + + if (auto status = newValueElement.tryCoerce(&newValue); !status.isOK()) { + return {status.code(), + str::stream() << "Failed setting " << name() << ": " << status.reason()}; + } + + _storage.set(newValue); + + return Status::OK(); +} + +Status FeatureFlagServerParameter::setFromString(const std::string& str) { + auto swNewValue = idl_server_parameter_detail::coerceFromString(str); + if (!swNewValue.isOK()) { + return swNewValue.getStatus(); + } + + _storage.set(swNewValue.getValue()); + + return Status::OK(); +} + +} // namespace mongo diff --git a/src/mongo/idl/feature_flag.h b/src/mongo/idl/feature_flag.h new file mode 100644 index 00000000000..2a09f4d4d08 --- /dev/null +++ b/src/mongo/idl/feature_flag.h @@ -0,0 +1,119 @@ +/** + * Copyright (C) 2020-present MongoDB, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the Server Side Public License, version 1, + * as published by MongoDB, Inc. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * Server Side Public License for more details. + * + * You should have received a copy of the Server Side Public License + * along with this program. If not, see + * . + * + * As a special exception, the copyright holders give permission to link the + * code of portions of this program with the OpenSSL library under certain + * conditions as described in each individual source file and distribute + * linked combinations including the program with the OpenSSL library. You + * must comply with the Server Side Public License in all respects for + * all of the code used other than as permitted herein. If you modify file(s) + * with this exception, you may extend this exception to your version of the + * file(s), but you are not obligated to do so. If you do not wish to do so, + * delete this exception statement from your version. If you delete this + * exception statement from all source files in the program, then also delete + * it in the license file. + */ + +#pragma once + +#include +#include + +#include "mongo/db/commands/feature_compatibility_version_parser.h" +#include "mongo/db/server_options.h" +#include "mongo/idl/server_parameter.h" +#include "mongo/idl/server_parameter_with_storage.h" + +namespace mongo { + +/** + * FeatureFlag contains information about whether a feature flag is enabled and what version it was + * released. + * + * FeatureFlag represents the state of a feature flag and whether it is associated with a particular + * version. It is not implicitly convertible to bool to force all call sites to make a decision + * about what check to use. + * + * It is only set at startup. + */ +class FeatureFlag { + friend class FeatureFlagServerParameter; + +public: + FeatureFlag(bool enabled, StringData versionString); + + /** + * Returns true if the flag is set to true and enabled for this FCV version. + */ + bool isEnabled(const ServerGlobalParams::FeatureCompatibility& fcv) const; + + /** + * Returns true if this flag is enabled regardless of the current FCV version. + * + * isEnabled() is prefered over this function since it will prevent upgrade/downgrade issues. + */ + bool isEnabledAndIgnoreFCV() const; + + /** + * Return the version associated with this feature flag. + * + * Throws if feature is not enabled. + */ + ServerGlobalParams::FeatureCompatibility::Version getVersion() const; + +private: + void set(bool enabled); + +private: + bool _enabled; + ServerGlobalParams::FeatureCompatibility::Version _version; +}; + +/** + * Specialization of ServerParameter for FeatureFlags used by IDL generator. + */ +class FeatureFlagServerParameter : public ServerParameter { +public: + FeatureFlagServerParameter(StringData name, FeatureFlag& storage); + + /** + * Encode the setting into BSON object. + * + * Typically invoked by {getParameter:...} to produce a dictionary + * of ServerParameter settings. + */ + void append(OperationContext* opCtx, BSONObjBuilder& b, const std::string& name) final; + + /** + * Update the underlying value using a BSONElement + * + * Allows setting non-basic values (e.g. vector) + * via the {setParameter: ...} call. + */ + Status set(const BSONElement& newValueElement) final; + + /** + * Update the underlying value from a string. + * + * Typically invoked from commandline --setParameter usage. + */ + Status setFromString(const std::string& str) final; + +private: + FeatureFlag& _storage; +}; + +} // namespace mongo diff --git a/src/mongo/idl/feature_flag_test.cpp b/src/mongo/idl/feature_flag_test.cpp index cb111756272..124bcd56482 100644 --- a/src/mongo/idl/feature_flag_test.cpp +++ b/src/mongo/idl/feature_flag_test.cpp @@ -27,9 +27,11 @@ * it in the license file. */ +#include "mongo/bson/bsonobjbuilder.h" #include "mongo/platform/basic.h" #include "mongo/idl/feature_flag_test_gen.h" +#include "mongo/unittest/bson_test_util.h" #include "mongo/unittest/unittest.h" namespace mongo { @@ -46,14 +48,137 @@ ServerParameter* getServerParameter(const std::string& name) { return sp; } +class FeatureFlagTest : public unittest::Test { + +protected: + void setUp() override; + +protected: + ServerParameter* _featureFlagBlender{nullptr}; + ServerParameter* _featureFlagSpoon{nullptr}; +}; + + +void FeatureFlagTest::setUp() { + // Set common flags which test the version string to true + _featureFlagBlender = getServerParameter("featureFlagBlender"); + ASSERT_OK(_featureFlagBlender->setFromString("true")); + + _featureFlagSpoon = getServerParameter("featureFlagSpoon"); + ASSERT_OK(_featureFlagSpoon->setFromString("true")); + + ASSERT(feature_flags::gFeatureFlagBlender.isEnabledAndIgnoreFCV() == true); + ASSERT(feature_flags::gFeatureFlagSpoon.isEnabledAndIgnoreFCV() == true); + + Test::setUp(); +} + +// Sanity check feature flags TEST(IDLFeatureFlag, Basic) { - // true is set by "default" attribute in the IDL file. - ASSERT_EQ(feature_flags::gFeatureFlagToaster, true); + // false is set by "default" attribute in the IDL file. + ASSERT(feature_flags::gFeatureFlagToaster.isEnabledAndIgnoreFCV() == false); auto* featureFlagToaster = getServerParameter("featureFlagToaster"); - ASSERT_OK(featureFlagToaster->setFromString("false")); - ASSERT_EQ(feature_flags::gFeatureFlagToaster, false); + ASSERT_OK(featureFlagToaster->setFromString("true")); + ASSERT(feature_flags::gFeatureFlagToaster.isEnabledAndIgnoreFCV() == true); ASSERT_NOT_OK(featureFlagToaster->setFromString("alpha")); + + ASSERT(feature_flags::gFeatureFlagToaster.getVersion() == + ServerGlobalParams::FeatureCompatibility::Version::kVersion49); +} + +// Verify getVersion works correctly when enabled and not enabled +TEST_F(FeatureFlagTest, Version) { + ASSERT(feature_flags::gFeatureFlagBlender.getVersion() == + ServerGlobalParams::FeatureCompatibility::Version::kVersion49); + + // NOTE: if you are hitting this assertion, the version in feature_flag_test.idl may need to be + // changed to match the current kLastLTS + // (Generic FCV reference): feature flag test + ASSERT(feature_flags::gFeatureFlagSpoon.getVersion() == + ServerGlobalParams::FeatureCompatibility::kLastLTS); + + ASSERT_OK(_featureFlagBlender->setFromString("false")); + ASSERT(feature_flags::gFeatureFlagBlender.isEnabledAndIgnoreFCV() == false); + ASSERT_NOT_OK(_featureFlagBlender->setFromString("alpha")); + + ASSERT_THROWS(feature_flags::gFeatureFlagBlender.getVersion(), AssertionException); +} + +// Test feature flag server parameters are serialized correctly +TEST_F(FeatureFlagTest, ServerStatus) { + { + ASSERT_OK(_featureFlagBlender->setFromString("true")); + ASSERT(feature_flags::gFeatureFlagBlender.isEnabledAndIgnoreFCV() == true); + + BSONObjBuilder builder; + + _featureFlagBlender->append(nullptr, builder, "blender"); + + ASSERT_BSONOBJ_EQ(builder.obj(), + BSON("blender" << BSON("value" << true << "version" + << "4.9"))); + } + + { + ASSERT_OK(_featureFlagBlender->setFromString("false")); + ASSERT(feature_flags::gFeatureFlagBlender.isEnabledAndIgnoreFCV() == false); + + BSONObjBuilder builder; + + _featureFlagBlender->append(nullptr, builder, "blender"); + + ASSERT_BSONOBJ_EQ(builder.obj(), BSON("blender" << BSON("value" << false))); + } +} + +// Test feature flags are enabled and not enabled based on fcv +TEST_F(FeatureFlagTest, IsEnabledTrue) { + // Test FCV checks with enabled flag + // Test newest version + serverGlobalParams.mutableFeatureCompatibility.setVersion( + ServerGlobalParams::FeatureCompatibility::Version::kVersion49); + + ASSERT_TRUE( + feature_flags::gFeatureFlagBlender.isEnabled(serverGlobalParams.featureCompatibility)); + ASSERT_TRUE( + feature_flags::gFeatureFlagSpoon.isEnabled(serverGlobalParams.featureCompatibility)); + + // Test oldest version + // (Generic FCV reference): feature flag test + serverGlobalParams.mutableFeatureCompatibility.setVersion( + ServerGlobalParams::FeatureCompatibility::kLastLTS); + + ASSERT_FALSE( + feature_flags::gFeatureFlagBlender.isEnabled(serverGlobalParams.featureCompatibility)); + ASSERT_TRUE( + feature_flags::gFeatureFlagSpoon.isEnabled(serverGlobalParams.featureCompatibility)); +} + +// Test feature flags are disabled regardless of fcv +TEST_F(FeatureFlagTest, IsEnabledFalse) { + // Test FCV checks with disabled flag + // Test newest version + ASSERT_OK(_featureFlagBlender->setFromString("false")); + ASSERT_OK(_featureFlagSpoon->setFromString("false")); + + serverGlobalParams.mutableFeatureCompatibility.setVersion( + ServerGlobalParams::FeatureCompatibility::Version::kVersion49); + + ASSERT_FALSE( + feature_flags::gFeatureFlagBlender.isEnabled(serverGlobalParams.featureCompatibility)); + ASSERT_FALSE( + feature_flags::gFeatureFlagSpoon.isEnabled(serverGlobalParams.featureCompatibility)); + + // Test oldest version + // (Generic FCV reference): feature flag test + serverGlobalParams.mutableFeatureCompatibility.setVersion( + ServerGlobalParams::FeatureCompatibility::kLastLTS); + + ASSERT_FALSE( + feature_flags::gFeatureFlagBlender.isEnabled(serverGlobalParams.featureCompatibility)); + ASSERT_FALSE( + feature_flags::gFeatureFlagSpoon.isEnabled(serverGlobalParams.featureCompatibility)); } } // namespace diff --git a/src/mongo/idl/feature_flag_test.idl b/src/mongo/idl/feature_flag_test.idl index c756ca5a90f..fc76119c208 100644 --- a/src/mongo/idl/feature_flag_test.idl +++ b/src/mongo/idl/feature_flag_test.idl @@ -33,4 +33,18 @@ feature_flags: featureFlagToaster: description: "Create a feature flag" cpp_varname: gFeatureFlagToaster + default: false + + featureFlagBlender: + description: "Create a feature flag" + cpp_varname: gFeatureFlagBlender + default: true + # The version should be a valid FCV not equal to kLastLTS in src/mongo/db/commands/feature_compatibility_version_parser.h + version: 4.9 + + featureFlagSpoon: + description: "Create a feature flag" + cpp_varname: gFeatureFlagSpoon default: true + # The version should match kLastLTS in src/mongo/db/commands/feature_compatibility_version_parser.h + version: 4.4 -- cgit v1.2.1