Merge pull request redpanda-data#16077 from dotnwat/default-sm-enable

storage: don't enable non-strict retention by default

Author: dotnwat

src/v/config/configuration.cc

@@ -2061,15 +2061,16 @@ configuration::configuration()
       "past the local retention limit, and will be trimmed automatically as "
       "storage reaches the configured target size.",
       {.needs_restart = needs_restart::no, .visibility = visibility::user},
-      false)
+      false,
+      property<bool>::noop_validator,
+      legacy_default<bool>(true, legacy_version{9}))
   , retention_local_strict_override(
       *this,
       "retention_local_strict_override",
       "Trim log data when a cloud topic reaches its local retention limit. "
       "When this option is disabled Redpanda will allow partitions to grow "
       "past the local retention limit, and will be trimmed automatically as "
-      "storage reaches the configured target size. This option is ignored and "
-      "deprecated in versions >= v23.3.",
+      "storage reaches the configured target size.",
       {.needs_restart = needs_restart::no, .visibility = visibility::user},
       true)
   , retention_local_target_capacity_bytes(

src/v/storage/disk_log_impl.cc

@@ -1009,7 +1009,9 @@ gc_config disk_log_impl::maybe_override_retention_config(gc_config cfg) const {
      * don't override with local retention settings--let partition data expand
      * up to standard retention settings.
      */
-    if (!config::shard_local_cfg().retention_local_strict()) {
+    if (
+      !config::shard_local_cfg().retention_local_strict()
+      || !config::shard_local_cfg().retention_local_strict_override()) {
         vlog(
           gclog.trace,
           "[{}] Skipped retention override for topic with remote write "

tests/rptest/tests/cluster_config_test.py

@@ -1941,24 +1941,20 @@ def test_removal_of_legacy_default_defaulted(self, wipe_cache: bool):
         # upgraded clusters.
         self._upgrade(wipe_cache, self.intermediate_version)
         self._check_value_everywhere("space_management_enable", False)
-        self._check_value_everywhere("retention_local_strict", True)
 
-        # in >=23.3 (using upstream build) space management should be enabled by
-        # default provided that it wasn't explicitly disabled in 23.2. in this
-        # case no configs were changed so it should be enabled now.
+        # in >=23.3 space management should be enabled by default provided that
+        # it wasn't explicitly disabled in 23.2.
         self._upgrade(wipe_cache)
         self._check_value_everywhere("space_management_enable", True)
-        self._check_value_everywhere("retention_local_strict", False)
 
         # survives a restart
         self.redpanda.restart_nodes(self.redpanda.nodes)
         self._check_value_everywhere("space_management_enable", True)
-        self._check_value_everywhere("retention_local_strict", False)
 
     @cluster(num_nodes=3)
     @parametrize(wipe_cache=True)
     @parametrize(wipe_cache=False)
-    def test_removal_of_legacy_default_disabled(self, wipe_cache: bool):
+    def test_removal_of_legacy_default_overriden(self, wipe_cache: bool):
         # in 23.1 space management feature does not exist
         old_version, _ = self.installer.latest_for_line(self.legacy_version)
         self.installer.install(self.redpanda.nodes, old_version)
@@ -1968,26 +1964,21 @@ def test_removal_of_legacy_default_disabled(self, wipe_cache: bool):
         # upgraded clusters.
         self._upgrade(wipe_cache, self.intermediate_version)
         self._check_value_everywhere("space_management_enable", False)
-        self._check_value_everywhere("retention_local_strict", True)
 
-        # the interface seems to ignore setting a value to its current value. so
-        # we enable then disable to make sure it gets through.
+        # we need to toggle it to get it to stick since the api seems to not
+        # change the underlying value explicitly if its default is that value.
+        # the legacy default bits here are to blame for the weirdness i presume
         self.redpanda.set_cluster_config({"space_management_enable": True})
         self.redpanda.set_cluster_config({"space_management_enable": False})
-        self.redpanda.set_cluster_config({"retention_local_strict": False})
-        self.redpanda.set_cluster_config({"retention_local_strict": True})
 
-        # in >=23.3 (using upstream build) space management should be enabled by
-        # default provided that it wasn't explicitly disabled in 23.2. in this
-        # case we disabled it in 23.2 state so it should still be disabled here.
+        # in >=23.3 space management should be enabled by default provided that
+        # it wasn't explicitly disabled in 23.2.
         self._upgrade(wipe_cache)
         self._check_value_everywhere("space_management_enable", False)
-        self._check_value_everywhere("retention_local_strict", True)
 
         # survives a restart
         self.redpanda.restart_nodes(self.redpanda.nodes)
         self._check_value_everywhere("space_management_enable", False)
-        self._check_value_everywhere("retention_local_strict", True)
 
 
 class ClusterConfigUnknownTest(RedpandaTest):