Remove init from pageserver CLI

[SomeoneToIgnore]

Removes pageserver init CLI command.

• pageserver.toml config file is now only created or updated when pageserver start command gets --update-config flag.
  id is still a protected field that is not allowed to be updated in the existing config file

• initial tenant and timeline creation is now only done in neon_local init, for that entire pageserver is started to serve two HTTP creation commands and then stopped.

Cloud does not use initial timelines at all, Python tests continue using the same "initial timeline" implicitly brought by the neon_local init invocation

[hlinnaka]

Ah, that's a patch to my liking 😄 :

 9 files changed, 218 insertions(+), 340 deletions(-)

Does starting the pageserver binary now implicitly create the target directory if it doesn't already exist? If so, that seems a bit fragile, I think I'd prefer to get an error, if for example the repository directory doesn't exist because you forgot to mount the filesystem it's in.

[hlinnaka]

Note that the old code used print!() rather than println!(), so this will print an extra empty line

[SomeoneToIgnore]

Yes, yet I've found that more natural when dealing with failing pageserver:

with empty line:

Work/neon/neon kb/pageserver-no-init:origin/kb/pageserver-no-init*​ ⇡❯ rm -rf .neon/ && ./target/debug/neon_local init
Starting pageserver at '127.0.0.1:64000' in '.neon'
thread 'main' panicked at '((((((((((((((((', pageserver/src/bin/pageserver.rs:41:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/panicking.rs:142:14
   2: pageserver::main
             at ./pageserver/src/bin/pageserver.rs:41:9
   3: core::ops::function::FnOnce::call_once
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
pageserver init failed: Pageserver failed to start. See console output and '.neon/pageserver.log' for details.

without:

Work/neon/neon kb/pageserver-no-init:origin/kb/pageserver-no-init*​ ⇡❯ rm -rf .neon/ && ./target/debug/neon_local init
Starting pageserver at '127.0.0.1:64000' in '.neon'thread 'main' panicked at '((((((((((((((((', pageserver/src/bin/pageserver.rs:41:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/panicking.rs:142:14
   2: pageserver::main
             at ./pageserver/src/bin/pageserver.rs:41:9
   3: core::ops::function::FnOnce::call_once
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
pageserver init failed: Pageserver failed to start. See console output and '.neon/pageserver.log' for details.

[hlinnaka]

Why is this change needed?

[SomeoneToIgnore]

Option<TimelineInfo> is None according to our logic only if we fail to create a timeline:

neon/pageserver/src/http/routes.rs

Line 244 in e593cba

None => json_response(StatusCode::CONFLICT, ())?,

So instead of doing various .unwrap() or other error handling above the client, I've simplified it to validating the presence here.

[SomeoneToIgnore]

Does starting the pageserver binary now implicitly create the target directory if it doesn't already exist?

The tenants/ one? Yes, now we do crate it implicitly. Do you think that base path should be mounted instead anyway? We'll fail if that's not done, so either I don't understand it or it should be ok.

We could somehow alter --update-config to something more generic and allow dir creation along with that operation too, yet it slightly becomes to resemble --init flag now 🙂

[SomeoneToIgnore]

A PR to fix e2e tests: https://github.com/neondatabase/cloud/pull/2058
Other tests seem to flack for known, daemonize-related issues.

[hlinnaka]

Does starting the pageserver binary now implicitly create the target directory if it doesn't already exist?

The tenants/ one? Yes, now we do crate it implicitly. Do you think that base path should be mounted instead anyway? We'll fail if that's not done, so either I don't understand it or it should be ok.

I meant the directory that contains tenants/. The data directory looks like this:

<path>/tenants/
<path>/pageserver.log
<path>/pageserver.pid
<path>/pageserver.toml

The log file is created on startup. The pid-file is also created. The config file isn't mandatory I guess. But the question is, if <path> itself doesn't exist, do we create it automatically, or error out? And if <path>/tenants doesn't already exist, do we create it or error out?

Imagine that pageserver runs on a host with two filesystems. The root filesystem, and 'data' that holds all the pageserver data. At server startup, we do this:

1. mount /data
2. pageserver --workdir /data/pageserver-data

What happens if the mount /data command fails? Does pageserver still start up and automatically create a new empty data directory as /data/pageserver-data/, now on the root filesystem? Or does it refuse to start? I think refusing to start is better.

We could somehow alter --update-config to something more generic and allow dir creation along with that operation too, yet it slightly becomes to resemble --init flag now slightly_smiling_face

Yeah, I think an explicit init option still makes sense. But without creating the initial tenant and timeline.

[SomeoneToIgnore]

I meant the directory that contains tenants/. The data directory looks like this:

Now I understand.
This is a workdir, the one that's being checked for existence implicitly, with .canonicalize:

neon/pageserver/src/bin/pageserver.rs

Lines 107 to 111 in d29c545

	let workdir = Path::new(arg_matches.value_of("workdir").unwrap_or(".neon"));
	let workdir = workdir
	.canonicalize()
	.with_context(|| format!("Error opening workdir '{}'", workdir.display()))?;
	let cfg_file_path = workdir.join("pageserver.toml");

If we try to canonicalize or do env::set_current_dir (we do that later below), we fail for non-existent path.
So we don't create the workdir, rather implicitly failing.

This means I can avoid changing the code to be --init, after all.

[LizardWizzard]

Looks good to me

I’m for avoiding init and bailing when passed directory doesn’t exist. Though I’m not sure that this is followed by safekeepers, need to check

UPD: safekeeper will start even without passed datadir argument. It will fallback to current working directory. It is the simplest behavior, but if we want to guard against such config errors we better use the same behavior everywhere. Thoughts?