Skip to content

Commit

Permalink
Fix setting "image_creation_threshold" setting in tenant config. (#3762)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
hlinnaka authored Mar 8, 2023
1 parent 02b8e0e commit fb1581d
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 8 deletions.
6 changes: 6 additions & 0 deletions pageserver/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)?);
}
Expand Down
10 changes: 5 additions & 5 deletions test_runner/regress/test_remote_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
}
)

Expand Down Expand Up @@ -301,7 +301,7 @@ def get_queued_count(file_kind, op_kind):

# 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):
Expand Down Expand Up @@ -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",
}
)

Expand Down
7 changes: 4 additions & 3 deletions test_runner/regress/test_tenant_conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ def test_tenant_config(neon_env_builder: NeonEnvBuilder):
"checkpoint_distance": "15000",
"gc_period": "80sec",
"compaction_period": "80sec",
"image_creation_threshold": "2",
}
env.neon_cli.config_tenant(
tenant_id=tenant,
Expand All @@ -149,7 +150,7 @@ def test_tenant_config(neon_env_builder: NeonEnvBuilder):
"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}"
Expand All @@ -174,7 +175,7 @@ def test_tenant_config(neon_env_builder: NeonEnvBuilder):
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
Expand All @@ -195,7 +196,7 @@ def test_tenant_config(neon_env_builder: NeonEnvBuilder):
"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}"
Expand Down

0 comments on commit fb1581d

Please sign in to comment.