diff --git a/CHANGELOG.md b/CHANGELOG.md index 1cbc2e0..20ef16d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,45 @@ expose. ## [Unreleased] +### Changed + +- **`--useage` now has a hard floor of 24 hours, and defaults to 24** instead + of 0. Below the floor a run is refused outright rather than warned about, for + `--dry-run` as well as delete. + + Why a floor and not a warning: the age window is the *only* thing standing + between a run and live data. ClickHouse uploads a part's blobs to S3 and + registers them in `system.remote_data_paths` a moment later; in that window a + live blob is absent from the reference table and looks orphaned, and there is + no per-object re-check before the S3 delete. `--useage 0` silently removed + that protection, the default *was* 0, and `render.py` accepted it — so the + dangerous configuration was also the out-of-the-box one for anyone who did + not set it. + + Why a floor and not a hardcoded constant: the parameter is only dangerous + downward. Upward it is the "be more careful" lever — a cluster with slow + merges or long-running mutations may legitimately want 72 hours or a week. + Removing it would forfeit that and buy nothing the floor does not already + give. + + Refused for `--dry-run` too, deliberately: a preview computed over a wider + set than the delete would honour is worse than no preview, because the + reviewed number is the one the customer approves. + + The floor is enforced in **both** `s3gc.py` and `render.py`. The renderer + catches it before a Job is applied; the tool catches a direct CLI run, which + never passes through the renderer at all. + + **Development escape hatch, scoped to the one non-production phase.** + `PHASE=dev-automation` seeds and deletes its own fixtures within minutes, so + a 24 hour window would make it find nothing and "succeed" vacuously — worse + than failing. That phase, and only that phase, passes + `--dev-allow-short-useage`; the `collect`, `dry-run` and `delete` branches of + the entrypoint never do, and a test asserts it. A run that uses it logs a + warning and writes a `warning` row to the durable run log, so it can never be + mistaken for a normal one. The entrypoint passes the explicit + `--dev-allow-short-useage=true` value required by the boolean parser. + ### Added - **A durable run log in ClickHouse**, `_log`, written diff --git a/README.md b/README.md index 9e426dc..960d736 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ export S3GC_CLUSTERNAME='' export S3GC_EXPECTED_REPLICAS=2 export S3GC_COLLECTTABLEPREFIX='s3gc_example_' export S3GC_AGE=24 -export S3GC_USEAGE=24 +export S3GC_USEAGE=24 # minimum 24; raising is fine, lowering is refused ``` ### S3 authentication modes diff --git a/TODO.md b/TODO.md index 7985fe9..6c786c1 100644 --- a/TODO.md +++ b/TODO.md @@ -13,11 +13,6 @@ forward-looking backlog. fixtures, and remain excluded from CI. - [ ] Require the `Container / test` GitHub Actions check before pull-request merges in the repository branch-protection settings. -- [ ] Decide what `USEAGE_HOURS=0` should do. It disables the age window that - is the only guard against deleting a part between its blob upload and its - registration in `system.remote_data_paths`. Options: reject it in - `render.py`, warn loudly in `s3gc.py`, or leave it and document it. Covered - today only by a test that documents the hazard. - [ ] Quote `--useafter` as a SQL string literal (strict `xfail` in the suite). - [ ] Consider re-checking cluster topology per sample, not once per run, so a replica lost mid-run cannot widen the deletion scope. diff --git a/deploy/kubernetes/README.md b/deploy/kubernetes/README.md index 92aae40..0c5c697 100644 --- a/deploy/kubernetes/README.md +++ b/deploy/kubernetes/README.md @@ -100,6 +100,15 @@ Fill `s3gc.env` from `example.env`. For production, start with `SAMPLES=4`, `USEAGE_HOURS=24`, `ORDER_BY_OBJPATH=false`, and a 12-hour deadline. +> **`USEAGE_HOURS` has a hard floor of 24 and the renderer enforces it.** +> ClickHouse uploads a part's blobs to S3 and registers them in +> `system.remote_data_paths` a moment later. In that window a live blob looks +> orphaned, and nothing re-checks it before the delete — so this window is the +> only thing protecting a part that is still being written. Raise it if a +> cluster has slow merges or long mutations; you cannot lower it. The one +> exception is `PHASE=dev-automation`, which seeds and deletes its own fixtures +> and is already documented as non-production. + Before the first *full* delete against a newly published image or a cluster you have not deleted from before, run one bounded delete with `USETOTAL` set to a few thousand. It exercises the whole path — anti-join, S3 deletion, tombstone diff --git a/deploy/kubernetes/example.env b/deploy/kubernetes/example.env index fddff14..373e95b 100644 --- a/deploy/kubernetes/example.env +++ b/deploy/kubernetes/example.env @@ -35,6 +35,10 @@ S3PROFILE= SAMPLES=4 DELETE_BATCH_SIZE=1000 +# Minimum 24, and the renderer refuses less for every phase except +# dev-automation. This window is the only protection against deleting a part +# between its upload to S3 and its registration in system.remote_data_paths. +# Raising it is fine and sometimes wise; lowering it is not. USEAGE_HOURS=24 # Optional cap on how many collected objects a use phase processes. Leave empty # for a full run. Set it to bound the first delete against a new image or a new diff --git a/deploy/kubernetes/render.py b/deploy/kubernetes/render.py index a4b275b..df8fff4 100644 --- a/deploy/kubernetes/render.py +++ b/deploy/kubernetes/render.py @@ -42,6 +42,9 @@ "VERBOSE", } DELETE_CONFIRMATION = "DELETE_ORPHANS" +# Mirrors MINIMUM_USEAGE_HOURS in s3gc.py. Enforced here as well so a bad value +# fails at render time rather than after a Job has been applied to a cluster. +MINIMUM_USEAGE_HOURS = 24 # Optional keys and their defaults. An empty value renders no environment # variable at all, because s3gc parses S3GC_USETOTAL as an integer and would # reject an empty string. @@ -90,8 +93,19 @@ def validate(values: dict[str, str]) -> None: for numeric_key in ("DELETE_BATCH_SIZE", "EXPECTED_REPLICAS", "SAMPLES", "ACTIVE_DEADLINE_SECONDS", "TTL_SECONDS_AFTER_FINISHED"): if not values[numeric_key].isdigit() or int(values[numeric_key]) < 1: raise ValueError(f"{numeric_key} must be a positive integer") - if not values["USEAGE_HOURS"].isdigit() or int(values["USEAGE_HOURS"]) < 0: + if not values["USEAGE_HOURS"].isdigit(): raise ValueError("USEAGE_HOURS must be a non-negative integer") + # dev-automation seeds and deletes its own fixtures within minutes, and is + # already documented as non-production. Every other phase gets the floor. + if ( + int(values["USEAGE_HOURS"]) < MINIMUM_USEAGE_HOURS + and values["PHASE"] != "dev-automation" + ): + raise ValueError( + f"USEAGE_HOURS must be at least {MINIMUM_USEAGE_HOURS} for PHASE={values['PHASE']}: " + "the age window is the only protection against deleting a part between " + "its upload to S3 and its registration in system.remote_data_paths" + ) if values["S3AUTH"] not in {"static", "aws", "iam"}: raise ValueError("S3AUTH must be static, aws or iam") if values["S3PROFILE"] and values["S3AUTH"] != "aws": diff --git a/docker/kubernetes-entrypoint.sh b/docker/kubernetes-entrypoint.sh index 27e51d7..a082201 100644 --- a/docker/kubernetes-entrypoint.sh +++ b/docker/kubernetes-entrypoint.sh @@ -36,12 +36,16 @@ case "${phase}" in # A fresh collection avoids mixing prior runs and their tombstones into an # automated development run. `set -e` stops subsequent stages on error. + # Development fixtures are seeded and deleted within minutes, so this phase + # -- and ONLY this phase -- may run below the 24 hour age minimum that + # protects a part between its upload to S3 and its registration in + # system.remote_data_paths. The prod phases above never pass this flag. echo "s3gc dev automation: collect" python /app/s3gc.py --collectonly --keepdata --drop-collecttable echo "s3gc dev automation: dry-run" - python /app/s3gc.py --usecollected --dry-run + python /app/s3gc.py --usecollected --dry-run --dev-allow-short-useage=true echo "s3gc dev automation: delete" - exec python /app/s3gc.py --usecollected --keepdata --non-interactive + exec python /app/s3gc.py --usecollected --keepdata --non-interactive --dev-allow-short-useage=true ;; *) echo "Invalid S3GC_PHASE=${phase}; use collect, dry-run, delete, or dev-automation" >&2 diff --git a/s3gc.py b/s3gc.py index abf9ca1..2992476 100644 --- a/s3gc.py +++ b/s3gc.py @@ -335,8 +335,12 @@ def coerce_bool(value): "--useage-hours", dest="useage", type=int, - default=0, - help="Process only already collected objects older than specified number of hours", + default=24, + help=( + "Process only already collected objects older than specified number of " + "hours. Minimum 24: below that a run can delete a part between its blob " + "upload and its registration in system.remote_data_paths" + ), ) parser.add_argument( "--samples", @@ -503,6 +507,17 @@ def coerce_bool(value): help="list all command line options for internal purposes", ) +parser.add_argument( + "--dev-allow-short-useage", + dest="dev_allow_short_useage", + type=coerce_bool, + default=False, + help=( + "development only: permit --useage below the 24 hour minimum. Passed only " + "by the dev-automation entrypoint phase, which seeds and deletes its own " + "fixtures within minutes. Never set this for customer or production work" + ), +) parser.add_argument( "--runlog", "--run-log", @@ -542,6 +557,7 @@ def coerce_bool(value): BOOLEAN_DESTS = ( "s3secure_flag", "runlog_flag", + "dev_allow_short_useage", "use_remove_objects", "keepdata_flag", "collectonly_flag", @@ -678,6 +694,18 @@ def graceful_exit(): ch_writer = None +# ClickHouse uploads a part's blobs to S3 and registers them in +# system.remote_data_paths a moment later. In that window a live blob is absent +# from the reference table and looks orphaned, and there is no per-object +# re-check before the S3 delete — so the age window is the ONLY thing standing +# between a run and live data. +# +# 24 hours is far longer than any part write, and short enough to stay useful. +# It is a floor, not a default to be talked down: a run configured below it is +# refused rather than warned about. +MINIMUM_USEAGE_HOURS = 24 + + class S3DeletionError(RuntimeError): """A delete failed after successful deletions were checkpointed.""" @@ -1186,6 +1214,33 @@ def check_samples_match_partitioning(): def do_use(): + if args.useage < MINIMUM_USEAGE_HOURS: + if not args.dev_allow_short_useage: + # Refused for --dry-run too, so the reviewed preview is exactly the + # set a delete would remove. A dry run that previews a wider set + # than the delete honours is worse than no preview at all. + raise UserVisibleError( + f"--useage {args.useage} is below the {MINIMUM_USEAGE_HOURS} hour minimum. " + "The age window is the only protection against deleting a part between " + "its upload to S3 and its registration in system.remote_data_paths; " + f"use --useage {MINIMUM_USEAGE_HOURS} or greater." + ) + # Reachable only through the dev-automation phase, which seeds and + # deletes its own fixtures. Say so loudly and put it in the durable run + # log, so a run that did this can never be mistaken for a normal one. + logger.warning( + f"--useage {args.useage} is below the {MINIMUM_USEAGE_HOURS} hour minimum and is " + "permitted ONLY because --dev-allow-short-useage is set. This run can " + "delete a part that is still being written. Never use this against " + "customer or production data." + ) + run_log( + "warning", + f"useage {args.useage} below the {MINIMUM_USEAGE_HOURS}h minimum, " + "permitted by --dev-allow-short-useage", + phase="use", + ) + if not args.dryrun_flag: preflight_cluster() diff --git a/tests/test_s3gc.py b/tests/test_s3gc.py index 43bbd86..9729f23 100644 --- a/tests/test_s3gc.py +++ b/tests/test_s3gc.py @@ -90,6 +90,7 @@ def make_args(**overrides): # tests that do not opt in are unaffected by these. "runlog_flag": True, "runid": "test-run", + "dev_allow_short_useage": False, # S3 auth surface (static | aws | iam) "s3auth": "static", "s3profile": "", @@ -304,8 +305,8 @@ def test_dev_automation_entrypoint_runs_collect_dry_run_and_delete(tmp_path): assert result.returncode == 0 assert calls_path.read_text().splitlines() == [ "/app/s3gc.py --collectonly --keepdata --drop-collecttable", - "/app/s3gc.py --usecollected --dry-run", - "/app/s3gc.py --usecollected --keepdata --non-interactive", + "/app/s3gc.py --usecollected --dry-run --dev-allow-short-useage=true", + "/app/s3gc.py --usecollected --keepdata --non-interactive --dev-allow-short-useage=true", ] @@ -315,7 +316,7 @@ def test_dev_automation_entrypoint_stops_after_an_error(tmp_path): fake_python.write_text( "#!/bin/sh\n" "printf '%s\\n' \"$*\" >> \"$CALLS_PATH\"\n" - "case \"$*\" in *--dry-run) exit 42 ;; esac\n" + "case \"$*\" in *--dry-run*) exit 42 ;; esac\n" ) fake_python.chmod(0o755) @@ -338,7 +339,7 @@ def test_dev_automation_entrypoint_stops_after_an_error(tmp_path): assert result.returncode == 42 assert calls_path.read_text().splitlines() == [ "/app/s3gc.py --collectonly --keepdata --drop-collecttable", - "/app/s3gc.py --usecollected --dry-run", + "/app/s3gc.py --usecollected --dry-run --dev-allow-short-useage=true", ] @@ -1061,18 +1062,96 @@ def test_age_guard_excludes_recently_written_objects( assert "s3o.last_modified < now() - interval 24 hour" in sql -def test_useage_zero_disables_the_age_guard(s3gc_module, args_factory, monkeypatch): - """Documents a HAZARD, not a desired behaviour. +@pytest.mark.parametrize("hours", [0, 1, 23]) +def test_useage_below_the_floor_is_refused( + s3gc_module, args_factory, monkeypatch, hours +): + """The age window is a floor, not a default to be talked down. - `if args.useage else ""` means useage=0 emits no age clause, and - render.py accepts USEAGE_HOURS=0 (only negatives are rejected). A run - configured that way has no protection against the mid-write window above. - If s3gc is later changed to refuse or warn on 0, this test should fail and - be updated deliberately. + Below 24 hours a run can delete a part between its upload to S3 and its + registration in remote_data_paths. Refused outright rather than warned + about, and refused before the cluster preflight or any S3 call. """ - sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch, useage=0) + namespace = s3gc_module["do_use"].__globals__ + monkeypatch.setitem(namespace, "args", args_factory(useage=hours)) + monkeypatch.setitem(namespace, "ch_client", FakeCH()) + + with pytest.raises(s3gc_module["UserVisibleError"], match="below the 24 hour minimum"): + s3gc_module["do_use"]() + + +@pytest.mark.parametrize("hours", [0, 1, 23]) +def test_useage_below_the_floor_is_refused_for_dry_run_too( + s3gc_module, args_factory, monkeypatch, hours +): + """A preview wider than the delete would honour is worse than no preview.""" + namespace = s3gc_module["do_use"].__globals__ + monkeypatch.setitem( + namespace, "args", args_factory(useage=hours, dryrun_flag=True) + ) + monkeypatch.setitem(namespace, "ch_client", FakeCH()) + + with pytest.raises(s3gc_module["UserVisibleError"], match="below the 24 hour minimum"): + s3gc_module["do_use"]() + + +def test_useage_at_the_floor_is_accepted(s3gc_module, args_factory, monkeypatch): + sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch, useage=24) + + assert "s3o.last_modified < now() - interval 24 hour" in sql + + +def test_useage_above_the_floor_is_accepted(s3gc_module, args_factory, monkeypatch): + """The parameter stays useful upward: more caution must remain possible.""" + sql = _antijoin_sql(s3gc_module, args_factory, monkeypatch, useage=168) + + assert "s3o.last_modified < now() - interval 168 hour" in sql - assert "interval" not in sql + +def test_default_useage_is_the_safe_floor(monkeypatch): + """A bare run must not opt itself out of the guard.""" + module = _load_with_env(monkeypatch) + + assert module["args"].useage == 24 + assert module["MINIMUM_USEAGE_HOURS"] == 24 + + +def test_dev_flag_permits_a_short_window_but_says_so( + s3gc_module, args_factory, monkeypatch, caplog +): + """Development automation seeds and deletes fixtures within minutes. + + The escape hatch is reachable only through the dev-automation entrypoint + phase, and a run that uses it must be impossible to mistake for a normal + one -- hence the warning and the durable run-log row. + """ + namespace = s3gc_module["do_use"].__globals__ + writer = RecordingCH() + monkeypatch.setitem( + namespace, + "args", + args_factory(useage=0, dryrun_flag=True, dev_allow_short_useage=True), + ) + monkeypatch.setitem(namespace, "ch_client", FakeCH()) + monkeypatch.setitem(namespace, "ch_writer", writer) + monkeypatch.setitem(namespace, "run_log_enabled", True) + monkeypatch.setitem(namespace, "run_id", "dev-run") + monkeypatch.setitem(namespace, "log_tname", "`aux_log`") + + with caplog.at_level("WARNING", logger="s3gc_test"): + s3gc_module["do_use"]() + + assert "dev-allow-short-useage" in caplog.text + events = [ + (row[3], row[4]) + for table, rows, _ in writer.inserts + if table == "`aux_log`" + for row in rows + ] + assert any( + event == "warning" and "below the 24h minimum" in message + for event, message in events + ) def test_dry_run_performs_no_s3_operations(s3gc_module, args_factory, monkeypatch): @@ -1492,3 +1571,71 @@ def test_renderer_uses_the_job_name_as_the_run_id(tmp_path): assert result.returncode == 0 assert "- name: S3GC_RUNID" in result.stdout assert 'value: "s3gc-example-dry-run"' in result.stdout + + +@pytest.mark.parametrize("hours", ["0", "1", "23"]) +def test_renderer_rejects_useage_below_the_floor(tmp_path, hours): + """A bad window must fail at render time, not after a Job is applied.""" + source = (ROOT / "deploy/kubernetes/example.env").read_text() + config_path = tmp_path / "short.env" + config_path.write_text(source.replace("USEAGE_HOURS=24", f"USEAGE_HOURS={hours}")) + + result = subprocess.run( + [sys.executable, str(ROOT / "deploy/kubernetes/render.py"), config_path], + capture_output=True, text=True, check=False, + ) + + assert result.returncode == 64 + assert "USEAGE_HOURS must be at least 24" in result.stderr + + +def test_renderer_allows_a_short_window_for_dev_automation(tmp_path): + """The one non-production phase, already documented as such.""" + source = (ROOT / "deploy/kubernetes/example.env").read_text() + config_path = tmp_path / "dev.env" + config_path.write_text( + source.replace("USEAGE_HOURS=24", "USEAGE_HOURS=0") + .replace("PHASE=dry-run", "PHASE=dev-automation") + .replace("DELETE_CONFIRMATION=", "DELETE_CONFIRMATION=DELETE_ORPHANS") + ) + + result = subprocess.run( + [sys.executable, str(ROOT / "deploy/kubernetes/render.py"), config_path], + capture_output=True, text=True, check=False, + ) + + assert result.returncode == 0, result.stderr + assert 'value: "0"' in result.stdout + + +def test_only_dev_automation_passes_the_short_window_flag(tmp_path): + """The prod phases must never hand s3gc the escape hatch.""" + calls_path = tmp_path / "calls" + fake_python = tmp_path / "python" + fake_python.write_text( + "#!/bin/sh\nprintf '%s\\n' \"$*\" >> \"$CALLS_PATH\"\n" + ) + fake_python.chmod(0o755) + + def run(phase): + calls_path.write_text("") + subprocess.run( + ["sh", str(ROOT / "docker/kubernetes-entrypoint.sh")], + env={ + **os.environ, + "PATH": f"{tmp_path}:{os.environ['PATH']}", + "CALLS_PATH": str(calls_path), + "S3GC_PHASE": phase, + "S3GC_DELETE_CONFIRMATION": "DELETE_ORPHANS", + "S3GC_CLUSTERNAME": "cluster", + "S3GC_EXPECTED_REPLICAS": "2", + }, + capture_output=True, text=True, check=False, + ) + return calls_path.read_text() + + for phase in ("collect", "dry-run", "delete"): + assert "--dev-allow-short-useage" not in run(phase), phase + dev_calls = run("dev-automation") + assert dev_calls.count("--dev-allow-short-useage=true") == 2 + assert "--dev-allow-short-useage\n" not in dev_calls