From fb1581d0b96eb0e54ebbda5e3463390f4c8c171f Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Wed, 8 Mar 2023 11:39:30 +0200 Subject: [PATCH] Fix setting "image_creation_threshold" setting in tenant config. (#3762) We have a few tests that try to set image_creation_threshold, but it didn't actually have any effect because we were missing some critical code to load the setting from config file into memory. The two modified tests in `test_remote_storage.py perform compaction and GC, and assert that GC removes some layers. That only happens if new image layers are created by the compaction. The tests explicitly disabled image layer creation by setting image_creation_threshold to a high value, but it didn't take effect because reading image_creation_threshold from config file was broken, which is why the test worked. Fix the test to set image_creation_threshold low, instead, so that GC has work to do. Change 'test_tenant_conf.py' so that it exercises the added code. This might explain why we're apparently missing test coverage for GC (issue #3415), although I didn't try to address that here, nor did I check if this improves the it. --- pageserver/src/config.rs | 6 ++++++ test_runner/regress/test_remote_storage.py | 10 +++++----- test_runner/regress/test_tenant_conf.py | 7 ++++--- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/pageserver/src/config.rs b/pageserver/src/config.rs index 309e5367a4..7442814c43 100644 --- a/pageserver/src/config.rs +++ b/pageserver/src/config.rs @@ -698,6 +698,12 @@ impl PageServerConf { Some(parse_toml_u64("compaction_threshold", compaction_threshold)?.try_into()?); } + if let Some(image_creation_threshold) = item.get("image_creation_threshold") { + t_conf.image_creation_threshold = Some( + parse_toml_u64("image_creation_threshold", image_creation_threshold)?.try_into()?, + ); + } + if let Some(gc_horizon) = item.get("gc_horizon") { t_conf.gc_horizon = Some(parse_toml_u64("gc_horizon", gc_horizon)?); } diff --git a/test_runner/regress/test_remote_storage.py b/test_runner/regress/test_remote_storage.py index 90d69c7b0e..24db80c7cc 100644 --- a/test_runner/regress/test_remote_storage.py +++ b/test_runner/regress/test_remote_storage.py @@ -233,8 +233,8 @@ def test_remote_storage_upload_queue_retries( # disable background compaction and GC. We invoke it manually when we want it to happen. "gc_period": "0s", "compaction_period": "0s", - # don't create image layers, that causes just noise - "image_creation_threshold": "10000", + # create image layers eagerly, so that GC can remove some layers + "image_creation_threshold": "1", } ) @@ -301,7 +301,7 @@ def test_remote_storage_upload_queue_retries( # Create more churn to generate all upload ops. # The checkpoint / compact / gc ops will block because they call remote_client.wait_completion(). - # So, run this in a differen thread. + # So, run this in a different thread. churn_thread_result = [False] def churn_while_failpoints_active(result): @@ -395,8 +395,8 @@ def test_remote_timeline_client_calls_started_metric( # disable background compaction and GC. We invoke it manually when we want it to happen. "gc_period": "0s", "compaction_period": "0s", - # don't create image layers, that causes just noise - "image_creation_threshold": "10000", + # create image layers eagerly, so that GC can remove some layers + "image_creation_threshold": "1", } ) diff --git a/test_runner/regress/test_tenant_conf.py b/test_runner/regress/test_tenant_conf.py index e087891bba..c5f9a3d157 100644 --- a/test_runner/regress/test_tenant_conf.py +++ b/test_runner/regress/test_tenant_conf.py @@ -129,6 +129,7 @@ tenant_config={checkpoint_distance = 10000, compaction_target_size = 1048576}""" "checkpoint_distance": "15000", "gc_period": "80sec", "compaction_period": "80sec", + "image_creation_threshold": "2", } env.neon_cli.config_tenant( tenant_id=tenant, @@ -149,7 +150,7 @@ tenant_config={checkpoint_distance = 10000, compaction_target_size = 1048576}""" "compaction_threshold": 10, "gc_horizon": 67108864, "gc_period": 80, - "image_creation_threshold": 3, + "image_creation_threshold": 2, "pitr_interval": 604800, }.items() ), f"Unexpected res: {res}" @@ -174,7 +175,7 @@ tenant_config={checkpoint_distance = 10000, compaction_target_size = 1048576}""" assert updated_effective_config["compaction_target_size"] == 1048576 assert updated_effective_config["compaction_threshold"] == 10 assert updated_effective_config["gc_horizon"] == 67108864 - assert updated_effective_config["image_creation_threshold"] == 3 + assert updated_effective_config["image_creation_threshold"] == 2 assert updated_effective_config["pitr_interval"] == "7days" # restart the pageserver and ensure that the config is still correct @@ -195,7 +196,7 @@ tenant_config={checkpoint_distance = 10000, compaction_target_size = 1048576}""" "compaction_threshold": 10, "gc_horizon": 67108864, "gc_period": 80, - "image_creation_threshold": 3, + "image_creation_threshold": 2, "pitr_interval": 604800, }.items() ), f"Unexpected res: {res}"