Merge pull request #24450 from bashtanov/handling-missing-features-in…

…-snapshot

Handling missing features in snapshot

src/v/cluster/feature_backend.h

@@ -41,7 +41,7 @@ class feature_backend {
     ss::future<> fill_snapshot(controller_snapshot&) const;
     ss::future<> apply_snapshot(model::offset, const controller_snapshot&);
 
-    /// this functions deal with the snapshot stored in local kvstore (in
+    /// these functions deal with the snapshot stored in local kvstore (in
     /// contrast to fill/apply_snapshot which deal with the feature table data
     /// in the replicated controller snapshot).
     bool has_local_snapshot();

src/v/features/feature_table.cc

@@ -197,23 +197,23 @@ bool is_major_version_upgrade(
 static std::array test_extra_schema{
   // For testing, a feature that does not auto-activate
   feature_spec{
-    cluster::cluster_version{2001},
+    TEST_VERSION,
     "__test_alpha",
     feature::test_alpha,
     feature_spec::available_policy::explicit_only,
     feature_spec::prepare_policy::always},
 
   // For testing, a feature that auto-activates
   feature_spec{
-    cluster::cluster_version{2001},
+    TEST_VERSION,
     "__test_bravo",
     feature::test_bravo,
     feature_spec::available_policy::always,
     feature_spec::prepare_policy::always},
 
   // For testing, a feature that auto-activates
   feature_spec{
-    cluster::cluster_version{2001},
+    TEST_VERSION,
     "__test_charlie",
     feature::test_charlie,
     feature_spec::available_policy::new_clusters_only,

src/v/features/feature_table.h

@@ -82,8 +82,13 @@ enum class feature : std::uint64_t {
 // controller messages for unknown features (unexpected), and controller
 // messages that refer to features that have been retired.
 //
-// retired does *not* mean the functionality is gone: it just means it
+// Retired does *not* mean the functionality is gone: it just means it
 // is no longer guarded by a feature flag.
+//
+// All feature checks need to be removed one version before the feature is
+// retired. That's because during upgrade, when a mixed version cluster is
+// running, the older version nodes may read a snaphot from the newer version
+// and get the feature automatically enabled.
 inline const std::unordered_set<std::string_view> retired_features = {
   "central_config",
   "consumer_offsets",
@@ -158,6 +163,8 @@ constexpr cluster::cluster_version to_cluster_version(release_version rv) {
     vassert(false, "Invalid release_version");
 }
 
+constexpr cluster::cluster_version TEST_VERSION{2001};
+
 bool is_major_version_upgrade(
   cluster::cluster_version from, cluster::cluster_version to);
 

src/v/features/feature_table_snapshot.cc

@@ -54,13 +54,25 @@ void feature_table_snapshot::apply(feature_table& ft) const {
           });
         if (snap_state_iter == states.end()) {
             // The feature table refers to a feature name that the snapshot
-            // doesn't mention: this is normal on upgrade.  The feature will
-            // remain in its default-initialized state.
+            // doesn't mention: this is normal on upgrade.
+            if (spec.require_version <= version) {
+                // The feature was introduced no later than the agreed version,
+                // which is no later than the version of the broker that took
+                // the snapshot. So it only can be missing because it has been
+                // retired and thus deemed active.
+                cur_state._state = feature_state::state::active;
+            } else {
+                // Otherwise the feature was introduced after the agreed
+                // version, so it can only be disabled before we reach it.
+                cur_state._state = feature_state::state::unavailable;
+            }
             vlog(
               featureslog.debug,
-              "No state for feature '{}' in snapshot, upgrade in progress?",
-              spec.name);
-            continue;
+              "No state for feature '{}' in snapshot v{}, upgrade in progress? "
+              "Assuming the feature state is {}",
+              spec.name,
+              version,
+              cur_state._state);
         } else {
             if (
               spec.require_version

src/v/features/tests/feature_table_test.cc

@@ -121,10 +121,10 @@ FIXTURE_TEST(feature_table_basic, feature_table_fixture) {
       ft.get_state(feature::test_alpha).get_state()
       == feature_state::state::unavailable);
 
-    // The dummy test features requires version 2001.  The feature
+    // The dummy test features requires version TEST_VERSION. The feature
     // should go available, but not any further: the feature table
     // relies on external stimulus to actually activate features.
-    set_active_version(cluster_version{2001});
+    set_active_version(TEST_VERSION);
 
     BOOST_REQUIRE(
       ft.get_state(feature::test_alpha).get_state()
@@ -274,7 +274,7 @@ FIXTURE_TEST(feature_uniqueness, feature_table_fixture) {
  * but also activates elegible features.
  */
 FIXTURE_TEST(feature_table_bootstrap, feature_table_fixture) {
-    bootstrap_active_version(cluster_version{2001});
+    bootstrap_active_version(TEST_VERSION);
 
     // A non-auto-activating feature should remain in available state:
     // explicit_only features always require explicit activation, even
@@ -334,6 +334,40 @@ FIXTURE_TEST(feature_table_old_snapshot, feature_table_fixture) {
       == feature_state::state::active);
 }
 
+// Test that applying an old snapshot disables features that we only enabled in
+// this version.
+FIXTURE_TEST(feature_table_old_snapshot_missing, feature_table_fixture) {
+    bootstrap_active_version(TEST_VERSION);
+
+    features::feature_table_snapshot snapshot;
+    snapshot.version = cluster::cluster_version{ft.get_active_version() - 1};
+    snapshot.states = {};
+    snapshot.apply(ft);
+
+    // A feature with explicit available_policy should be activated by the
+    // snapshot.
+    BOOST_CHECK(
+      ft.get_state(feature::test_alpha).get_state()
+      == feature_state::state::unavailable);
+}
+
+// Test that applying a snapshot of the same version with a missing feature
+// enables it, as we assume it was retired in the next version.
+FIXTURE_TEST(feature_table_new_snapshot_missing, feature_table_fixture) {
+    bootstrap_active_version(TEST_VERSION);
+
+    features::feature_table_snapshot snapshot;
+    snapshot.version = cluster::cluster_version{ft.get_active_version()};
+    snapshot.states = {};
+    snapshot.apply(ft);
+
+    // A feature with explicit available_policy should be activated by the
+    // snapshot.
+    BOOST_CHECK(
+      ft.get_state(feature::test_alpha).get_state()
+      == feature_state::state::active);
+}
+
 FIXTURE_TEST(feature_table_trial_license_test, feature_table_fixture) {
     const char* sample_valid_license = std::getenv("REDPANDA_SAMPLE_LICENSE");
     if (sample_valid_license == nullptr) {