Allow creating config for attached tenant

[koivunej]

This might be a bit controversial change but I thought to write the code takes about as much as writing a proper issue.

Currently attach doesn't write a tenant config, because we don't back it up in the first place. The current implementation of Tenant::persist_tenant_config does not allow changing tenant's configuration through the http api which will fail because the file wasn't created on attach and OpenOptions::truncate(true).write(true).create_new(false) is used.

I think this patch allows for least controversial middle ground which enables changing tenant configuration even for attached tenants (not just created tenants).

I also removed the conditional fsync on the directory because it's now a hazard because the file might be created when !first_save.

See also:

• Fix tenant config & tenant relocation integration #1555

Does not address:

• Tenant config file update isn't durable which may result in empty/corrupted state #3062

Found while trying to reproduce #3387.

[koivunej]

Unrelated regress(release):

• FAILED test_runner/regress/test_tenant_size.py::test_get_tenant_size_with_multiple_branches

[SomeoneToIgnore]

I find the config-related changes always somewhat confusing, since the only real source of truth and use cases now is the console (control plane later?).

Without their specs/demands/stories, all these changes are guessing.
The PR is not bad per se, but I cannot say if it makes any sense due to the above.

[LizardWizzard]

Thanks! I've noticed that attach doesnt create config files, but didnt realise that config changes can be broken because of that. As an alternative we can always persist default config with attach. And as I see it attach can have tenant knobs as parameters so they're added to the config during the attach itself

I find the config-related changes always somewhat confusing, since the only real source of truth and use cases now is the console (control plane later?).

Without their specs/demands/stories, all these changes are guessing.
The PR is not bad per se, but I cannot say if it makes any sense due to the above.

I think this is not really relevant here. The PR fixes a bug which is observed when you call attach and then try to change config with usual config endpoint. I can confirm that this patch fixes the issue (verified locally)

[koivunej]

@SomeoneToIgnore and @LizardWizzard please peek again for the fixes, tried to make isolated commits.

Workflow failure is not related.

[LizardWizzard]

Looks good to me, thanks! Two minor comments

[problame]

If we're changing the in-memory config and then fail to persist it, we report an error to the client but still run with the new config. Is this intentional?

[koivunej]

That part was not explicitly changed on this PR but yeah it seems quite suboptimal. The update_tenant_config does a merge all of the given values ... So actually the value we end up writing to the disk is not the active tenant config. Raah, I'll open an issue. Well, perhaps investigate a bit more before calling it :)

[koivunej]

Modifying the added test in this PR to instead of changing gc_horizon twice to in different http PUT requests:

1. set gc_horizon
2. set pitr_interval

Will write to disk as last version:

# This file contains a specific per-tenant's config.
#  It is read in case of pageserver restart.

[tenant_config]
pitr_interval = "0s"

But adding logging to Tenant::update_tenant_config shows that the tenant config is first updated to:

2023-01-27T15:27:39.514582Z  INFO tenant_config{tenant=db6238f481f8459ce03705f1fa96ab42}: updated config TenantConfOpt {
    checkpoint_distance: None,
    checkpoint_timeout: None,
    compaction_target_size: None,
    compaction_period: None,
    compaction_threshold: None,
    gc_horizon: Some(
        1000000,
    ),
    gc_period: None,
    image_creation_threshold: None,
    pitr_interval: None,
    walreceiver_connect_timeout: None,
    lagging_wal_timeout: None,
    max_lsn_wal_lag: None,
    trace_read_requests: None,
}

Then by the second request:

2023-01-27T15:27:39.521233Z  INFO tenant_config{tenant=db6238f481f8459ce03705f1fa96ab42}: updated config TenantConfOpt {
    checkpoint_distance: None,
    checkpoint_timeout: None,
    compaction_target_size: None,
    compaction_period: None,
    compaction_threshold: None,
    gc_horizon: Some(
        1000000,
    ),
    gc_period: None,
    image_creation_threshold: None,
    pitr_interval: Some(
        0ns,
    ),
    walreceiver_connect_timeout: None,
    lagging_wal_timeout: None,
    max_lsn_wal_lag: None,
    trace_read_requests: None,
}

Creating a new issue, including your comment.

[koivunej]

Created two new issues, linked below.