From 12026d76e6934a5f678c56f33c1b393db79324d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20B=C4=85k?= Date: Thu, 11 Jun 2026 17:31:41 +0200 Subject: [PATCH 01/16] S3 auth via AWS profile / boto3 credential chain Add --s3auth=aws and --s3profile so s3gc can resolve temporary session-token credentials (e.g. from AWS SSO) instead of static keys; --s3-session-token for explicit temporary credentials --- README.md | 39 ++++++++++++ requirements.txt | 1 + s3gc.py | 163 +++++++++++++++++++++++++++++++++++++++++------ 3 files changed, 184 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 4ed0c77..b1cabd7 100644 --- a/README.md +++ b/README.md @@ -25,10 +25,22 @@ It is important to use `--s3diskname` if your disk name is not `s3` which is by WARNING!: Please use `--dry-run` to check and compare results of what is going to be deleted, just to be on the safe side. ## script invocation +### install +Install the Python dependencies, ideally in a virtualenv: +``` +python3 -m venv .venv +source .venv/bin/activate +pip install -r requirements.txt +``` +On Debian/Ubuntu/WSL you may first need `sudo apt install -y python3-pip python3-venv`. +Without a virtualenv, use `pip install --user -r requirements.txt` (add +`--break-system-packages` if pip refuses on an externally-managed Python). + ### help ``` python3 s3gc.py --help ``` + ### typical usage #### all together with dry-run for https://altinity-clickhouse-data-demo20565656565620663600000001.s3.amazonaws.com/github @@ -43,6 +55,30 @@ S3GC_S3PATH=github/ \ S3GC_S3SECURE_FLAG=true \ python3 ./s3gc.py --verbose --dry-run ``` +#### AWS SSO or AWS profile credentials +Authenticate with AWS CLI first, then let `s3gc` resolve temporary credentials through the boto3 credential chain. +``` +aws sso login --profile my-sso-profile + +S3GC_S3AUTH=aws \ +S3GC_S3PROFILE=my-sso-profile \ +S3GC_S3IP=s3.amazonaws.com \ +S3GC_S3PORT=443 \ +S3GC_S3REGION=us-east-1 \ +S3GC_S3BUCKET=altinity-clickhouse-data-demo20565656565620663600000001 \ +S3GC_S3PATH=github/ \ +S3GC_S3SECURE_FLAG=true \ +python3 ./s3gc.py --verbose --dry-run +``` + +`S3GC_S3ACCESSKEY` and `S3GC_S3SECRETKEY` are not used with `S3GC_S3AUTH=aws`. +The selected credentials must allow `s3:ListBucket` on the bucket for +`S3GC_S3PATH`, even for `--dry-run`. Verify the same profile with: +``` +aws sts get-caller-identity --profile my-sso-profile +aws s3api list-objects-v2 --bucket altinity-clickhouse-data-demo20565656565620663600000001 --prefix github/ --max-keys 1 --profile my-sso-profile +``` + #### GCS and object storage that do not support batch delete operations ``` S3GC_S3ACCESSKEY=GOOG1xxxxxxxxx \ @@ -94,6 +130,9 @@ sudo docker run --network="host" -e S3GC_S3PORT=19000 -e S3GC_S3ACCESSKEY=minio9 ### v_0.2 Fri Jan 31 2025 - added option to avoid batch deletion for services like GCS +### v_0.3 Mon Jun 15 2026 +- added s3 profile option + ## to do list ~~1. option to avoid `remove_objects` which is reportedly not supported by GCE~~ diff --git a/requirements.txt b/requirements.txt index 839b8f5..81a157d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,4 @@ Minio +boto3 clickhouse_connect jsonargparse[all] diff --git a/s3gc.py b/s3gc.py index f668589..9ea461b 100644 --- a/s3gc.py +++ b/s3gc.py @@ -19,6 +19,7 @@ from io import StringIO from minio import Minio from minio.deleteobjects import DeleteObject +from minio.error import S3Error from contextlib import redirect_stdout import clickhouse_connect @@ -31,7 +32,6 @@ import urllib3 import logging import datetime -from distutils.util import strtobool usage = """ s3 garbage collector for ClickHouse @@ -102,6 +102,27 @@ default="", help="S3 secret key", ) +parser.add_argument( + "--s3-session-token", + "--s3sessiontoken", + dest="s3sessiontoken", + default="", + help="S3 session token for explicit temporary credentials", +) +parser.add_argument( + "--s3auth", + "--s3-auth", + dest="s3auth", + default="static", + help="S3 auth mode: static or aws. aws uses the boto3 credential chain, including AWS SSO profiles", +) +parser.add_argument( + "--s3profile", + "--s3-profile", + dest="s3profile", + default="", + help="AWS profile name for S3 auth. Setting this enables aws auth mode", +) parser.add_argument( "--s3secure", "--s3-secure", @@ -446,10 +467,14 @@ class LogFormatter(logging.Formatter): def get_filter_strings(): filter_strings = [] - if len(args.chpass) > 3: - filter_strings.append(args.chpass) - if len(args.s3secretkey) > 3: - filter_strings.append(args.s3secretkey) + for secret in [ + args.chpass, + args.s3accesskey, + args.s3secretkey, + args.s3sessiontoken, + ]: + if len(secret) > 3: + filter_strings.append(secret) return filter_strings filter_strings = get_filter_strings() @@ -500,6 +525,19 @@ def graceful_exit(): ch_client = None +class UserVisibleError(RuntimeError): + pass + + +def strtobool(value): + value = value.lower() + if value in ["y", "yes", "t", "true", "on", "1"]: + return True + if value in ["n", "no", "f", "false", "off", "0"]: + return False + raise ValueError(f"invalid truth value {value}") + + def connect_to_ch(): logger.info( f"Connecting to ClickHouse, host={args.chhost}, port={args.chport}, username={args.chuser}, password={args.chpass}, s3path={args.s3path}, bucket={args.s3bucket}, s3path={args.s3path}" @@ -514,25 +552,105 @@ def connect_to_ch(): ) +def resolve_static_s3_credentials(): + if bool(args.s3accesskey) != bool(args.s3secretkey): + raise ValueError("s3accesskey and s3secretkey must be specified together") + if args.s3sessiontoken and not args.s3accesskey: + raise ValueError("s3sessiontoken requires s3accesskey and s3secretkey") + + if args.s3accesskey: + return args.s3accesskey, args.s3secretkey, args.s3sessiontoken or None, args.s3region, "static" + + return None, None, None, args.s3region, "anonymous" + + +def resolve_aws_s3_credentials(): + if args.s3accesskey or args.s3secretkey or args.s3sessiontoken: + raise ValueError("s3auth=aws cannot be combined with explicit S3 access keys") + + try: + import boto3 + except ImportError as exc: + raise UserVisibleError("boto3 is required for s3auth=aws") from exc + + session = boto3.Session( + profile_name=args.s3profile or None, + region_name=args.s3region, + ) + credentials = session.get_credentials() + if credentials is None: + profile_hint = f" profile {args.s3profile}" if args.s3profile else "" + raise UserVisibleError(f"unable to resolve AWS credentials{profile_hint}") + + frozen_credentials = credentials.get_frozen_credentials() + if not frozen_credentials.access_key or not frozen_credentials.secret_key: + profile_hint = f" profile {args.s3profile}" if args.s3profile else "" + raise UserVisibleError(f"resolved AWS credentials{profile_hint} are incomplete") + + return ( + frozen_credentials.access_key, + frozen_credentials.secret_key, + frozen_credentials.token, + args.s3region or session.region_name, + "aws", + ) + + +def resolve_s3_credentials(): + auth_mode = args.s3auth.lower() + if auth_mode not in ["static", "aws"]: + raise ValueError("s3auth must be static or aws") + if args.s3profile: + auth_mode = "aws" + + if auth_mode == "aws": + return resolve_aws_s3_credentials() + + return resolve_static_s3_credentials() + + def connect_to_s3(): if args.s3secure_flag: logger.debug(f"using SSL certificate {args.s3sslcertfile}") os.environ["SSL_CERT_FILE"] = args.s3sslcertfile + access_key, secret_key, session_token, s3_region, s3_auth = resolve_s3_credentials() logger.info( - f"Connecting to S3, host:port={args.s3ip}:{args.s3port}, access_key={args.s3accesskey}, secret_key={args.s3secretkey}, secure={args.s3secure_flag}, region={args.s3region}" + f"Connecting to S3, host:port={args.s3ip}:{args.s3port}, auth={s3_auth}, secure={args.s3secure_flag}, region={s3_region}" ) global minio_client minio_client = Minio( f"{args.s3ip}:{args.s3port}", - access_key=args.s3accesskey, - secret_key=args.s3secretkey, + access_key=access_key, + secret_key=secret_key, + session_token=session_token, secure=args.s3secure_flag, - region=args.s3region, + region=s3_region, http_client=urllib3.PoolManager(cert_reqs="CERT_NONE"), ) +def format_s3_list_error(exc): + code = getattr(exc, "code", "unknown") + message = getattr(exc, "message", str(exc)) + profile_arg = f" --profile {args.s3profile}" if args.s3profile else "" + return ( + f"unable to list S3 objects for bucket={args.s3bucket!r}, prefix={args.s3path!r}: " + f"{code}: {message}. " + f"s3gc collection requires s3:ListBucket on arn:aws:s3:::{args.s3bucket} " + f"for this prefix, even with --dry-run. Verify the same credentials with: " + f"aws sts get-caller-identity{profile_arg}; " + f"aws s3api list-objects-v2 --bucket {args.s3bucket} --prefix {args.s3path} --max-keys 1{profile_arg}" + ) + + +def next_s3_object(objects): + try: + return next(objects) + except S3Error as exc: + raise UserVisibleError(format_s3_list_error(exc)) from exc + + def do_collect(): logger.debug(f"start_after {args.collectafter}") objects = minio_client.list_objects( @@ -568,7 +686,7 @@ def do_collect(): objs = [] for batch_element in range(0, args.collectbatchsize): try: - obj = next(objects) + obj = next_s3_object(objects) delta = datetime.datetime.now(datetime.timezone.utc) - obj.last_modified hours = int(delta.seconds / 3600) if hours >= args.age: @@ -713,15 +831,22 @@ def make_antijoin(calc_only=False, sample=None): def main(): - connect_to_ch() - if not (args.usecollected_flag and args.dryrun_flag): - connect_to_s3() - if not args.usecollected_flag: - do_collect() - if not args.collectonly_flag: - do_use() - - graceful_exit() + try: + connect_to_ch() + if not (args.usecollected_flag and args.dryrun_flag): + connect_to_s3() + if not args.usecollected_flag: + do_collect() + if not args.collectonly_flag: + do_use() + + graceful_exit() + except UserVisibleError as exc: + if args.debug_flag: + logger.exception(str(exc)) + else: + logger.error(str(exc)) + sys.exit(1) if __name__ == "__main__": From cfee0cf6591b5b0e0ab25b279fc3b16b6071a1cd Mon Sep 17 00:00:00 2001 From: "Diego Nieto (lesandie)" Date: Tue, 4 Aug 2026 13:19:09 +0200 Subject: [PATCH 02/16] WIP: kubernetes deployment --- .dockerignore | 7 ++ .github/workflows/container.yml | 40 ++++++++ .gitignore | 3 + Dockerfile | 78 +++++++------- Dockerfile.in | 6 +- Makefile | 6 +- README.md | 20 +++- deploy/kubernetes/README.md | 67 ++++++++++++ deploy/kubernetes/example.env | 34 ++++++ deploy/kubernetes/job.yaml.tmpl | 92 +++++++++++++++++ deploy/kubernetes/render.py | 104 +++++++++++++++++++ kubernetes-entrypoint.sh | 33 ++++++ requirements.txt | 6 +- s3gc.py | 174 +++++++++++++++++++++++++------ tests/test_s3gc.py | 176 ++++++++++++++++++++++++++++++++ 15 files changed, 769 insertions(+), 77 deletions(-) create mode 100644 .dockerignore create mode 100644 .github/workflows/container.yml create mode 100644 .gitignore create mode 100644 deploy/kubernetes/README.md create mode 100644 deploy/kubernetes/example.env create mode 100644 deploy/kubernetes/job.yaml.tmpl create mode 100644 deploy/kubernetes/render.py create mode 100644 kubernetes-entrypoint.sh create mode 100644 tests/test_s3gc.py diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..201997f --- /dev/null +++ b/.dockerignore @@ -0,0 +1,7 @@ +.git +.github +.venv +__pycache__ +tests +deploy +*.pyc diff --git a/.github/workflows/container.yml b/.github/workflows/container.yml new file mode 100644 index 0000000..fed3b38 --- /dev/null +++ b/.github/workflows/container.yml @@ -0,0 +1,40 @@ +name: Container + +on: + pull_request: + push: + branches: [master] + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: "3.11" + - run: python -m pip install --disable-pip-version-check -r requirements.txt + - run: python -m unittest discover -s tests -v + - run: python deploy/kubernetes/render.py deploy/kubernetes/example.env > /tmp/s3gc-job.yaml + + build: + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@v4 + - uses: docker/setup-qemu-action@v3 + - uses: docker/setup-buildx-action@v3 + - uses: docker/login-action@v3 + if: github.event_name == 'push' + with: + username: ${{ secrets.DOCKERHUB_USERNAME }} + password: ${{ secrets.DOCKERHUB_TOKEN }} + - uses: docker/build-push-action@v6 + with: + context: . + platforms: linux/amd64,linux/arm64 + push: ${{ github.event_name == 'push' }} + tags: | + altinity/s3gc:sha-${{ github.sha }} + altinity/s3gc:latest diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..f8b8d8b --- /dev/null +++ b/.gitignore @@ -0,0 +1,3 @@ +__pycache__/ +*.pyc +options.lst diff --git a/Dockerfile b/Dockerfile index a52e231..735e73e 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,46 +1,52 @@ -FROM python:3 +FROM python:3.11-slim WORKDIR /usr/src/app COPY requirements.txt ./ -RUN pip install --no-cache-dir -r requirements.txt +RUN pip install --no-cache-dir --disable-pip-version-check -r requirements.txt COPY . . -ENV S3GC_CHHOST=localhost \ - S3GC_CHPORT=8123 \ - S3GC_CHUSER=default \ - S3GC_CHPASS='' \ - S3GC_S3IP=127.0.0.1 \ - S3GC_S3PORT=9001 \ - S3GC_S3BUCKET=root \ - S3GC_S3PATH=data/ \ - S3GC_S3ACCESSKEY='' \ - S3GC_S3SECRETKEY='' \ - S3GC_S3SECURE_FLAG=false \ - S3GC_S3SSLCERTFILE='' \ - S3GC_S3REGION=null \ - S3GC_S3DISKNAME=s3 \ - S3GC_KEEPDATA_FLAG=false \ - S3GC_COLLECTONLY_FLAG=false \ - S3GC_USECOLLECTED_FLAG=false \ - S3GC_COLLECTTABLEPREFIX=s3objects_for_ \ - S3GC_COLLECTBATCHSIZE=1024 \ - S3GC_TOTAL=null \ - S3GC_COLLECTAFTER=null \ - S3GC_USEAFTER=null \ - S3GC_USETOTAL=null \ - S3GC_DRYRUN_FLAG=false \ - S3GC_CLUSTERNAME='' \ - S3GC_AGE=0 \ - S3GC_USEAGE=0 \ - S3GC_SAMPLES=4 \ - S3GC_CHTIMEOUT=1800 \ - S3GC_CREATEDATABASE_FLAG=false \ - S3GC_DROP_COLLECTTABLE_FLAG=false \ - S3GC_INTERACTIVE_FLAG=true \ - S3GC_VERBOSE_FLAG=false \ - S3GC_DEBUG_FLAG=false \ +RUN chmod 0555 /usr/src/app/kubernetes-entrypoint.sh + +ENV S3GC_CHHOST=localhost \ + S3GC_CHPORT=8123 \ + S3GC_CHUSER=default \ + S3GC_CHPASS='' \ + S3GC_S3IP=127.0.0.1 \ + S3GC_S3PORT=9001 \ + S3GC_S3BUCKET=root \ + S3GC_S3PATH=data/ \ + S3GC_S3ACCESSKEY='' \ + S3GC_S3SECRETKEY='' \ + S3GC_S3SECURE_FLAG=false \ + S3GC_S3SSLCERTFILE='' \ + S3GC_S3REGION=null \ + S3GC_S3DISKNAME=s3 \ + S3GC_S3USEIAM=false \ + S3GC_KEEPDATA_FLAG=false \ + S3GC_COLLECTONLY_FLAG=false \ + S3GC_USECOLLECTED_FLAG=false \ + S3GC_COLLECTTABLEPREFIX=s3objects_for_ \ + S3GC_COLLECTBATCHSIZE=1024 \ + S3GC_TOTAL=null \ + S3GC_COLLECTAFTER=null \ + S3GC_USEAFTER=null \ + S3GC_USETOTAL=null \ + S3GC_DRYRUN_FLAG=false \ + S3GC_CLUSTERNAME='' \ + S3GC_EXPECTED_REPLICAS=null \ + S3GC_AGE=0 \ + S3GC_USEAGE=0 \ + S3GC_SAMPLES=4 \ + S3GC_DELETEBATCHSIZE=1000 \ + S3GC_CHTIMEOUT=1800 \ + S3GC_CREATEDATABASE_FLAG=false \ + S3GC_DROP_COLLECTTABLE_FLAG=false \ + S3GC_USE_REMOVE_OBJECTS=true \ + S3GC_INTERACTIVE_FLAG=true \ + S3GC_VERBOSE_FLAG=false \ + S3GC_DEBUG_FLAG=false \ S3GC_SILENT_FLAG=false diff --git a/Dockerfile.in b/Dockerfile.in index a30fde4..5bae422 100644 --- a/Dockerfile.in +++ b/Dockerfile.in @@ -1,12 +1,14 @@ -FROM python:3 +FROM python:3.11-slim WORKDIR /usr/src/app COPY requirements.txt ./ -RUN pip install --no-cache-dir -r requirements.txt +RUN pip install --no-cache-dir --disable-pip-version-check -r requirements.txt COPY . . +RUN chmod 0555 /usr/src/app/kubernetes-entrypoint.sh + # @@ ENTRYPOINT ["python", "./s3gc.py"] diff --git a/Makefile b/Makefile index 6e320df..f9d33fe 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,7 @@ +PYTHON ?= python3 + Dockerfile: Dockerfile.in options.lst - python3 -c "import sys; sys.stdout.write(sys.stdin.read().replace('# @@', open('./options.lst', 'r').read()))" < Dockerfile.in > Dockerfile + $(PYTHON) -c "import sys; sys.stdout.write(sys.stdin.read().replace('# @@', open('./options.lst', 'r').read()))" < Dockerfile.in > Dockerfile options.lst: ./s3gc.py - python3 ./s3gc.py --listoptions > options.lst + $(PYTHON) ./s3gc.py --listoptions > options.lst diff --git a/README.md b/README.md index 4ed0c77..56d5c48 100644 --- a/README.md +++ b/README.md @@ -72,18 +72,32 @@ S3GC_S3PORT=19000 S3GC_S3ACCESSKEY=minio99 S3GC_S3SECRETKEY=minio123 S3GC_USEC ## docker There is a docker image for the script. +The published image is pinned to Python 3.11 for reproducibility. + +When regenerating the Dockerfile defaults, run `make` with an interpreter that +has the pinned requirements installed, for example +`make PYTHON=.venv/bin/python`. + ### rebuild ``` make -sudo docker buildx build --platform linux/arm/v7,linux/arm64/v8,linux/amd64 -t ilejn/s3gc . +sudo docker buildx build --platform linux/arm/v7,linux/arm64/v8,linux/amd64 -t altinity/s3gc . ``` ### usage ``` -sudo docker run ilejn/s3gc --help -sudo docker run --network="host" -e S3GC_S3PORT=19000 -e S3GC_S3ACCESSKEY=minio99 -e S3GC_S3SECRETKEY=minio123 ilejn/s3gc +sudo docker run altinity/s3gc --help +sudo docker run --network="host" -e S3GC_S3PORT=19000 -e S3GC_S3ACCESSKEY=minio99 -e S3GC_S3SECRETKEY=minio123 altinity/s3gc ``` +## Kubernetes + +`deploy/kubernetes/` contains a plain-template one-shot Job runner for running +the collector inside the ClickHouse namespace. It has separate `collect`, +`dry-run`, and guarded `delete` phases and does not create or contain secrets. +See [deploy/kubernetes/README.md](deploy/kubernetes/README.md) for the render +contract and safety requirements. + ## changelog ### v_0.1 Wed Jun 12 2024 diff --git a/deploy/kubernetes/README.md b/deploy/kubernetes/README.md new file mode 100644 index 0000000..ba6d013 --- /dev/null +++ b/deploy/kubernetes/README.md @@ -0,0 +1,67 @@ +# Kubernetes Job runner + +The runner executes `s3gc` in the same namespace as ClickHouse so it reaches the +cluster service directly and does not depend on a `kubectl port-forward`. + +## Safety model + +- Render and apply exactly one phase at a time: `collect`, `dry-run`, or `delete`. +- `delete` requires `DELETE_CONFIRMATION=DELETE_ORPHANS`, a cluster name, and an + expected replica count. The program validates the local macro and the number of + reachable replicas before it deletes anything. +- Jobs never retry automatically. Successful S3 deletions are tombstoned in the + auxiliary table after every `DELETE_BATCH_SIZE` objects, with an accumulated + progress line in the Job log, so a manually rerun Job resumes safely. +- The Job does not receive a Kubernetes API token and receives no in-pod RBAC. + +## Credentials + +Create or reference a namespaced Secret outside this repository. For static S3 +credentials it must contain these four keys, which are passed unchanged to +`s3gc`: + +``` +S3GC_CHUSER +S3GC_CHPASS +S3GC_S3ACCESSKEY +S3GC_S3SECRETKEY +``` + +Do not put secret values in the config file or rendered manifest. The secret +provisioning mechanism is intentionally environment-owned. + +For EKS/IRSA or another AWS workload-identity setup, set `S3USEIAM=true` and +set `SERVICE_ACCOUNT` to the identity-enabled ServiceAccount. In that mode the +Secret only needs `S3GC_CHUSER` and `S3GC_CHPASS`; static S3 keys are not used. + +## Render and validate + +Copy `example.env` outside the repository and fill the target values. Use an +immutable image digest after publishing the image: + +```bash +python3 deploy/kubernetes/render.py /secure/path/s3gc.env > /tmp/s3gc-job.yaml +kubectl apply --dry-run=client -f /tmp/s3gc-job.yaml +``` + +`IMAGE_PULL_SECRET` is a namespaced `kubernetes.io/dockerconfigjson` Secret for +the registry containing the pinned image. It is separate from the runtime +credentials Secret and must be created by the environment owner. + +Apply only after a separate explicit approval for the target environment: + +```bash +kubectl apply -f /tmp/s3gc-job.yaml +kubectl logs -f job/ +``` + +For a fresh run, use `PHASE=collect` and a unique `COLLECTTABLEPREFIX`. The +collect Job retains the table. Render `PHASE=dry-run` next, inspect its result, +then render `PHASE=delete` plus the required delete confirmation only when +approved. Never reuse an auxiliary table for a different bucket/prefix. + +Set `VERBOSE=true` for the Job logs to include its connection, collection, and +anti-join totals. + +`DELETE_BATCH_SIZE` controls delete progress granularity (use `1000` normally; +use a smaller value only when more frequent progress checkpoints are useful). diff --git a/deploy/kubernetes/example.env b/deploy/kubernetes/example.env new file mode 100644 index 0000000..bf0d269 --- /dev/null +++ b/deploy/kubernetes/example.env @@ -0,0 +1,34 @@ +# Copy this file outside the repository, fill non-secret target values, and render it. +# Credentials must be supplied separately by the named Kubernetes Secret. +JOB_NAME=s3gc-example-dry-run +NAMESPACE=clickhouse +IMAGE=altinity/s3gc@sha256:0000000000000000000000000000000000000000000000000000000000000000 +IMAGE_PULL_SECRET=altinity-dockerhub-pull +PHASE=dry-run +DELETE_CONFIRMATION= +CREDENTIALS_SECRET=s3gc-credentials +SERVICE_ACCOUNT=s3gc + +CHHOST=clickhouse.example.svc.cluster.local +CHPORT=8123 +CLUSTERNAME=example-cluster +EXPECTED_REPLICAS=2 +COLLECTTABLEPREFIX=s3gc_example_ + +S3IP=s3.example.com +S3PORT=443 +S3BUCKET=example-bucket +S3PATH=clickhouse-data/ +S3REGION=us-east-1 +S3SECURE_FLAG=true +S3DISKNAME=s3 +S3USEIAM=false + +SAMPLES=4 +DELETE_BATCH_SIZE=1000 +USEAGE_HOURS=24 +ACTIVE_DEADLINE_SECONDS=14400 +TTL_SECONDS_AFTER_FINISHED=604800 +MEMORY_REQUEST=1Gi +MEMORY_LIMIT=4Gi +VERBOSE=true diff --git a/deploy/kubernetes/job.yaml.tmpl b/deploy/kubernetes/job.yaml.tmpl new file mode 100644 index 0000000..073d4e3 --- /dev/null +++ b/deploy/kubernetes/job.yaml.tmpl @@ -0,0 +1,92 @@ +apiVersion: batch/v1 +kind: Job +metadata: + name: "${JOB_NAME}" + namespace: "${NAMESPACE}" + labels: + app.kubernetes.io/name: s3gc + app.kubernetes.io/component: object-storage-garbage-collection + s3gc.altinity.com/phase: "${PHASE}" +spec: + backoffLimit: 0 + activeDeadlineSeconds: ${ACTIVE_DEADLINE_SECONDS} + ttlSecondsAfterFinished: ${TTL_SECONDS_AFTER_FINISHED} + template: + metadata: + labels: + app.kubernetes.io/name: s3gc + app.kubernetes.io/component: object-storage-garbage-collection + s3gc.altinity.com/phase: "${PHASE}" + spec: + restartPolicy: Never + serviceAccountName: "${SERVICE_ACCOUNT}" + imagePullSecrets: + - name: "${IMAGE_PULL_SECRET}" + automountServiceAccountToken: false + securityContext: + runAsNonRoot: true + runAsUser: 65532 + runAsGroup: 65532 + seccompProfile: + type: RuntimeDefault + containers: + - name: s3gc + image: "${IMAGE}" + imagePullPolicy: IfNotPresent + command: ["/usr/src/app/kubernetes-entrypoint.sh"] + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: ["ALL"] + resources: + requests: + cpu: "500m" + memory: ${MEMORY_REQUEST} + limits: + cpu: "2" + memory: ${MEMORY_LIMIT} + envFrom: + - secretRef: + name: "${CREDENTIALS_SECRET}" + env: + - name: S3GC_PHASE + value: "${PHASE}" + - name: S3GC_DELETE_CONFIRMATION + value: "${DELETE_CONFIRMATION}" + - name: S3GC_CHHOST + value: "${CHHOST}" + - name: S3GC_CHPORT + value: "${CHPORT}" + - name: S3GC_CLUSTERNAME + value: "${CLUSTERNAME}" + - name: S3GC_EXPECTED_REPLICAS + value: "${EXPECTED_REPLICAS}" + - name: S3GC_COLLECTTABLEPREFIX + value: "${COLLECTTABLEPREFIX}" + - name: S3GC_S3IP + value: "${S3IP}" + - name: S3GC_S3PORT + value: "${S3PORT}" + - name: S3GC_S3BUCKET + value: "${S3BUCKET}" + - name: S3GC_S3PATH + value: "${S3PATH}" + - name: S3GC_S3REGION + value: "${S3REGION}" + - name: S3GC_S3SECURE_FLAG + value: "${S3SECURE_FLAG}" + - name: S3GC_S3DISKNAME + value: "${S3DISKNAME}" + - name: S3GC_S3USEIAM + value: "${S3USEIAM}" + - name: S3GC_SAMPLES + value: "${SAMPLES}" + - name: S3GC_DELETEBATCHSIZE + value: "${DELETE_BATCH_SIZE}" + - name: S3GC_USEAGE + value: "${USEAGE_HOURS}" + - name: S3GC_CHTIMEOUT + value: "${ACTIVE_DEADLINE_SECONDS}" + - name: S3GC_VERBOSE_FLAG + value: "${VERBOSE}" diff --git a/deploy/kubernetes/render.py b/deploy/kubernetes/render.py new file mode 100644 index 0000000..3da7267 --- /dev/null +++ b/deploy/kubernetes/render.py @@ -0,0 +1,104 @@ +#!/usr/bin/env python3 +"""Render the plain s3gc Job template from a non-secret KEY=VALUE file.""" + +import re +import sys +from pathlib import Path +from string import Template + + +ROOT = Path(__file__).parent +TEMPLATE = ROOT / "job.yaml.tmpl" +REQUIRED = { + "ACTIVE_DEADLINE_SECONDS", + "CHHOST", + "CHPORT", + "CLUSTERNAME", + "COLLECTTABLEPREFIX", + "CREDENTIALS_SECRET", + "DELETE_BATCH_SIZE", + "EXPECTED_REPLICAS", + "IMAGE", + "IMAGE_PULL_SECRET", + "JOB_NAME", + "MEMORY_LIMIT", + "MEMORY_REQUEST", + "NAMESPACE", + "PHASE", + "S3BUCKET", + "S3DISKNAME", + "S3IP", + "S3PATH", + "S3PORT", + "S3REGION", + "S3SECURE_FLAG", + "S3USEIAM", + "SAMPLES", + "SERVICE_ACCOUNT", + "TTL_SECONDS_AFTER_FINISHED", + "USEAGE_HOURS", + "VERBOSE", +} +DELETE_CONFIRMATION = "DELETE_ORPHANS" +JOB_NAME_RE = re.compile(r"^[a-z0-9]([-a-z0-9]*[a-z0-9])?$") + + +def read_values(path: Path) -> dict[str, str]: + values: dict[str, str] = {} + for line_number, line in enumerate(path.read_text().splitlines(), start=1): + line = line.strip() + if not line or line.startswith("#"): + continue + if "=" not in line: + raise ValueError(f"{path}:{line_number}: expected KEY=VALUE") + key, value = line.split("=", 1) + if not key.isidentifier(): + raise ValueError(f"{path}:{line_number}: invalid key {key!r}") + values[key] = value + return values + + +def validate(values: dict[str, str]) -> None: + missing = sorted(REQUIRED - values.keys()) + if missing: + raise ValueError("missing required values: " + ", ".join(missing)) + if values["PHASE"] not in {"collect", "dry-run", "delete"}: + raise ValueError("PHASE must be collect, dry-run, or delete") + if values["PHASE"] == "delete" and values.get("DELETE_CONFIRMATION") != DELETE_CONFIRMATION: + raise ValueError( + f"delete requires DELETE_CONFIRMATION={DELETE_CONFIRMATION}" + ) + if not JOB_NAME_RE.fullmatch(values["JOB_NAME"]) or len(values["JOB_NAME"]) > 63: + raise ValueError("JOB_NAME must be a DNS label of at most 63 characters") + if "@sha256:" not in values["IMAGE"]: + raise ValueError("IMAGE must be pinned by digest (for example, altinity/s3gc@sha256:...)") + for key, value in values.items(): + if any(character in value for character in ('"', "\\n", "\\r")): + raise ValueError(f"{key} may not contain quotes or newlines") + 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: + raise ValueError("USEAGE_HOURS must be a non-negative integer") + if values["S3USEIAM"] not in {"true", "false"}: + raise ValueError("S3USEIAM must be true or false") + if values["VERBOSE"] not in {"true", "false"}: + raise ValueError("VERBOSE must be true or false") + + +def main() -> int: + if len(sys.argv) != 2: + print(f"usage: {Path(sys.argv[0]).name} CONFIG.env", file=sys.stderr) + return 64 + try: + values = read_values(Path(sys.argv[1])) + validate(values) + sys.stdout.write(Template(TEMPLATE.read_text()).substitute(values)) + except (OSError, ValueError, KeyError) as exc: + print(f"render error: {exc}", file=sys.stderr) + return 64 + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/kubernetes-entrypoint.sh b/kubernetes-entrypoint.sh new file mode 100644 index 0000000..be82e74 --- /dev/null +++ b/kubernetes-entrypoint.sh @@ -0,0 +1,33 @@ +#!/usr/bin/env sh +set -eu + +phase="${S3GC_PHASE:-dry-run}" + +case "${phase}" in + collect) + set -- --collectonly --keepdata + if [ "${S3GC_FRESH_RUN:-false}" = "true" ]; then + set -- "$@" --drop-collecttable + fi + ;; + dry-run) + set -- --usecollected --dry-run + ;; + delete) + if [ "${S3GC_DELETE_CONFIRMATION:-}" != "DELETE_ORPHANS" ]; then + echo "Refusing delete: set S3GC_DELETE_CONFIRMATION=DELETE_ORPHANS" >&2 + exit 64 + fi + if [ -z "${S3GC_CLUSTERNAME:-}" ] || [ -z "${S3GC_EXPECTED_REPLICAS:-}" ]; then + echo "Refusing delete: S3GC_CLUSTERNAME and S3GC_EXPECTED_REPLICAS are required" >&2 + exit 64 + fi + set -- --usecollected --keepdata --non-interactive + ;; + *) + echo "Invalid S3GC_PHASE=${phase}; use collect, dry-run, or delete" >&2 + exit 64 + ;; +esac + +exec python ./s3gc.py "$@" diff --git a/requirements.txt b/requirements.txt index 839b8f5..8db080d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,3 @@ -Minio -clickhouse_connect -jsonargparse[all] +minio==7.2.20 +clickhouse-connect==1.6.0 +jsonargparse==4.36.0 diff --git a/s3gc.py b/s3gc.py index f668589..0c38a02 100644 --- a/s3gc.py +++ b/s3gc.py @@ -19,6 +19,7 @@ from io import StringIO from minio import Minio from minio.deleteobjects import DeleteObject +from minio.credentials import IamAwsProvider from contextlib import redirect_stdout import clickhouse_connect @@ -31,13 +32,22 @@ import urllib3 import logging import datetime -from distutils.util import strtobool usage = """ s3 garbage collector for ClickHouse example: $ ./s3gc.py """ + +def strtobool(value): + """Minimal stdlib-compatible replacement for distutils.util.strtobool.""" + normalized = value.lower() + if normalized in {"y", "yes", "t", "true", "on", "1"}: + return 1 + if normalized in {"n", "no", "f", "false", "off", "0"}: + return 0 + raise ValueError(f"invalid truth value {value!r}") + parser = ArgumentParser( usage=usage, env_prefix="S3GC", default_env=True, exit_on_error=False ) @@ -139,6 +149,14 @@ default="s3", help="S3 disk name", ) +parser.add_argument( + "--s3useiam", + "--s3-use-iam", + action="store_true", + dest="s3useiam", + default=False, + help="Use the AWS SDK-compatible workload identity credential chain instead of static S3 keys", +) parser.add_argument( "--keepdata", "--keep-data", @@ -256,6 +274,12 @@ default="", help="Consider an objects unused if there is no host in the cluster refers the object", ) +parser.add_argument( + "--expected-replicas", + dest="expected_replicas", + type=Optional[int], + help="Fail before deleting when clusterAllReplicas() does not return this many replicas", +) parser.add_argument( "--age", "--hours", @@ -284,6 +308,14 @@ default=4, help="Number of partitions in auxiliary table", ) +parser.add_argument( + "--deletebatchsize", + "--delete-batch-size", + dest="deletebatchsize", + type=int, + default=1000, + help="S3 objects to delete and checkpoint per progress batch", +) parser.add_argument( "--chtimeout", "--ch-timeout", @@ -422,7 +454,7 @@ if key in ["listoptions"]: continue if backslash: - print(" \\ ") + print(" \\") print(f" S3GC_{key.upper()}={value}", end="") backslash = True @@ -500,6 +532,45 @@ def graceful_exit(): ch_client = None +class S3DeletionError(RuntimeError): + """A delete failed after successful deletions were checkpointed.""" + + +def _query_single_value(query): + result = ch_client.query(query) + if not result.result_rows or not result.result_rows[0]: + raise RuntimeError(f"ClickHouse returned no result for preflight query: {query}") + return result.result_rows[0][0] + + +def preflight_cluster(): + """Make destructive cluster-wide cleanup fail closed when topology is unexpected.""" + if not args.expected_replicas: + return + if not args.clustername: + raise ValueError("--expected-replicas requires --cluster") + + actual_cluster = _query_single_value("SELECT getMacro('cluster')") + if actual_cluster != args.clustername: + raise RuntimeError( + f"cluster preflight failed: expected local cluster macro {args.clustername!r}, " + f"got {actual_cluster!r}" + ) + + cluster_name = args.clustername.replace("'", "\\\\'") + actual_replicas = _query_single_value( + f"SELECT count() FROM clusterAllReplicas('{cluster_name}', system.one)" + ) + if actual_replicas != args.expected_replicas: + raise RuntimeError( + f"replica preflight failed: expected {args.expected_replicas}, got {actual_replicas}" + ) + + logger.info( + f"cluster preflight passed: cluster={args.clustername}, replicas={actual_replicas}" + ) + + def connect_to_ch(): logger.info( f"Connecting to ClickHouse, host={args.chhost}, port={args.chport}, username={args.chuser}, password={args.chpass}, s3path={args.s3path}, bucket={args.s3bucket}, s3path={args.s3path}" @@ -519,17 +590,25 @@ def connect_to_s3(): logger.debug(f"using SSL certificate {args.s3sslcertfile}") os.environ["SSL_CERT_FILE"] = args.s3sslcertfile + authentication = "AWS workload identity" if args.s3useiam else "static credentials" logger.info( - f"Connecting to S3, host:port={args.s3ip}:{args.s3port}, access_key={args.s3accesskey}, secret_key={args.s3secretkey}, secure={args.s3secure_flag}, region={args.s3region}" + f"Connecting to S3, host:port={args.s3ip}:{args.s3port}, authentication={authentication}, " + f"secure={args.s3secure_flag}, region={args.s3region}" ) global minio_client + connection_options = { + "secure": args.s3secure_flag, + "region": args.s3region, + "http_client": urllib3.PoolManager(cert_reqs="CERT_NONE"), + } + if args.s3useiam: + connection_options["credentials"] = IamAwsProvider() + else: + connection_options["access_key"] = args.s3accesskey + connection_options["secret_key"] = args.s3secretkey minio_client = Minio( f"{args.s3ip}:{args.s3port}", - access_key=args.s3accesskey, - secret_key=args.s3secretkey, - secure=args.s3secure_flag, - region=args.s3region, - http_client=urllib3.PoolManager(cert_reqs="CERT_NONE"), + **connection_options, ) @@ -595,6 +674,9 @@ def do_collect(): def do_use(): + if not args.dryrun_flag: + preflight_cluster() + srdp = "system.remote_data_paths" if args.clustername: srdp = f"clusterAllReplicas('{args.clustername}', {srdp})" @@ -662,46 +744,76 @@ def make_antijoin(calc_only=False, sample=None): num_removed = 0 total_size = 0 - objs = [] - + if not args.dryrun_flag and args.deletebatchsize < 1: + raise ValueError("--deletebatchsize must be a positive integer") for sample in range(0, args.samples): antijoin = make_antijoin(sample=sample) logger.info(f"antijoin {antijoin}") with ch_client.query_row_block_stream(antijoin) as stream: for block in stream: - objects_to_remove = [] - object_to_remove = [] + selected_rows = [] for row in block: logger.debug( f"{'removing' if not args.dryrun_flag else 'would remove if no dryrun flag'} {row[0]} of size {row[1]}" ) + selected_rows.append(row) + + if args.dryrun_flag: + num_removed += len(selected_rows) + total_size += sum(row[1] for row in selected_rows) + continue + + for offset in range(0, len(selected_rows), args.deletebatchsize): + batch_rows = selected_rows[offset : offset + args.deletebatchsize] + errors = [] if args.use_remove_objects: - objects_to_remove.append(DeleteObject(row[0])) - else: - object_to_remove.append(row[0]) - objs.append([row[0], row[1], row[2], False]) - total_size += row[1] - if not args.dryrun_flag: - if args.use_remove_objects: - errors = minio_client.remove_objects( - args.s3bucket, objects_to_remove - ) + errors = list(minio_client.remove_objects( + args.s3bucket, [DeleteObject(row[0]) for row in batch_rows] + )) for error in errors: logger.info(f"error occurred when deleting object via remove_objects {error}") + + failed_names = { + getattr(error, "object_name", None) or getattr(error, "name", None) + for error in errors + } + if None in failed_names: + # Do not tombstone any object for an uncorrelatable batch error. + successful_rows = [] + else: + successful_rows = [ + row for row in batch_rows if row[0] not in failed_names + ] else: - for object_path in object_to_remove: + successful_rows = [] + for row in batch_rows: try: - minio_client.remove_object( - args.s3bucket, object_path - ) + minio_client.remove_object(args.s3bucket, row[0]) + successful_rows.append(row) except Exception as error: - logger.info(f"error occurred when deleting object {object_path} via remove_object {error}") - - num_removed += len(objects_to_remove) + logger.info(f"error occurred when deleting object {row[0]} via remove_object {error}") + errors.append(error) + + if successful_rows: + tombstones = [ + [row[0], row[1], row[2], False] for row in successful_rows + ] + ch_client.insert( + tname, + tombstones, + column_names=["objpath", "size", "last_modified", "active"], + ) + num_removed += len(successful_rows) + total_size += sum(row[1] for row in successful_rows) + logger.info( + f"delete checkpoint: {num_removed} objects / {total_size} bytes removed so far" + ) - if not args.dryrun_flag: - ch_client.insert(tname, objs, column_names=["objpath", "size", "last_modified", "active"]) + if errors: + raise S3DeletionError( + f"{len(errors)} S3 deletion error(s); successful deletes were checkpointed" + ) logger.info( f"{num_removed} objects of total size {total_size} {'are removed' if not args.dryrun_flag else 'would be removed but for dryrun flag'}" diff --git a/tests/test_s3gc.py b/tests/test_s3gc.py new file mode 100644 index 0000000..8c4e3a0 --- /dev/null +++ b/tests/test_s3gc.py @@ -0,0 +1,176 @@ +import os +import runpy +import subprocess +import sys +import tempfile +import types +import unittest +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[1] + + +def load_s3gc(): + original_argv = sys.argv[:] + try: + sys.argv = [str(ROOT / "s3gc.py")] + return runpy.run_path(str(ROOT / "s3gc.py"), run_name="s3gc_test") + finally: + sys.argv = original_argv + + +class QueryResult: + def __init__(self, value): + self.result_rows = [(value,)] + + +class FakeStream: + def __init__(self, blocks): + self.blocks = blocks + + def __enter__(self): + return iter(self.blocks) + + def __exit__(self, *args): + return False + + +class FakeCH: + def __init__(self, cluster="cluster", replicas=2, blocks=()): + self.cluster = cluster + self.replicas = replicas + self.blocks = blocks + self.inserts = [] + + def query(self, query): + if "getMacro" in query: + return QueryResult(self.cluster) + if "clusterAllReplicas" in query and "system.one" in query: + return QueryResult(self.replicas) + raise AssertionError(query) + + def command(self, query): + if "COUNT(1)" in query: + return 1 + raise AssertionError(query) + + def query_row_block_stream(self, query): + return FakeStream(self.blocks) + + def insert(self, table, rows, column_names): + self.inserts.append((table, rows, column_names)) + + +class DeleteError: + def __init__(self, name): + self.name = name + + +class FakeMinio: + def remove_objects(self, bucket, objects): + return iter([DeleteError("bad-object")]) + + +def use_args(**overrides): + values = { + "clustername": "cluster", + "expected_replicas": 2, + "dryrun_flag": False, + "s3diskname": "s3", + "useafter": None, + "useage": 24, + "usetotal": None, + "samples": 1, + "deletebatchsize": 1000, + "interactive_flag": False, + "use_remove_objects": True, + "s3bucket": "bucket", + "keepdata_flag": True, + "silent_flag": True, + } + values.update(overrides) + return types.SimpleNamespace(**values) + + +class S3GCTest(unittest.TestCase): + def test_preflight_rejects_wrong_cluster(self): + module = load_s3gc() + namespace = module["preflight_cluster"].__globals__ + namespace["args"] = use_args(clustername="expected") + namespace["ch_client"] = FakeCH(cluster="actual") + + with self.assertRaisesRegex(RuntimeError, "cluster preflight failed"): + module["preflight_cluster"]() + + def test_batch_errors_checkpoint_only_confirmed_deletes(self): + module = load_s3gc() + namespace = module["do_use"].__globals__ + namespace["args"] = use_args() + namespace["ch_client"] = FakeCH( + blocks=[[("good-object", 10, "time"), ("bad-object", 20, "time")]] + ) + namespace["minio_client"] = FakeMinio() + + with self.assertRaises(module["S3DeletionError"]): + module["do_use"]() + + self.assertEqual(len(namespace["ch_client"].inserts), 1) + self.assertEqual( + namespace["ch_client"].inserts[0][1], [["good-object", 10, "time", False]] + ) + + def test_delete_batches_are_checkpointed_independently(self): + module = load_s3gc() + namespace = module["do_use"].__globals__ + namespace["args"] = use_args(deletebatchsize=1) + namespace["ch_client"] = FakeCH( + blocks=[[("object-a", 10, "time"), ("object-b", 20, "time")]] + ) + + class SuccessfulMinio: + def remove_objects(self, bucket, objects): + return iter(()) + + namespace["minio_client"] = SuccessfulMinio() + module["do_use"]() + + self.assertEqual(len(namespace["ch_client"].inserts), 2) + self.assertEqual(namespace["ch_client"].inserts[0][1], [["object-a", 10, "time", False]]) + self.assertEqual(namespace["ch_client"].inserts[1][1], [["object-b", 20, "time", False]]) + + def test_delete_entrypoint_requires_confirmation(self): + result = subprocess.run( + ["sh", str(ROOT / "kubernetes-entrypoint.sh")], + env={ + **os.environ, + "S3GC_PHASE": "delete", + "S3GC_CLUSTERNAME": "cluster", + "S3GC_EXPECTED_REPLICAS": "2", + }, + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 64) + self.assertIn("Refusing delete", result.stderr) + + def test_render_rejects_unacknowledged_delete(self): + source = (ROOT / "deploy/kubernetes/example.env").read_text() + with tempfile.NamedTemporaryFile("w", suffix=".env", delete=False) as config: + config.write(source.replace("PHASE=dry-run", "PHASE=delete")) + config_path = config.name + self.addCleanup(lambda: os.unlink(config_path)) + + result = subprocess.run( + [sys.executable, str(ROOT / "deploy/kubernetes/render.py"), config_path], + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(result.returncode, 64) + self.assertIn("delete requires", result.stderr) + + +if __name__ == "__main__": + unittest.main() From f06b6695796f396cb48ec2f4e19d842fea2bc6e3 Mon Sep 17 00:00:00 2001 From: Diego Nieto Date: Tue, 4 Aug 2026 16:52:54 +0200 Subject: [PATCH 03/16] Added Pytest instead of UnitTest and some README info --- .github/workflows/container.yml | 25 +- .gitignore | 1 + .python-version | 1 + Dockerfile | 54 ---- Dockerfile.in | 15 - Makefile | 7 - README.md | 44 ++- deploy/kubernetes/README.md | 255 +++++++++++++--- deploy/kubernetes/example.env | 2 + deploy/kubernetes/job.yaml.tmpl | 4 +- deploy/kubernetes/render.py | 3 + docker/Dockerfile | 14 + .../kubernetes-entrypoint.sh | 2 +- pytest.ini | 5 + requirements-dev.txt | 1 + s3gc.py | 83 ++++- tests/conftest.py | 24 ++ tests/test_s3gc.py | 283 +++++++++++------- 18 files changed, 569 insertions(+), 254 deletions(-) create mode 100644 .python-version delete mode 100644 Dockerfile delete mode 100644 Dockerfile.in delete mode 100644 Makefile create mode 100644 docker/Dockerfile rename kubernetes-entrypoint.sh => docker/kubernetes-entrypoint.sh (96%) create mode 100644 pytest.ini create mode 100644 requirements-dev.txt create mode 100644 tests/conftest.py diff --git a/.github/workflows/container.yml b/.github/workflows/container.yml index fed3b38..97f721e 100644 --- a/.github/workflows/container.yml +++ b/.github/workflows/container.yml @@ -5,6 +5,10 @@ on: push: branches: [master] +env: + # This repository must be private in the configured registry. + IMAGE_REPOSITORY: altinity/s3gc + jobs: test: runs-on: ubuntu-latest @@ -13,11 +17,13 @@ jobs: - uses: actions/setup-python@v5 with: python-version: "3.11" - - run: python -m pip install --disable-pip-version-check -r requirements.txt - - run: python -m unittest discover -s tests -v + - run: python -m pip install --disable-pip-version-check -r requirements.txt -r requirements-dev.txt + - run: pytest -v - run: python deploy/kubernetes/render.py deploy/kubernetes/example.env > /tmp/s3gc-job.yaml - build: + publish: + needs: test + if: github.event_name == 'push' && github.ref == 'refs/heads/master' runs-on: ubuntu-latest permissions: contents: read @@ -26,15 +32,18 @@ jobs: - uses: docker/setup-qemu-action@v3 - uses: docker/setup-buildx-action@v3 - uses: docker/login-action@v3 - if: github.event_name == 'push' with: username: ${{ secrets.DOCKERHUB_USERNAME }} password: ${{ secrets.DOCKERHUB_TOKEN }} - - uses: docker/build-push-action@v6 + - id: image + uses: docker/build-push-action@v6 with: context: . + file: docker/Dockerfile platforms: linux/amd64,linux/arm64 - push: ${{ github.event_name == 'push' }} + push: true tags: | - altinity/s3gc:sha-${{ github.sha }} - altinity/s3gc:latest + ${{ env.IMAGE_REPOSITORY }}:sha-${{ github.sha }} + ${{ env.IMAGE_REPOSITORY }}:latest + - name: Record immutable image reference + run: echo "${IMAGE_REPOSITORY}@${{ steps.image.outputs.digest }}" >> "$GITHUB_STEP_SUMMARY" diff --git a/.gitignore b/.gitignore index f8b8d8b..5f55853 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ __pycache__/ *.pyc options.lst +.pytest_cache/ diff --git a/.python-version b/.python-version new file mode 100644 index 0000000..2c07333 --- /dev/null +++ b/.python-version @@ -0,0 +1 @@ +3.11 diff --git a/Dockerfile b/Dockerfile deleted file mode 100644 index 735e73e..0000000 --- a/Dockerfile +++ /dev/null @@ -1,54 +0,0 @@ -FROM python:3.11-slim - -WORKDIR /usr/src/app - -COPY requirements.txt ./ -RUN pip install --no-cache-dir --disable-pip-version-check -r requirements.txt - -COPY . . - -RUN chmod 0555 /usr/src/app/kubernetes-entrypoint.sh - -ENV S3GC_CHHOST=localhost \ - S3GC_CHPORT=8123 \ - S3GC_CHUSER=default \ - S3GC_CHPASS='' \ - S3GC_S3IP=127.0.0.1 \ - S3GC_S3PORT=9001 \ - S3GC_S3BUCKET=root \ - S3GC_S3PATH=data/ \ - S3GC_S3ACCESSKEY='' \ - S3GC_S3SECRETKEY='' \ - S3GC_S3SECURE_FLAG=false \ - S3GC_S3SSLCERTFILE='' \ - S3GC_S3REGION=null \ - S3GC_S3DISKNAME=s3 \ - S3GC_S3USEIAM=false \ - S3GC_KEEPDATA_FLAG=false \ - S3GC_COLLECTONLY_FLAG=false \ - S3GC_USECOLLECTED_FLAG=false \ - S3GC_COLLECTTABLEPREFIX=s3objects_for_ \ - S3GC_COLLECTBATCHSIZE=1024 \ - S3GC_TOTAL=null \ - S3GC_COLLECTAFTER=null \ - S3GC_USEAFTER=null \ - S3GC_USETOTAL=null \ - S3GC_DRYRUN_FLAG=false \ - S3GC_CLUSTERNAME='' \ - S3GC_EXPECTED_REPLICAS=null \ - S3GC_AGE=0 \ - S3GC_USEAGE=0 \ - S3GC_SAMPLES=4 \ - S3GC_DELETEBATCHSIZE=1000 \ - S3GC_CHTIMEOUT=1800 \ - S3GC_CREATEDATABASE_FLAG=false \ - S3GC_DROP_COLLECTTABLE_FLAG=false \ - S3GC_USE_REMOVE_OBJECTS=true \ - S3GC_INTERACTIVE_FLAG=true \ - S3GC_VERBOSE_FLAG=false \ - S3GC_DEBUG_FLAG=false \ - S3GC_SILENT_FLAG=false - - -ENTRYPOINT ["python", "./s3gc.py"] -# CMD ["--help" ] diff --git a/Dockerfile.in b/Dockerfile.in deleted file mode 100644 index 5bae422..0000000 --- a/Dockerfile.in +++ /dev/null @@ -1,15 +0,0 @@ -FROM python:3.11-slim - -WORKDIR /usr/src/app - -COPY requirements.txt ./ -RUN pip install --no-cache-dir --disable-pip-version-check -r requirements.txt - -COPY . . - -RUN chmod 0555 /usr/src/app/kubernetes-entrypoint.sh - -# @@ - -ENTRYPOINT ["python", "./s3gc.py"] -# CMD ["--help" ] diff --git a/Makefile b/Makefile deleted file mode 100644 index f9d33fe..0000000 --- a/Makefile +++ /dev/null @@ -1,7 +0,0 @@ -PYTHON ?= python3 - -Dockerfile: Dockerfile.in options.lst - $(PYTHON) -c "import sys; sys.stdout.write(sys.stdin.read().replace('# @@', open('./options.lst', 'r').read()))" < Dockerfile.in > Dockerfile - -options.lst: ./s3gc.py - $(PYTHON) ./s3gc.py --listoptions > options.lst diff --git a/README.md b/README.md index 56d5c48..f17250a 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,31 @@ # s3gc Garbage collector for ClickHouse S3 disks +## Repository layout + +- `s3gc.py` — collector and deletion logic. +- `docker/` — reproducible Python 3.11 container packaging. +- `deploy/kubernetes/` — generic one-shot Kubernetes Job renderer and operator runbook. +- `tests/` — safety and renderer unit tests. + +The repository is intentionally public and contains no target-cluster details, +credentials, rendered manifests, or environment configuration. Operators keep +those values outside Git and deploy an image pinned by digest. + +## Testing + +Install test-only dependencies with +`uv pip install --python .venv/bin/python -r requirements-dev.txt`, then run +the isolated unit suite with: + +``` +.venv/bin/python -m pytest -v +``` + +Tests marked `dev_cluster` require the dedicated development Kubernetes cluster +and are never run by CI. They must be selected explicitly with +`pytest -m dev_cluster` after reviewing their fixture scope. + ## description The script removes orphaned objects from s3 object storage Ones that are not mentioned in system.remote_data_paths table @@ -72,22 +97,19 @@ S3GC_S3PORT=19000 S3GC_S3ACCESSKEY=minio99 S3GC_S3SECRETKEY=minio123 S3GC_USEC ## docker There is a docker image for the script. -The published image is pinned to Python 3.11 for reproducibility. - -When regenerating the Dockerfile defaults, run `make` with an interpreter that -has the pinned requirements installed, for example -`make PYTHON=.venv/bin/python`. +Development, CI, and the image use Python 3.11. With `uv` installed, create the +local environment with `uv venv --python 3.11 .venv`, then install the pinned +requirements with `uv pip install --python .venv/bin/python -r requirements.txt`. ### rebuild ``` -make -sudo docker buildx build --platform linux/arm/v7,linux/arm64/v8,linux/amd64 -t altinity/s3gc . +docker buildx build --platform linux/amd64,linux/arm64 -f docker/Dockerfile -t altinity/s3gc . ``` ### usage ``` -sudo docker run altinity/s3gc --help -sudo docker run --network="host" -e S3GC_S3PORT=19000 -e S3GC_S3ACCESSKEY=minio99 -e S3GC_S3SECRETKEY=minio123 altinity/s3gc +docker run altinity/s3gc --help +docker run --network="host" -e S3GC_S3PORT=19000 -e S3GC_S3ACCESSKEY=minio99 -e S3GC_S3SECRETKEY=minio123 altinity/s3gc ``` ## Kubernetes @@ -98,6 +120,10 @@ the collector inside the ClickHouse namespace. It has separate `collect`, See [deploy/kubernetes/README.md](deploy/kubernetes/README.md) for the render contract and safety requirements. +Images are published only by the GitHub Actions workflow after a trusted push to +protected `master`. Pull requests run tests without registry credentials and do +not publish an image. + ## changelog ### v_0.1 Wed Jun 12 2024 diff --git a/deploy/kubernetes/README.md b/deploy/kubernetes/README.md index ba6d013..db347b5 100644 --- a/deploy/kubernetes/README.md +++ b/deploy/kubernetes/README.md @@ -1,67 +1,236 @@ # Kubernetes Job runner -The runner executes `s3gc` in the same namespace as ClickHouse so it reaches the -cluster service directly and does not depend on a `kubectl port-forward`. +This directory runs `s3gc` as a one-shot Kubernetes Job in the same namespace +as ClickHouse. It uses the in-cluster ClickHouse Service directly, so it does +not depend on a laptop or `kubectl port-forward`. + +The runner has three separate phases: `collect`, `dry-run`, and `delete`. +Run them in that order. Only the delete phase changes S3. + +## Safety rules + +- Use a unique auxiliary-table prefix for each bucket/prefix cleanup. +- Never delete by S3 age or prefix heuristic. Delete only after a successful + cluster-wide dry-run has been reviewed and approved. +- `delete` requires `DELETE_CONFIRMATION=DELETE_ORPHANS`, a target cluster + name, and the expected replica count. +- The delete Job checks the local cluster macro and reachable replica count + before deleting. It records confirmed deletion tombstones after every + `DELETE_BATCH_SIZE` objects. +- Jobs have `backoffLimit: 0`: a failed Job never retries automatically. A + replacement Job with a new name and the same auxiliary table resumes safely. +- The Job has no Kubernetes API token and no in-pod Kubernetes RBAC. + +## 1. Publish and pin the image + +Merge the reviewed code through protected `master`. The publishing workflow +creates a private multi-architecture image for `linux/amd64` and `linux/arm64`. +Copy the immutable digest from the workflow summary: + +```text +altinity/s3gc@sha256: +``` + +Use this digest in deployment configuration. Do not use `latest` or a mutable +tag. + +## 2. Identify the customer target + +Use the explicit customer kubeconfig and namespace; do not rely on your default +Kubernetes context. + +```bash +KUBECONFIG=/secure/customer.kubeconfig \ + kubectl -n get svc +``` + +Before rendering a Job, determine and review: + +- `CHHOST`: the in-namespace ClickHouse Service name, not `localhost`. +- `CLUSTERNAME`: the exact ClickHouse `cluster` macro. +- `EXPECTED_REPLICAS`: the number of replicas expected to be reachable during + deletion. +- S3 endpoint, port, bucket, prefix, region, TLS setting, and ClickHouse disk + name. +- a unique `COLLECTTABLEPREFIX` for this bucket/prefix pair. + +The ClickHouse user must be able to read `system.remote_data_paths` across the +target cluster and create, insert into, select from, and truncate the auxiliary +table. + +## 3. Create Kubernetes prerequisites + +These resources are namespace-local and intentionally not created by this +repository. + +Create or reuse a dedicated ServiceAccount. Static S3 credentials require no +Kubernetes RBAC: + +```bash +kubectl -n create serviceaccount s3gc +``` + +The published image is private, so create an image-pull Secret: + +```bash +kubectl -n create secret docker-registry altinity-s3gc-pull \ + --docker-server=https://index.docker.io/v1/ \ + --docker-username='' \ + --docker-password='' +``` + +### Static S3 credentials + +For customers without workload identity, create the runtime Secret with four +keys. Provide actual values through your approved secret manager or an +interactive terminal; never commit a Secret manifest or `.env` file containing +credentials. + +```bash +kubectl -n create secret generic s3gc-runtime \ + --from-literal=S3GC_CHUSER='' \ + --from-literal=S3GC_CHPASS='' \ + --from-literal=S3GC_S3ACCESSKEY='' \ + --from-literal=S3GC_S3SECRETKEY='' +``` + +Set `S3USEIAM=false` in the deployment config. `s3gc` then uses the access and +secret keys directly and does not attempt workload-identity authentication. -## Safety model +### Workload identity -- Render and apply exactly one phase at a time: `collect`, `dry-run`, or `delete`. -- `delete` requires `DELETE_CONFIRMATION=DELETE_ORPHANS`, a cluster name, and an - expected replica count. The program validates the local macro and the number of - reachable replicas before it deletes anything. -- Jobs never retry automatically. Successful S3 deletions are tombstoned in the - auxiliary table after every `DELETE_BATCH_SIZE` objects, with an accumulated - progress line in the Job log, so a manually rerun Job resumes safely. -- The Job does not receive a Kubernetes API token and receives no in-pod RBAC. +For EKS/IRSA or another workload-identity setup, annotate or reuse the +identity-enabled ServiceAccount, set `S3USEIAM=true`, and provide only +`S3GC_CHUSER` and `S3GC_CHPASS` in the runtime Secret. -## Credentials +## 4. Create a non-secret deployment config -Create or reference a namespaced Secret outside this repository. For static S3 -credentials it must contain these four keys, which are passed unchanged to -`s3gc`: +Copy the template to a secure location outside the repository: +```bash +cp deploy/kubernetes/example.env /secure/s3gc-customer.env ``` -S3GC_CHUSER -S3GC_CHPASS -S3GC_S3ACCESSKEY -S3GC_S3SECRETKEY + +Set the target values. This static-S3 example shows the important fields: + +```dotenv +JOB_NAME=s3gc-customer-collect +NAMESPACE= +IMAGE=altinity/s3gc@sha256: +IMAGE_PULL_SECRET=altinity-s3gc-pull +CREDENTIALS_SECRET=s3gc-runtime +SERVICE_ACCOUNT=s3gc + +CHHOST= +CHPORT=8123 +CLUSTERNAME= +EXPECTED_REPLICAS= +COLLECTTABLEPREFIX=s3gc_customer_20260804_ + +S3IP= +S3PORT=443 +S3BUCKET= +S3PATH= +S3REGION= +S3SECURE_FLAG=true +S3DISKNAME= +S3USEIAM=false + +SAMPLES=4 +DELETE_BATCH_SIZE=1000 +USEAGE_HOURS=24 +ORDER_BY_OBJPATH=false +ACTIVE_DEADLINE_SECONDS=43200 +TTL_SECONDS_AFTER_FINISHED=604800 +MEMORY_REQUEST=1Gi +MEMORY_LIMIT=4Gi +VERBOSE=true ``` -Do not put secret values in the config file or rendered manifest. The secret -provisioning mechanism is intentionally environment-owned. +For a large production cleanup, begin with `SAMPLES=4`, `USEAGE_HOURS=24`, and +a 12-hour deadline. `ORDER_BY_OBJPATH=false` avoids an unnecessary global sort +that can consume substantial ClickHouse memory and CPU. + +## 5. Collect the S3 inventory -For EKS/IRSA or another AWS workload-identity setup, set `S3USEIAM=true` and -set `SERVICE_ACCOUNT` to the identity-enabled ServiceAccount. In that mode the -Secret only needs `S3GC_CHUSER` and `S3GC_CHPASS`; static S3 keys are not used. +Set these values in the secure config: -## Render and validate +```dotenv +PHASE=collect +DELETE_CONFIRMATION= +JOB_NAME=s3gc-customer-collect +``` -Copy `example.env` outside the repository and fill the target values. Use an -immutable image digest after publishing the image: +Render, validate, apply, and follow the Job log: ```bash -python3 deploy/kubernetes/render.py /secure/path/s3gc.env > /tmp/s3gc-job.yaml -kubectl apply --dry-run=client -f /tmp/s3gc-job.yaml +python3 deploy/kubernetes/render.py /secure/s3gc-customer.env \ + > /secure/s3gc-customer-collect.yaml +kubectl apply --dry-run=server -f /secure/s3gc-customer-collect.yaml +kubectl apply -f /secure/s3gc-customer-collect.yaml +kubectl -n logs -f job/s3gc-customer-collect +``` + +The collect phase creates and populates the auxiliary ClickHouse table. It does +not delete S3 objects. + +## 6. Dry-run the cluster-wide anti-join + +Keep every target field and `COLLECTTABLEPREFIX` identical. Change only: + +```dotenv +PHASE=dry-run +DELETE_CONFIRMATION= +JOB_NAME=s3gc-customer-dry-run +``` + +Render and apply a new manifest using the commands above. The final Job log +reports a line such as: + +```text + objects of total size would be removed but for dryrun flag ``` -`IMAGE_PULL_SECRET` is a namespaced `kubernetes.io/dockerconfigjson` Secret for -the registry containing the pinned image. It is separate from the runtime -credentials Secret and must be created by the environment owner. +Review the count and size with the customer. Do not continue based only on a +successful Job exit status. + +## 7. Delete only after explicit approval + +After the dry-run result has been approved, keep all target fields identical +and change only: + +```dotenv +PHASE=delete +DELETE_CONFIRMATION=DELETE_ORPHANS +JOB_NAME=s3gc-customer-delete +``` -Apply only after a separate explicit approval for the target environment: +Render, server-dry-run, and apply a new manifest. Follow the logs: ```bash -kubectl apply -f /tmp/s3gc-job.yaml -kubectl logs -f job/ +kubectl -n logs -f job/s3gc-customer-delete ``` -For a fresh run, use `PHASE=collect` and a unique `COLLECTTABLEPREFIX`. The -collect Job retains the table. Render `PHASE=dry-run` next, inspect its result, -then render `PHASE=delete` plus the required delete confirmation only when -approved. Never reuse an auxiliary table for a different bucket/prefix. +Before deleting, the Job verifies `CLUSTERNAME` and `EXPECTED_REPLICAS`. During +deletion, logs show durable checkpoints such as: -Set `VERBOSE=true` for the Job logs to include its connection, collection, and -anti-join totals. +```text +delete checkpoint: 1000 objects / removed so far +``` + +If the Job fails, do not re-run collect. Diagnose the failure, then render a +new delete Job name using the same auxiliary-table prefix. Objects already +deleted successfully are tombstoned and are not selected again. + +## 8. Verify and retain evidence + +Run one final dry-run with: + +```dotenv +PHASE=dry-run +DELETE_CONFIRMATION= +JOB_NAME=s3gc-customer-verify +``` -`DELETE_BATCH_SIZE` controls delete progress granularity (use `1000` normally; -use a smaller value only when more frequent progress checkpoints are useful). +It must report zero candidates. Retain the auxiliary table and completed Job +logs for audit. Do not reuse that table for another bucket or prefix. diff --git a/deploy/kubernetes/example.env b/deploy/kubernetes/example.env index bf0d269..23ab36a 100644 --- a/deploy/kubernetes/example.env +++ b/deploy/kubernetes/example.env @@ -27,6 +27,8 @@ S3USEIAM=false SAMPLES=4 DELETE_BATCH_SIZE=1000 USEAGE_HOURS=24 +# Leave false for Kubernetes Jobs; global ordering is unnecessary for deletion. +ORDER_BY_OBJPATH=false ACTIVE_DEADLINE_SECONDS=14400 TTL_SECONDS_AFTER_FINISHED=604800 MEMORY_REQUEST=1Gi diff --git a/deploy/kubernetes/job.yaml.tmpl b/deploy/kubernetes/job.yaml.tmpl index 073d4e3..1786058 100644 --- a/deploy/kubernetes/job.yaml.tmpl +++ b/deploy/kubernetes/job.yaml.tmpl @@ -33,7 +33,7 @@ spec: - name: s3gc image: "${IMAGE}" imagePullPolicy: IfNotPresent - command: ["/usr/src/app/kubernetes-entrypoint.sh"] + command: ["/app/kubernetes-entrypoint.sh"] securityContext: allowPrivilegeEscalation: false readOnlyRootFilesystem: true @@ -86,6 +86,8 @@ spec: value: "${DELETE_BATCH_SIZE}" - name: S3GC_USEAGE value: "${USEAGE_HOURS}" + - name: S3GC_ORDER_BY_OBJPATH + value: "${ORDER_BY_OBJPATH}" - name: S3GC_CHTIMEOUT value: "${ACTIVE_DEADLINE_SECONDS}" - name: S3GC_VERBOSE_FLAG diff --git a/deploy/kubernetes/render.py b/deploy/kubernetes/render.py index 3da7267..cfc584a 100644 --- a/deploy/kubernetes/render.py +++ b/deploy/kubernetes/render.py @@ -24,6 +24,7 @@ "MEMORY_LIMIT", "MEMORY_REQUEST", "NAMESPACE", + "ORDER_BY_OBJPATH", "PHASE", "S3BUCKET", "S3DISKNAME", @@ -84,6 +85,8 @@ def validate(values: dict[str, str]) -> None: raise ValueError("S3USEIAM must be true or false") if values["VERBOSE"] not in {"true", "false"}: raise ValueError("VERBOSE must be true or false") + if values["ORDER_BY_OBJPATH"] not in {"true", "false"}: + raise ValueError("ORDER_BY_OBJPATH must be true or false") def main() -> int: diff --git a/docker/Dockerfile b/docker/Dockerfile new file mode 100644 index 0000000..3a1dca8 --- /dev/null +++ b/docker/Dockerfile @@ -0,0 +1,14 @@ +FROM python:3.11-slim + +WORKDIR /app + +COPY requirements.txt ./ +RUN pip install --no-cache-dir --disable-pip-version-check -r requirements.txt + +COPY s3gc.py ./ +COPY docker/kubernetes-entrypoint.sh ./kubernetes-entrypoint.sh +RUN chmod 0555 /app/kubernetes-entrypoint.sh + +USER 65532:65532 + +ENTRYPOINT ["python", "/app/s3gc.py"] diff --git a/kubernetes-entrypoint.sh b/docker/kubernetes-entrypoint.sh similarity index 96% rename from kubernetes-entrypoint.sh rename to docker/kubernetes-entrypoint.sh index be82e74..3a3b9ca 100644 --- a/kubernetes-entrypoint.sh +++ b/docker/kubernetes-entrypoint.sh @@ -30,4 +30,4 @@ case "${phase}" in ;; esac -exec python ./s3gc.py "$@" +exec python /app/s3gc.py "$@" diff --git a/pytest.ini b/pytest.ini new file mode 100644 index 0000000..c993f2f --- /dev/null +++ b/pytest.ini @@ -0,0 +1,5 @@ +[pytest] +addopts = -ra +testpaths = tests +markers = + dev_cluster: requires the dedicated development Kubernetes cluster and is never run in CI diff --git a/requirements-dev.txt b/requirements-dev.txt new file mode 100644 index 0000000..2c78728 --- /dev/null +++ b/requirements-dev.txt @@ -0,0 +1 @@ +pytest==8.3.5 diff --git a/s3gc.py b/s3gc.py index 0c38a02..b808655 100644 --- a/s3gc.py +++ b/s3gc.py @@ -20,6 +20,7 @@ from minio import Minio from minio.deleteobjects import DeleteObject from minio.credentials import IamAwsProvider +from minio.error import S3Error from contextlib import redirect_stdout import clickhouse_connect @@ -316,6 +317,41 @@ def strtobool(value): default=1000, help="S3 objects to delete and checkpoint per progress batch", ) +parser.add_argument( + "--order-by-objpath", + action="store_true", + dest="order_by_objpath", + default=False, + help="Order anti-join output by object path (costly for large Kubernetes Jobs)", +) +parser.add_argument( + "--order-by-objpath-flag", + dest="order_by_objpath", + type=bool, + default=False, + help="Order anti-join output by object path (costly for large Kubernetes Jobs)", +) +parser.add_argument( + "--s3-connect-timeout", + dest="s3_connect_timeout", + type=int, + default=15, + help="S3 connection timeout in seconds", +) +parser.add_argument( + "--s3-read-timeout", + dest="s3_read_timeout", + type=int, + default=120, + help="S3 read timeout in seconds", +) +parser.add_argument( + "--s3-retries", + dest="s3_retries", + type=int, + default=3, + help="S3 HTTP retries for transient failures", +) parser.add_argument( "--chtimeout", "--ch-timeout", @@ -599,7 +635,21 @@ def connect_to_s3(): connection_options = { "secure": args.s3secure_flag, "region": args.s3region, - "http_client": urllib3.PoolManager(cert_reqs="CERT_NONE"), + "http_client": urllib3.PoolManager( + cert_reqs="CERT_NONE", + timeout=urllib3.Timeout( + connect=args.s3_connect_timeout, read=args.s3_read_timeout + ), + retries=urllib3.Retry( + total=args.s3_retries, + connect=args.s3_retries, + read=args.s3_retries, + status=args.s3_retries, + backoff_factor=1, + status_forcelist=[429, 500, 502, 503, 504], + allowed_methods=frozenset({"DELETE", "GET", "HEAD", "POST"}), + ), + ), } if args.s3useiam: connection_options["credentials"] = IamAwsProvider() @@ -612,6 +662,30 @@ def connect_to_s3(): ) +def remove_objects_reconnecting(batch_rows): + """Delete one batch, reconnecting once if the S3 transport is stale. + + DeleteObject requests are idempotent: retrying after an interrupted response + can only leave the object absent, never delete a different object. + """ + for attempt in range(2): + try: + return list( + minio_client.remove_objects( + args.s3bucket, [DeleteObject(row[0]) for row in batch_rows] + ) + ) + except (S3Error, urllib3.exceptions.HTTPError) as exc: + if attempt: + raise + logger.warning( + "S3 delete transport failed (%s); reconnecting and retrying once", exc + ) + connect_to_s3() + + raise AssertionError("unreachable") + + def do_collect(): logger.debug(f"start_after {args.collectafter}") objects = minio_client.list_objects( @@ -704,11 +778,12 @@ def make_antijoin(calc_only=False, sample=None): if not calc_only: sample_condition = f"CRC32(s3o.objpath) % {args.samples} = {sample} AND " + order_by = " ORDER BY s3o.objpath" if args.order_by_objpath else "" antijoin = f""" SELECT s3o.objpath, s3o.size as size, s3o.last_modified as last_modified FROM {tname} AS s3o LEFT ANTI JOIN {srdp} AS rdp ON (rdp.remote_path = s3o.objpath AND rdp.disk_name='{args.s3diskname}') WHERE {sample_condition} s3o.active=true {after_condition} {age_condition} - ORDER BY s3o.objpath {limit} SETTINGS final = 1""" + {order_by} {limit} SETTINGS final = 1""" if calc_only: countantijoin = f"SELECT COUNT(1), SUM(size) FROM ({antijoin}) q" @@ -768,9 +843,7 @@ def make_antijoin(calc_only=False, sample=None): batch_rows = selected_rows[offset : offset + args.deletebatchsize] errors = [] if args.use_remove_objects: - errors = list(minio_client.remove_objects( - args.s3bucket, [DeleteObject(row[0]) for row in batch_rows] - )) + errors = remove_objects_reconnecting(batch_rows) for error in errors: logger.info(f"error occurred when deleting object via remove_objects {error}") diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..afda1a7 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,24 @@ +import logging +import runpy +import sys +from pathlib import Path + +import pytest + + +ROOT = Path(__file__).resolve().parents[1] + + +@pytest.fixture +def s3gc_module(monkeypatch): + """Load the script with isolated command-line arguments and logging.""" + logger = logging.getLogger("s3gc_test") + existing_handlers = list(logger.handlers) + monkeypatch.setattr(sys, "argv", [str(ROOT / "s3gc.py")]) + module = runpy.run_path(str(ROOT / "s3gc.py"), run_name="s3gc_test") + yield module + + for handler in list(logger.handlers): + if handler not in existing_handlers: + logger.removeHandler(handler) + handler.close() diff --git a/tests/test_s3gc.py b/tests/test_s3gc.py index 8c4e3a0..2595517 100644 --- a/tests/test_s3gc.py +++ b/tests/test_s3gc.py @@ -1,23 +1,13 @@ import os -import runpy import subprocess import sys -import tempfile import types -import unittest from pathlib import Path - -ROOT = Path(__file__).resolve().parents[1] +import pytest -def load_s3gc(): - original_argv = sys.argv[:] - try: - sys.argv = [str(ROOT / "s3gc.py")] - return runpy.run_path(str(ROOT / "s3gc.py"), run_name="s3gc_test") - finally: - sys.argv = original_argv +ROOT = Path(__file__).resolve().parents[1] class QueryResult: @@ -42,6 +32,7 @@ def __init__(self, cluster="cluster", replicas=2, blocks=()): self.replicas = replicas self.blocks = blocks self.inserts = [] + self.stream_query = "" def query(self, query): if "getMacro" in query: @@ -56,6 +47,7 @@ def command(self, query): raise AssertionError(query) def query_row_block_stream(self, query): + self.stream_query = query return FakeStream(self.blocks) def insert(self, table, rows, column_names): @@ -67,110 +59,179 @@ def __init__(self, name): self.name = name -class FakeMinio: +class FailingMinio: def remove_objects(self, bucket, objects): return iter([DeleteError("bad-object")]) -def use_args(**overrides): - values = { - "clustername": "cluster", - "expected_replicas": 2, - "dryrun_flag": False, - "s3diskname": "s3", - "useafter": None, - "useage": 24, - "usetotal": None, - "samples": 1, - "deletebatchsize": 1000, - "interactive_flag": False, - "use_remove_objects": True, - "s3bucket": "bucket", - "keepdata_flag": True, - "silent_flag": True, - } - values.update(overrides) - return types.SimpleNamespace(**values) - - -class S3GCTest(unittest.TestCase): - def test_preflight_rejects_wrong_cluster(self): - module = load_s3gc() - namespace = module["preflight_cluster"].__globals__ - namespace["args"] = use_args(clustername="expected") - namespace["ch_client"] = FakeCH(cluster="actual") - - with self.assertRaisesRegex(RuntimeError, "cluster preflight failed"): - module["preflight_cluster"]() - - def test_batch_errors_checkpoint_only_confirmed_deletes(self): - module = load_s3gc() - namespace = module["do_use"].__globals__ - namespace["args"] = use_args() - namespace["ch_client"] = FakeCH( - blocks=[[("good-object", 10, "time"), ("bad-object", 20, "time")]] - ) - namespace["minio_client"] = FakeMinio() - - with self.assertRaises(module["S3DeletionError"]): - module["do_use"]() - - self.assertEqual(len(namespace["ch_client"].inserts), 1) - self.assertEqual( - namespace["ch_client"].inserts[0][1], [["good-object", 10, "time", False]] +@pytest.fixture +def args_factory(): + def make_args(**overrides): + values = { + "clustername": "cluster", + "expected_replicas": 2, + "dryrun_flag": False, + "s3diskname": "s3", + "useafter": None, + "useage": 24, + "usetotal": None, + "samples": 1, + "deletebatchsize": 1000, + "order_by_objpath": False, + "interactive_flag": False, + "use_remove_objects": True, + "s3bucket": "bucket", + "keepdata_flag": True, + "silent_flag": True, + } + values.update(overrides) + return types.SimpleNamespace(**values) + + return make_args + + +def test_preflight_rejects_wrong_cluster(s3gc_module, args_factory, monkeypatch): + namespace = s3gc_module["preflight_cluster"].__globals__ + monkeypatch.setitem(namespace, "args", args_factory(clustername="expected")) + monkeypatch.setitem(namespace, "ch_client", FakeCH(cluster="actual")) + + with pytest.raises(RuntimeError, match="cluster preflight failed"): + s3gc_module["preflight_cluster"]() + + +def test_batch_errors_checkpoint_only_confirmed_deletes( + s3gc_module, args_factory, monkeypatch +): + namespace = s3gc_module["do_use"].__globals__ + client = FakeCH(blocks=[[("good-object", 10, "time"), ("bad-object", 20, "time")]]) + monkeypatch.setitem(namespace, "args", args_factory()) + monkeypatch.setitem(namespace, "ch_client", client) + monkeypatch.setitem(namespace, "minio_client", FailingMinio()) + + with pytest.raises(s3gc_module["S3DeletionError"]): + s3gc_module["do_use"]() + + assert client.inserts == [ + ( + "`s3objects_for_s3`", + [["good-object", 10, "time", False]], + ["objpath", "size", "last_modified", "active"], ) - - def test_delete_batches_are_checkpointed_independently(self): - module = load_s3gc() - namespace = module["do_use"].__globals__ - namespace["args"] = use_args(deletebatchsize=1) - namespace["ch_client"] = FakeCH( - blocks=[[("object-a", 10, "time"), ("object-b", 20, "time")]] - ) - - class SuccessfulMinio: - def remove_objects(self, bucket, objects): - return iter(()) - + ] + + +def test_delete_batches_are_checkpointed_independently( + s3gc_module, args_factory, monkeypatch +): + namespace = s3gc_module["do_use"].__globals__ + client = FakeCH(blocks=[[("object-a", 10, "time"), ("object-b", 20, "time")]]) + + class SuccessfulMinio: + def remove_objects(self, bucket, objects): + return iter(()) + + monkeypatch.setitem(namespace, "args", args_factory(deletebatchsize=1)) + monkeypatch.setitem(namespace, "ch_client", client) + monkeypatch.setitem(namespace, "minio_client", SuccessfulMinio()) + s3gc_module["do_use"]() + + assert [insert[1] for insert in client.inserts] == [ + [["object-a", 10, "time", False]], + [["object-b", 20, "time", False]], + ] + + +def test_delete_entrypoint_requires_confirmation(): + result = subprocess.run( + ["sh", str(ROOT / "docker/kubernetes-entrypoint.sh")], + env={ + **os.environ, + "S3GC_PHASE": "delete", + "S3GC_CLUSTERNAME": "cluster", + "S3GC_EXPECTED_REPLICAS": "2", + }, + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 64 + assert "Refusing delete" in result.stderr + + +@pytest.mark.parametrize( + ("replacement", "message"), + [ + (("PHASE=dry-run", "PHASE=delete"), "delete requires"), + (("ORDER_BY_OBJPATH=false", "ORDER_BY_OBJPATH=yes"), "ORDER_BY_OBJPATH"), + ], +) +def test_renderer_rejects_invalid_configuration(tmp_path, replacement, message): + source = (ROOT / "deploy/kubernetes/example.env").read_text() + config_path = tmp_path / "invalid.env" + config_path.write_text(source.replace(*replacement)) + + 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 message in result.stderr + + +def test_kubernetes_default_antijoin_does_not_globally_sort( + s3gc_module, args_factory, monkeypatch +): + namespace = s3gc_module["do_use"].__globals__ + client = FakeCH() + monkeypatch.setitem(namespace, "args", args_factory(dryrun_flag=True)) + monkeypatch.setitem(namespace, "ch_client", client) + + s3gc_module["do_use"]() + + assert "ORDER BY s3o.objpath" not in client.stream_query + + +def test_antijoin_ordering_is_an_explicit_opt_in(s3gc_module, args_factory, monkeypatch): + namespace = s3gc_module["do_use"].__globals__ + client = FakeCH() + monkeypatch.setitem( + namespace, "args", args_factory(dryrun_flag=True, order_by_objpath=True) + ) + monkeypatch.setitem(namespace, "ch_client", client) + + s3gc_module["do_use"]() + + assert "ORDER BY s3o.objpath" in client.stream_query + + +def test_delete_transport_failure_reconnects_once( + s3gc_module, args_factory, monkeypatch +): + namespace = s3gc_module["remove_objects_reconnecting"].__globals__ + attempts = [] + + class TransportFailingMinio: + def remove_objects(self, bucket, objects): + attempts.append("failed") + raise s3gc_module["urllib3"].exceptions.ReadTimeoutError( + None, "https://s3.example", "timed out" + ) + + class SuccessfulMinio: + def remove_objects(self, bucket, objects): + attempts.append("success") + return iter(()) + + def reconnect(): namespace["minio_client"] = SuccessfulMinio() - module["do_use"]() - - self.assertEqual(len(namespace["ch_client"].inserts), 2) - self.assertEqual(namespace["ch_client"].inserts[0][1], [["object-a", 10, "time", False]]) - self.assertEqual(namespace["ch_client"].inserts[1][1], [["object-b", 20, "time", False]]) - - def test_delete_entrypoint_requires_confirmation(self): - result = subprocess.run( - ["sh", str(ROOT / "kubernetes-entrypoint.sh")], - env={ - **os.environ, - "S3GC_PHASE": "delete", - "S3GC_CLUSTERNAME": "cluster", - "S3GC_EXPECTED_REPLICAS": "2", - }, - capture_output=True, - text=True, - check=False, - ) - self.assertEqual(result.returncode, 64) - self.assertIn("Refusing delete", result.stderr) - - def test_render_rejects_unacknowledged_delete(self): - source = (ROOT / "deploy/kubernetes/example.env").read_text() - with tempfile.NamedTemporaryFile("w", suffix=".env", delete=False) as config: - config.write(source.replace("PHASE=dry-run", "PHASE=delete")) - config_path = config.name - self.addCleanup(lambda: os.unlink(config_path)) - - result = subprocess.run( - [sys.executable, str(ROOT / "deploy/kubernetes/render.py"), config_path], - capture_output=True, - text=True, - check=False, - ) - self.assertEqual(result.returncode, 64) - self.assertIn("delete requires", result.stderr) + monkeypatch.setitem(namespace, "args", args_factory()) + monkeypatch.setitem(namespace, "minio_client", TransportFailingMinio()) + monkeypatch.setitem(namespace, "connect_to_s3", reconnect) -if __name__ == "__main__": - unittest.main() + assert s3gc_module["remove_objects_reconnecting"]([("object-a", 10, "time")]) == [] + assert attempts == ["failed", "success"] From f2defac097350f064ff2ae668f8e0c203aa82c67 Mon Sep 17 00:00:00 2001 From: Diego Nieto Date: Tue, 4 Aug 2026 22:48:00 +0200 Subject: [PATCH 04/16] kubernetes info update --- deploy/kubernetes/README.md | 278 ++++++++-------------------------- deploy/kubernetes/example.env | 2 + deploy/kubernetes/render.py | 8 +- 3 files changed, 72 insertions(+), 216 deletions(-) diff --git a/deploy/kubernetes/README.md b/deploy/kubernetes/README.md index db347b5..3afe5f6 100644 --- a/deploy/kubernetes/README.md +++ b/deploy/kubernetes/README.md @@ -1,236 +1,90 @@ # Kubernetes Job runner -This directory runs `s3gc` as a one-shot Kubernetes Job in the same namespace -as ClickHouse. It uses the in-cluster ClickHouse Service directly, so it does -not depend on a laptop or `kubectl port-forward`. - -The runner has three separate phases: `collect`, `dry-run`, and `delete`. -Run them in that order. Only the delete phase changes S3. - -## Safety rules - -- Use a unique auxiliary-table prefix for each bucket/prefix cleanup. -- Never delete by S3 age or prefix heuristic. Delete only after a successful - cluster-wide dry-run has been reviewed and approved. -- `delete` requires `DELETE_CONFIRMATION=DELETE_ORPHANS`, a target cluster - name, and the expected replica count. -- The delete Job checks the local cluster macro and reachable replica count - before deleting. It records confirmed deletion tombstones after every - `DELETE_BATCH_SIZE` objects. -- Jobs have `backoffLimit: 0`: a failed Job never retries automatically. A - replacement Job with a new name and the same auxiliary table resumes safely. -- The Job has no Kubernetes API token and no in-pod Kubernetes RBAC. - -## 1. Publish and pin the image - -Merge the reviewed code through protected `master`. The publishing workflow -creates a private multi-architecture image for `linux/amd64` and `linux/arm64`. -Copy the immutable digest from the workflow summary: +Run `s3gc` as a one-shot Job in the ClickHouse namespace. For customer and +production work, always run: ```text -altinity/s3gc@sha256: +collect → dry-run → approved delete → verify ``` -Use this digest in deployment configuration. Do not use `latest` or a mutable -tag. - -## 2. Identify the customer target +The Job reaches ClickHouse through its in-cluster Service; no laptop tunnel is +needed. -Use the explicit customer kubeconfig and namespace; do not rely on your default -Kubernetes context. +## Before the first Job -```bash -KUBECONFIG=/secure/customer.kubeconfig \ - kubectl -n get svc -``` +- Use an immutable, multi-architecture image digest: + `altinity/s3gc@sha256:`. +- Create or reuse a namespace-local ServiceAccount and registry pull Secret. +- Create a runtime Secret named by `CREDENTIALS_SECRET`: + - static S3: `S3GC_CHUSER`, `S3GC_CHPASS`, `S3GC_S3ACCESSKEY`, and + `S3GC_S3SECRETKEY`; set `S3USEIAM=false`. + - workload identity: only `S3GC_CHUSER` and `S3GC_CHPASS`; set + `S3USEIAM=true` and use an identity-enabled ServiceAccount. +- Confirm the ClickHouse Service name, cluster macro, expected replica count, + S3 bucket/prefix, and disk name. Use a unique `COLLECTTABLEPREFIX` per + bucket/prefix cleanup. -Before rendering a Job, determine and review: +Never commit credentials, rendered customer manifests, or customer `.env` +files to this repository. -- `CHHOST`: the in-namespace ClickHouse Service name, not `localhost`. -- `CLUSTERNAME`: the exact ClickHouse `cluster` macro. -- `EXPECTED_REPLICAS`: the number of replicas expected to be reachable during - deletion. -- S3 endpoint, port, bucket, prefix, region, TLS setting, and ClickHouse disk - name. -- a unique `COLLECTTABLEPREFIX` for this bucket/prefix pair. - -The ClickHouse user must be able to read `system.remote_data_paths` across the -target cluster and create, insert into, select from, and truncate the auxiliary -table. - -## 3. Create Kubernetes prerequisites - -These resources are namespace-local and intentionally not created by this -repository. - -Create or reuse a dedicated ServiceAccount. Static S3 credentials require no -Kubernetes RBAC: - -```bash -kubectl -n create serviceaccount s3gc -``` +## Local run directory -The published image is private, so create an image-pull Secret: +Keep generated files outside this repository: ```bash -kubectl -n create secret docker-registry altinity-s3gc-pull \ - --docker-server=https://index.docker.io/v1/ \ - --docker-username='' \ - --docker-password='' +export S3GC_RUN_DIR=/path/to/private-s3gc-runs/customer-cluster +mkdir -p "$S3GC_RUN_DIR" +cp deploy/kubernetes/example.env "$S3GC_RUN_DIR/s3gc.env" ``` -### Static S3 credentials - -For customers without workload identity, create the runtime Secret with four -keys. Provide actual values through your approved secret manager or an -interactive terminal; never commit a Secret manifest or `.env` file containing -credentials. - -```bash -kubectl -n create secret generic s3gc-runtime \ - --from-literal=S3GC_CHUSER='' \ - --from-literal=S3GC_CHPASS='' \ - --from-literal=S3GC_S3ACCESSKEY='' \ - --from-literal=S3GC_S3SECRETKEY='' -``` - -Set `S3USEIAM=false` in the deployment config. `s3gc` then uses the access and -secret keys directly and does not attempt workload-identity authentication. - -### Workload identity - -For EKS/IRSA or another workload-identity setup, annotate or reuse the -identity-enabled ServiceAccount, set `S3USEIAM=true`, and provide only -`S3GC_CHUSER` and `S3GC_CHPASS` in the runtime Secret. - -## 4. Create a non-secret deployment config - -Copy the template to a secure location outside the repository: - -```bash -cp deploy/kubernetes/example.env /secure/s3gc-customer.env -``` - -Set the target values. This static-S3 example shows the important fields: - -```dotenv -JOB_NAME=s3gc-customer-collect -NAMESPACE= -IMAGE=altinity/s3gc@sha256: -IMAGE_PULL_SECRET=altinity-s3gc-pull -CREDENTIALS_SECRET=s3gc-runtime -SERVICE_ACCOUNT=s3gc - -CHHOST= -CHPORT=8123 -CLUSTERNAME= -EXPECTED_REPLICAS= -COLLECTTABLEPREFIX=s3gc_customer_20260804_ - -S3IP= -S3PORT=443 -S3BUCKET= -S3PATH= -S3REGION= -S3SECURE_FLAG=true -S3DISKNAME= -S3USEIAM=false - -SAMPLES=4 -DELETE_BATCH_SIZE=1000 -USEAGE_HOURS=24 -ORDER_BY_OBJPATH=false -ACTIVE_DEADLINE_SECONDS=43200 -TTL_SECONDS_AFTER_FINISHED=604800 -MEMORY_REQUEST=1Gi -MEMORY_LIMIT=4Gi -VERBOSE=true -``` - -For a large production cleanup, begin with `SAMPLES=4`, `USEAGE_HOURS=24`, and -a 12-hour deadline. `ORDER_BY_OBJPATH=false` avoids an unnecessary global sort -that can consume substantial ClickHouse memory and CPU. - -## 5. Collect the S3 inventory - -Set these values in the secure config: - -```dotenv -PHASE=collect -DELETE_CONFIRMATION= -JOB_NAME=s3gc-customer-collect -``` - -Render, validate, apply, and follow the Job log: - -```bash -python3 deploy/kubernetes/render.py /secure/s3gc-customer.env \ - > /secure/s3gc-customer-collect.yaml -kubectl apply --dry-run=server -f /secure/s3gc-customer-collect.yaml -kubectl apply -f /secure/s3gc-customer-collect.yaml -kubectl -n logs -f job/s3gc-customer-collect -``` - -The collect phase creates and populates the auxiliary ClickHouse table. It does -not delete S3 objects. - -## 6. Dry-run the cluster-wide anti-join - -Keep every target field and `COLLECTTABLEPREFIX` identical. Change only: - -```dotenv -PHASE=dry-run -DELETE_CONFIRMATION= -JOB_NAME=s3gc-customer-dry-run -``` - -Render and apply a new manifest using the commands above. The final Job log -reports a line such as: - ```text - objects of total size would be removed but for dryrun flag +$S3GC_RUN_DIR/ +├── s3gc.env +├── collect.yaml +├── dry-run.yaml +├── delete.yaml +└── verify.yaml ``` -Review the count and size with the customer. Do not continue based only on a -successful Job exit status. +Fill `s3gc.env` from `example.env`. For production, start with +`SAMPLES=4`, `USEAGE_HOURS=24`, `ORDER_BY_OBJPATH=false`, and a 12-hour +deadline. -## 7. Delete only after explicit approval +## Run each phase -After the dry-run result has been approved, keep all target fields identical -and change only: - -```dotenv -PHASE=delete -DELETE_CONFIRMATION=DELETE_ORPHANS -JOB_NAME=s3gc-customer-delete -``` - -Render, server-dry-run, and apply a new manifest. Follow the logs: +For each phase, update only `PHASE`, `JOB_NAME`, and (for delete) +`DELETE_CONFIRMATION` in `s3gc.env`, then render and apply: ```bash -kubectl -n logs -f job/s3gc-customer-delete -``` - -Before deleting, the Job verifies `CLUSTERNAME` and `EXPECTED_REPLICAS`. During -deletion, logs show durable checkpoints such as: - -```text -delete checkpoint: 1000 objects / removed so far -``` - -If the Job fails, do not re-run collect. Diagnose the failure, then render a -new delete Job name using the same auxiliary-table prefix. Objects already -deleted successfully are tombstoned and are not selected again. - -## 8. Verify and retain evidence - -Run one final dry-run with: - -```dotenv -PHASE=dry-run -DELETE_CONFIRMATION= -JOB_NAME=s3gc-customer-verify +python3 deploy/kubernetes/render.py "$S3GC_RUN_DIR/s3gc.env" \ + > "$S3GC_RUN_DIR/.yaml" +kubectl apply --dry-run=server -f "$S3GC_RUN_DIR/.yaml" +kubectl apply -f "$S3GC_RUN_DIR/.yaml" +kubectl -n logs -f job/ ``` -It must report zero candidates. Retain the auxiliary table and completed Job -logs for audit. Do not reuse that table for another bucket or prefix. +| Phase | Required values | Result | +|---|---|---| +| `collect` | `PHASE=collect` | Lists S3 objects into the auxiliary ClickHouse table. No deletion. | +| `dry-run` | `PHASE=dry-run` | Reports candidates and total size. Review this result. | +| `delete` | `PHASE=delete`, `DELETE_CONFIRMATION=DELETE_ORPHANS` | Checks cluster/replicas, deletes candidates, and checkpoints confirmed progress. | +| `verify` | `PHASE=dry-run` | Must report zero candidates. | + +### Development automation only + +`PHASE=dev-automation` runs `collect → dry-run → delete` in one Job. It always +starts with a fresh auxiliary table and requires +`DELETE_CONFIRMATION=DELETE_ORPHANS`, `CLUSTERNAME`, and `EXPECTED_REPLICAS`. +Any failed stage stops the Job and later stages do not run; successful delete +batches remain checkpointed. Do not use this phase for customer or production +work because it removes the manual dry-run approval gate. + +## Safety + +- Delete checks the local cluster macro and expected replica count before S3 + calls. +- Confirmed deletions are tombstoned in the auxiliary table. If a delete Job + fails, create a new delete Job name with the same table prefix; do **not** + re-collect. +- No Job retries automatically (`backoffLimit: 0`). +- Do not run delete until the customer explicitly approves the dry-run result. diff --git a/deploy/kubernetes/example.env b/deploy/kubernetes/example.env index 23ab36a..3324c30 100644 --- a/deploy/kubernetes/example.env +++ b/deploy/kubernetes/example.env @@ -5,6 +5,8 @@ NAMESPACE=clickhouse IMAGE=altinity/s3gc@sha256:0000000000000000000000000000000000000000000000000000000000000000 IMAGE_PULL_SECRET=altinity-dockerhub-pull PHASE=dry-run +# Development automation only: set PHASE=dev-automation and +# DELETE_CONFIRMATION=DELETE_ORPHANS to run collect, dry-run, and delete in one Job. DELETE_CONFIRMATION= CREDENTIALS_SECRET=s3gc-credentials SERVICE_ACCOUNT=s3gc diff --git a/deploy/kubernetes/render.py b/deploy/kubernetes/render.py index cfc584a..465e4a3 100644 --- a/deploy/kubernetes/render.py +++ b/deploy/kubernetes/render.py @@ -63,11 +63,11 @@ def validate(values: dict[str, str]) -> None: missing = sorted(REQUIRED - values.keys()) if missing: raise ValueError("missing required values: " + ", ".join(missing)) - if values["PHASE"] not in {"collect", "dry-run", "delete"}: - raise ValueError("PHASE must be collect, dry-run, or delete") - if values["PHASE"] == "delete" and values.get("DELETE_CONFIRMATION") != DELETE_CONFIRMATION: + if values["PHASE"] not in {"collect", "dry-run", "delete", "dev-automation"}: + raise ValueError("PHASE must be collect, dry-run, delete, or dev-automation") + if values["PHASE"] in {"delete", "dev-automation"} and values.get("DELETE_CONFIRMATION") != DELETE_CONFIRMATION: raise ValueError( - f"delete requires DELETE_CONFIRMATION={DELETE_CONFIRMATION}" + f"{values['PHASE']} requires DELETE_CONFIRMATION={DELETE_CONFIRMATION}" ) if not JOB_NAME_RE.fullmatch(values["JOB_NAME"]) or len(values["JOB_NAME"]) > 63: raise ValueError("JOB_NAME must be a DNS label of at most 63 characters") From 7f2b55d233b61e4676d9229fcdf1b09bdf5743ac Mon Sep 17 00:00:00 2001 From: Diego Nieto Date: Tue, 4 Aug 2026 22:48:25 +0200 Subject: [PATCH 05/16] Added s3gc:dev-automation-check --- README.md | 3 +- docker/kubernetes-entrypoint.sh | 21 +++++++- tests/test_s3gc.py | 87 +++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index f17250a..34a2b74 100644 --- a/README.md +++ b/README.md @@ -116,7 +116,8 @@ docker run --network="host" -e S3GC_S3PORT=19000 -e S3GC_S3ACCESSKEY=minio99 -e `deploy/kubernetes/` contains a plain-template one-shot Job runner for running the collector inside the ClickHouse namespace. It has separate `collect`, -`dry-run`, and guarded `delete` phases and does not create or contain secrets. +`dry-run`, and guarded `delete` phases, plus a guarded `dev-automation` phase +for non-production testing, and does not create or contain secrets. See [deploy/kubernetes/README.md](deploy/kubernetes/README.md) for the render contract and safety requirements. diff --git a/docker/kubernetes-entrypoint.sh b/docker/kubernetes-entrypoint.sh index 3a3b9ca..27e51d7 100644 --- a/docker/kubernetes-entrypoint.sh +++ b/docker/kubernetes-entrypoint.sh @@ -24,8 +24,27 @@ case "${phase}" in fi set -- --usecollected --keepdata --non-interactive ;; + dev-automation) + if [ "${S3GC_DELETE_CONFIRMATION:-}" != "DELETE_ORPHANS" ]; then + echo "Refusing dev automation: set S3GC_DELETE_CONFIRMATION=DELETE_ORPHANS" >&2 + exit 64 + fi + if [ -z "${S3GC_CLUSTERNAME:-}" ] || [ -z "${S3GC_EXPECTED_REPLICAS:-}" ]; then + echo "Refusing dev automation: S3GC_CLUSTERNAME and S3GC_EXPECTED_REPLICAS are required" >&2 + exit 64 + fi + + # A fresh collection avoids mixing prior runs and their tombstones into an + # automated development run. `set -e` stops subsequent stages on error. + 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 + echo "s3gc dev automation: delete" + exec python /app/s3gc.py --usecollected --keepdata --non-interactive + ;; *) - echo "Invalid S3GC_PHASE=${phase}; use collect, dry-run, or delete" >&2 + echo "Invalid S3GC_PHASE=${phase}; use collect, dry-run, delete, or dev-automation" >&2 exit 64 ;; esac diff --git a/tests/test_s3gc.py b/tests/test_s3gc.py index 2595517..82d479c 100644 --- a/tests/test_s3gc.py +++ b/tests/test_s3gc.py @@ -159,10 +159,77 @@ def test_delete_entrypoint_requires_confirmation(): assert "Refusing delete" in result.stderr +def test_dev_automation_entrypoint_runs_collect_dry_run_and_delete(tmp_path): + calls_path = tmp_path / "calls" + fake_python = tmp_path / "python" + fake_python.write_text( + "#!/bin/sh\n" + "printf '%s\\n' \"$*\" >> \"$CALLS_PATH\"\n" + ) + fake_python.chmod(0o755) + + result = 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": "dev-automation", + "S3GC_DELETE_CONFIRMATION": "DELETE_ORPHANS", + "S3GC_CLUSTERNAME": "cluster", + "S3GC_EXPECTED_REPLICAS": "2", + }, + capture_output=True, + text=True, + check=False, + ) + + 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", + ] + + +def test_dev_automation_entrypoint_stops_after_an_error(tmp_path): + calls_path = tmp_path / "calls" + fake_python = tmp_path / "python" + fake_python.write_text( + "#!/bin/sh\n" + "printf '%s\\n' \"$*\" >> \"$CALLS_PATH\"\n" + "case \"$*\" in *--dry-run) exit 42 ;; esac\n" + ) + fake_python.chmod(0o755) + + result = 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": "dev-automation", + "S3GC_DELETE_CONFIRMATION": "DELETE_ORPHANS", + "S3GC_CLUSTERNAME": "cluster", + "S3GC_EXPECTED_REPLICAS": "2", + }, + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 42 + assert calls_path.read_text().splitlines() == [ + "/app/s3gc.py --collectonly --keepdata --drop-collecttable", + "/app/s3gc.py --usecollected --dry-run", + ] + + @pytest.mark.parametrize( ("replacement", "message"), [ (("PHASE=dry-run", "PHASE=delete"), "delete requires"), + (("PHASE=dry-run", "PHASE=dev-automation"), "dev-automation requires"), (("ORDER_BY_OBJPATH=false", "ORDER_BY_OBJPATH=yes"), "ORDER_BY_OBJPATH"), ], ) @@ -182,6 +249,26 @@ def test_renderer_rejects_invalid_configuration(tmp_path, replacement, message): assert message in result.stderr +def test_renderer_accepts_confirmed_dev_automation(tmp_path): + source = (ROOT / "deploy/kubernetes/example.env").read_text() + config_path = tmp_path / "dev-automation.env" + config_path.write_text( + source.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 + assert 's3gc.altinity.com/phase: "dev-automation"' in result.stdout + + def test_kubernetes_default_antijoin_does_not_globally_sort( s3gc_module, args_factory, monkeypatch ): From e08f099285958a8ac54809e46d149fe63cd51098 Mon Sep 17 00:00:00 2001 From: Diego Nieto Date: Tue, 4 Aug 2026 22:58:36 +0200 Subject: [PATCH 06/16] README updated --- README.md | 261 +++++++++++++++++++++++++++++++++--------------------- 1 file changed, 161 insertions(+), 100 deletions(-) diff --git a/README.md b/README.md index 34a2b74..20e74a1 100644 --- a/README.md +++ b/README.md @@ -1,141 +1,202 @@ # s3gc -Garbage collector for ClickHouse S3 disks -## Repository layout +`s3gc` finds and removes orphaned objects from ClickHouse S3 disks and other +S3-compatible storage. An object is a candidate only when it exists under the +configured bucket/prefix but is absent from ClickHouse +`system.remote_data_paths` for the configured disk. -- `s3gc.py` — collector and deletion logic. -- `docker/` — reproducible Python 3.11 container packaging. -- `deploy/kubernetes/` — generic one-shot Kubernetes Job renderer and operator runbook. -- `tests/` — safety and renderer unit tests. +## How it works -The repository is intentionally public and contains no target-cluster details, -credentials, rendered manifests, or environment configuration. Operators keep -those values outside Git and deploy an image pinned by digest. +1. Collect object names, sizes, and timestamps into an auxiliary ClickHouse + table. +2. Anti-join that inventory with `system.remote_data_paths` (or all replicas of + a configured cluster). +3. Report candidates in dry-run mode, or delete them in batches and record + confirmed deletion checkpoints in the auxiliary table. -## Testing +The command-line script supports these actions directly. For Kubernetes, the +repository supplies a one-shot Job runner that separates collection, review, +and deletion. -Install test-only dependencies with -`uv pip install --python .venv/bin/python -r requirements-dev.txt`, then run -the isolated unit suite with: +## Safety -``` -.venv/bin/python -m pytest -v -``` +Deleting an object is irreversible. Always run and review a dry-run before +deletion, and scope the configured bucket and prefix as narrowly as possible. -Tests marked `dev_cluster` require the dedicated development Kubernetes cluster -and are never run by CI. They must be selected explicitly with -`pytest -m dev_cluster` after reviewing their fixture scope. +- Use a unique collection-table prefix for each cleanup. +- For clustered ClickHouse, use the cluster name and expected replica count. +- A failed delete Job does not automatically retry. Successfully deleted + batches remain checkpointed, so a replacement delete Job can resume safely. +- Never put credentials, customer manifests, or target-cluster details in Git. -## description -The script removes orphaned objects from s3 object storage - Ones that are not mentioned in system.remote_data_paths table +## Requirements -There are two stages: -1. Collecting. - Paths of all objects found in object storage are put in auxiliary ClickHouse table. - It's name is a concatenation of 's3objects_for_' and disk name by default. - Created in the same ClickHouse instance where data from system.remote_data_paths selected -2. Removing. - All objects that exist in s3 and not used according to system.remote_data_paths - are removed from object storage. +- Python 3.11 for local development; the container image also uses Python 3.11. +- Network access to ClickHouse and the target S3-compatible endpoint. +- A ClickHouse user that can read `system.remote_data_paths` and manage the + auxiliary table. +- S3 permissions appropriate to the action: list for collection, plus delete + for deletion. -It is possible to split these stages or do everything at one go. +## Quick start -Besides this, it is possible to calculate objects to remove without actual removing AKA dry run. -If dryrun is set together with usecollected, it uses collected data. -If dryrun is set together with collectonly, error is raised. +Create a local environment and inspect the available options: -It is important to use `--s3diskname` if your disk name is not `s3` which is by default. +```bash +python3.11 -m venv .venv +.venv/bin/python -m pip install -r requirements.txt -r requirements-dev.txt +.venv/bin/python s3gc.py --help +``` -WARNING!: Please use `--dry-run` to check and compare results of what is going to be deleted, just to be on the safe side. +Configuration can be supplied as command-line arguments or `S3GC_*` +environment variables. Set the ClickHouse connection, S3 endpoint/bucket/prefix, +region, disk name, and either static S3 keys or workload identity. Keep secrets +in your approved secret manager or environment, not in command history. -## script invocation -### help -``` -python3 s3gc.py --help +Run a dry-run first: + +```bash +.venv/bin/python s3gc.py --verbose --dry-run ``` -### typical usage -#### all together with dry-run -for https://altinity-clickhouse-data-demo20565656565620663600000001.s3.amazonaws.com/github + +For a production or customer cleanup, use the Kubernetes procedure below rather +than a one-line delete command. + +## Direct script examples + +The following is a non-secret target configuration. Replace every +`` value and do not commit this environment to Git: + +```bash +export S3GC_CHHOST='' +export S3GC_CHPORT=8123 +export S3GC_CHUSER='' +export S3GC_S3IP='s3.eu-central-1.amazonaws.com' +export S3GC_S3PORT=443 +export S3GC_S3BUCKET='' +export S3GC_S3PATH='/' +export S3GC_S3REGION='eu-central-1' +export S3GC_S3SECURE_FLAG=true +export S3GC_S3DISKNAME=s3 +export S3GC_CLUSTERNAME='' +export S3GC_EXPECTED_REPLICAS=2 +export S3GC_COLLECTTABLEPREFIX='s3gc_example_' +export S3GC_AGE=24 +export S3GC_USEAGE=24 ``` -S3GC_S3ACCESSKEY=sdfasfaerasasf \ -S3GC_S3SECRETKEY=werqwsdfqwersdfasf \ -S3GC_S3IP=s3.amazonaws.com \ -S3GC_S3PORT=443 \ -S3GC_S3REGION=us-east-1 \ -S3GC_S3BUCKET=altinity-clickhouse-data-demo20565656565620663600000001 \ -S3GC_S3PATH=github/ \ -S3GC_S3SECURE_FLAG=true \ -python3 ./s3gc.py --verbose --dry-run + +For static S3 credentials, inject the following values from a secret manager +or interactive shell rather than saving them in a file: + +```bash +export S3GC_CHPASS='' +export S3GC_S3ACCESSKEY='' +export S3GC_S3SECRETKEY='' +export S3GC_S3USEIAM=false ``` -#### GCS and object storage that do not support batch delete operations + +For an identity-enabled environment such as EKS/IRSA, do not set static S3 +keys; use the workload identity available to the process instead: + +```bash +export S3GC_CHPASS='' +export S3GC_S3USEIAM=true ``` -S3GC_S3ACCESSKEY=GOOG1xxxxxxxxx \ -S3GC_S3SECRETKEY=xxxxxxxxxxx \ -S3GC_S3IP=storage.googleapis.com \ -S3GC_S3PORT=443 \ -S3GC_S3BUCKET=clickhouse-altinity-main-disk \ -S3GC_S3PATH=chi-main-main-0-0/ \ -S3GC_S3SECURE_FLAG=true \ -S3GC_S3DISKNAME=gcs \ -python3 ./s3gc.py --verbose --use-remove-objects=false + +Run the safe, split workflow directly. Collection makes an auxiliary table; +the second command reads it and reports candidates without deleting objects: + +```bash +.venv/bin/python s3gc.py --collectonly --keepdata +.venv/bin/python s3gc.py --usecollected --dry-run ``` -GCS_HMAC_KEY = S3GC_S3ACCESSKEY -GCS_HMAC_SECRET = S3GC_S3SECRETKEY +The same variables can be passed as flags (for example, +`--ch-host` or `--s3-bucket`). Run `.venv/bin/python s3gc.py --help` for the +complete flag and environment-variable reference. Avoid direct deletion for +customer or production work; use the reviewed Kubernetes workflow instead. +## Container image -#### collect only -``` -S3GC_S3PORT=19000 S3GC_S3ACCESSKEY=minio99 S3GC_S3SECRETKEY=minio123 python3 ./s3gc.py --verbose --collectonly -``` -#### use collected +Build the image locally: + +```bash +docker build -f docker/Dockerfile -t s3gc:local . ``` -S3GC_S3PORT=19000 S3GC_S3ACCESSKEY=minio99 S3GC_S3SECRETKEY=minio123 S3GC_USECOLLECTED=true python3 ./s3gc.py --debug + +For a Kubernetes image, build and publish both supported architectures: + +```bash +docker buildx build --platform linux/amd64,linux/arm64 \ + -f docker/Dockerfile -t /s3gc: --push . ``` -## docker -There is a docker image for the script. +The CI workflow publishes only from trusted pushes to protected `master`; +pull requests run tests but do not receive registry credentials. + +## Kubernetes + +The Kubernetes runner lives in [`deploy/kubernetes/`](deploy/kubernetes/). It +uses a digest-pinned image and external Kubernetes Secrets; it does not create +or store credentials in the repository. -Development, CI, and the image use Python 3.11. With `uv` installed, create the -local environment with `uv venv --python 3.11 .venv`, then install the pinned -requirements with `uv pip install --python .venv/bin/python -r requirements.txt`. +For customer and production work, follow: -### rebuild +```text +collect → dry-run → approved delete → verify ``` -docker buildx build --platform linux/amd64,linux/arm64 -f docker/Dockerfile -t altinity/s3gc . + +The concise operator procedure, Secret requirements, and renderer configuration +are in [deploy/kubernetes/README.md](deploy/kubernetes/README.md). A guarded +`dev-automation` phase is available only for non-production testing; it runs +collect, dry-run, and delete in one Job and still requires an explicit delete +confirmation. + +## Testing + +Run all isolated unit tests: + +```bash +.venv/bin/python -m pytest -v ``` -### usage +Run only the development-automation tests: + +```bash +.venv/bin/python -m pytest -v -k dev_automation ``` -docker run altinity/s3gc --help -docker run --network="host" -e S3GC_S3PORT=19000 -e S3GC_S3ACCESSKEY=minio99 -e S3GC_S3SECRETKEY=minio123 altinity/s3gc + +Validate that the example Kubernetes configuration renders without creating a +cluster resource: + +```bash +python3 deploy/kubernetes/render.py deploy/kubernetes/example.env > /tmp/s3gc-job.yaml +kubectl apply --dry-run=client -f /tmp/s3gc-job.yaml ``` -## Kubernetes +The unit suite does not contact ClickHouse, S3, or Kubernetes. The reserved +`dev_cluster` pytest marker is excluded from CI; any future tests using it must +be selected explicitly with `.venv/bin/python -m pytest -m dev_cluster` after +reviewing their fixture scope. The collect/dry-run/delete exercise is manual +because it can intentionally delete development objects. + +## Repository layout -`deploy/kubernetes/` contains a plain-template one-shot Job runner for running -the collector inside the ClickHouse namespace. It has separate `collect`, -`dry-run`, and guarded `delete` phases, plus a guarded `dev-automation` phase -for non-production testing, and does not create or contain secrets. -See [deploy/kubernetes/README.md](deploy/kubernetes/README.md) for the render -contract and safety requirements. +- `s3gc.py` — collection, anti-join, and deletion logic. +- `docker/` — Python 3.11 container image and Kubernetes entrypoint. +- `deploy/kubernetes/` — plain Job template, renderer, example configuration, + and operator guide. +- `tests/` — pytest safety, renderer, and entrypoint tests. -Images are published only by the GitHub Actions workflow after a trusted push to -protected `master`. Pull requests run tests without registry credentials and do -not publish an image. +## History and roadmap -## changelog +### v0.2 — 2025-01-31 -### v_0.1 Wed Jun 12 2024 +- Added an option to avoid batch deletion for services such as GCS. -- object last modified in auxiliary table -- useage command line parameter - -### v_0.2 Fri Jan 31 2025 -- added option to avoid batch deletion for services like GCS +### v0.1 — 2024-06-12 -## to do list -~~1. option to avoid `remove_objects` which is reportedly not supported by GCE~~ +- Added object last-modified timestamps to the auxiliary table. +- Added the object age option. -- concurrency / async +Planned: concurrency and asynchronous collection/deletion. From 20253ab75a24a699bc55215578f7fb9fb0d6e99c Mon Sep 17 00:00:00 2001 From: Diego Nieto Date: Tue, 4 Aug 2026 23:01:08 +0200 Subject: [PATCH 07/16] README: added IAM role explanation --- README.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/README.md b/README.md index 20e74a1..8486d79 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,25 @@ export S3GC_CHPASS='' export S3GC_S3USEIAM=true ``` +### IAM role support + +With `S3GC_S3USEIAM=true`, `s3gc` uses MinIO's AWS IAM credential provider. +It obtains and refreshes temporary credentials from one of these environments: + +- an EKS Pod using IRSA/workload identity (`AWS_WEB_IDENTITY_TOKEN_FILE` and + `AWS_ROLE_ARN`); +- an EC2 instance with an attached instance profile; or +- an ECS task with task-role credentials. + +Setting `S3GC_S3USEIAM=true` on an ordinary workstation is not enough. The +current provider does **not** read AWS CLI profiles, `aws sso login` state, +`~/.aws/config`, or `AWS_PROFILE`. For a direct local run, use static S3 keys +or run the script from an identity-enabled EC2/EKS/ECS environment. + +The static-key path accepts an access key and secret key only; it does not yet +accept an AWS session token. Therefore, do not copy temporary +`aws sts assume-role` credentials into the static-key variables. + Run the safe, split workflow directly. Collection makes an auxiliary table; the second command reads it and reports candidates without deleting objects: From 61b0ad27754e71a3912642cd0d9987440899a6f6 Mon Sep 17 00:00:00 2001 From: "Diego Nieto (lesandie)" Date: Thu, 6 Aug 2026 11:04:19 +0200 Subject: [PATCH 08/16] Fix boolean env parsing and --age; publish public multi-arch images to GHCR Found while using s3gc to reclaim 164.50 TiB across three customer clusters, including the Kubernetes Job runner's first production run. Boolean options were unusable from the environment. jsonargparse populates action="store_true" flags from env as the RAW STRING, and every non-empty string is truthy, so S3GC_S3USEIAM=false meant *true*: a Job selected the IAM credential provider instead of its static keys and hung indefinitely in the IMDS loop with no error, no exception and no log line, until activeDeadlineSeconds killed it. Thirteen flags shared the defect and four are set by job.yaml.tmpl; S3GC_S3SECURE_FLAG=false would likewise have stayed TLS. Coerce all boolean options once after parsing, reusing the existing strtobool helper, so true/false, yes/no, on/off, 1/0, empty and unset all behave. Bare CLI flags keep working, which the container entrypoint depends on. Retires the --order-by-objpath-flag twin that worked around this for one flag. --age was silently wrong for anything older than a day: timedelta.seconds is the sub-day remainder, so computed age never exceeded 23 h and --age 24 collected nothing, leaving an empty aux table and a dry-run reporting a clean bucket. Also: - --usecollected against a missing/empty aux table raised no error and exited 0, which reads as success. It is what a load-balanced CHHOST produces, since the aux table is node-local. It now fails loudly and says why. - Warn when --samples disagrees with the aux table's PARTITION BY; the mismatch loses partition pruning (measured ~26 min vs ~2 min per sample). - The final tally counted only the current attempt, understating one resumed run by 16.61 TiB. Say "in this attempt" and log the cumulative tombstones. - Detect GCS endpoints and fall back to per-object deletes, since GCS has no batch DeleteObjects. Images now publish publicly to ghcr.io/altinity/s3gc via the automatic GITHUB_TOKEN, matching altinity-mcp and altinity-sql-browser. A private image forced operators to copy a registry credential into the customer's namespace as an imagePullSecret and delete it afterwards; a public one removes that step. IMAGE_PULL_SECRET is now optional and the renderer omits the block when empty. Both architectures stay mandatory: one customer node pool is 5x arm64 + 1x amd64, where an amd64-only image cannot schedule. Documents the per-replica CHHOST requirement, the minimum grant set, the collect sharding recipe, and the per-cluster values that bite when wrong. 30 regression tests cover the boolean matrix, the age filter, the fail-loud path, the samples warning, GCS fallback and the renderer. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/container.yml | 70 +++++++-- README.md | 51 ++++++- deploy/kubernetes/README.md | 52 ++++++- deploy/kubernetes/example.env | 7 +- deploy/kubernetes/render.py | 24 ++- s3gc.py | 136 +++++++++++++++-- tests/test_s3gc.py | 254 ++++++++++++++++++++++++++++++++ 7 files changed, 559 insertions(+), 35 deletions(-) diff --git a/.github/workflows/container.yml b/.github/workflows/container.yml index 97f721e..19c84a9 100644 --- a/.github/workflows/container.yml +++ b/.github/workflows/container.yml @@ -1,13 +1,24 @@ name: Container +# Publishes a PUBLIC multi-architecture image to GHCR. +# +# Public on purpose: a private image forces every operator to copy a registry +# credential into the customer's namespace as an imagePullSecret and remember to +# delete it afterwards. A public image removes that step entirely. +# +# Multi-arch on purpose: ClickHouse node pools are frequently arm64 (one customer +# cluster is 5x arm64 + 1x amd64), and an amd64-only image can only ever schedule +# on a fraction of such a pool. + on: pull_request: push: branches: [master] + tags: ['v*.*.*'] env: - # This repository must be private in the configured registry. - IMAGE_REPOSITORY: altinity/s3gc + REGISTRY: ghcr.io + IMAGE_NAME: altinity/s3gc jobs: test: @@ -19,31 +30,68 @@ jobs: python-version: "3.11" - run: python -m pip install --disable-pip-version-check -r requirements.txt -r requirements-dev.txt - run: pytest -v + # The renderer is part of the contract: it refuses images that are not + # digest-pinned, so a broken template breaks every deployment. - run: python deploy/kubernetes/render.py deploy/kubernetes/example.env > /tmp/s3gc-job.yaml + - name: Rendered manifest must be valid Kubernetes + run: kubectl apply --dry-run=client -f /tmp/s3gc-job.yaml publish: needs: test - if: github.event_name == 'push' && github.ref == 'refs/heads/master' + # Fork pull requests cannot write packages, and there is nothing to publish + # from them anyway. + if: github.event_name == 'push' runs-on: ubuntu-latest permissions: contents: read + packages: write steps: - uses: actions/checkout@v4 + - uses: docker/setup-qemu-action@v3 + - uses: docker/setup-buildx-action@v3 - - uses: docker/login-action@v3 + + - name: Log in to GHCR + uses: docker/login-action@v3 + with: + registry: ${{ env.REGISTRY }} + username: ${{ github.actor }} + # Automatic token: this repository lives in the Altinity organisation, + # so it already grants packages:write for ghcr.io/altinity/*. + password: ${{ secrets.GITHUB_TOKEN }} + + - name: Docker metadata + id: meta + uses: docker/metadata-action@v5 with: - username: ${{ secrets.DOCKERHUB_USERNAME }} - password: ${{ secrets.DOCKERHUB_TOKEN }} - - id: image + images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} + tags: | + type=semver,pattern={{version}} + type=sha,format=long + type=raw,value=latest,enable={{is_default_branch}} + + - name: Build and push + id: image uses: docker/build-push-action@v6 with: context: . file: docker/Dockerfile platforms: linux/amd64,linux/arm64 push: true - tags: | - ${{ env.IMAGE_REPOSITORY }}:sha-${{ github.sha }} - ${{ env.IMAGE_REPOSITORY }}:latest + tags: ${{ steps.meta.outputs.tags }} + labels: ${{ steps.meta.outputs.labels }} + annotations: ${{ steps.meta.outputs.annotations }} + cache-from: type=gha + cache-to: type=gha,mode=max + - name: Record immutable image reference - run: echo "${IMAGE_REPOSITORY}@${{ steps.image.outputs.digest }}" >> "$GITHUB_STEP_SUMMARY" + # render.py requires IMAGE to be digest-pinned, so this is the exact + # string an operator pastes into their .env file. + run: | + { + echo "### Image published" + echo '```' + echo "IMAGE=${REGISTRY}/${IMAGE_NAME}@${{ steps.image.outputs.digest }}" + echo '```' + } >> "$GITHUB_STEP_SUMMARY" diff --git a/README.md b/README.md index 8486d79..9f66820 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,28 @@ export S3GC_CHPASS='' export S3GC_S3USEIAM=true ``` +Every `S3GC_*` boolean accepts `true/false`, `yes/no`, `on/off`, `1/0`, or an +empty value for false. Unset also means false. + +### Collect has no resume — shard large buckets + +A crashed or interrupted `--collectonly` restarts its listing from the +beginning; there is no checkpoint. On a multi-million-object bucket that can +cost hours, and long runs are exactly where a rotating password or a dropped +connection tends to strike. + +Shard the listing by prefix and re-run only the shards that failed. This is safe +to repeat: the auxiliary table is a `ReplacingMergeTree` keyed on `objpath`, so +re-listing a shard is idempotent. + +```bash +# buckets laid out as /<3-char hash>/ +for shard in 0 1 2 3 4 5 6 7 8 9 a b c d e f g h i j k l m n o p q r s t u v w x y z; do + S3GC_S3PATH="/${shard}" ./s3gc.py --collectonly --keepdata || \ + echo "shard ${shard} FAILED — re-run just this one" +done +``` + ### IAM role support With `S3GC_S3USEIAM=true`, `s3gc` uses MinIO's AWS IAM credential provider. @@ -137,21 +159,40 @@ customer or production work; use the reviewed Kubernetes workflow instead. ## Container image -Build the image locally: +Released images are **public** at `ghcr.io/altinity/s3gc`, so Kubernetes needs no +`imagePullSecret`. Always reference them **by digest**, never by tag — tags get +re-pushed and stop reproducing what you tested: + +```bash +docker pull ghcr.io/altinity/s3gc@sha256: +``` + +CI prints the exact `IMAGE=` line in its job summary; paste that into your +`.env`. `render.py` refuses anything not digest-pinned. + +Build locally for a quick check: ```bash docker build -f docker/Dockerfile -t s3gc:local . ``` -For a Kubernetes image, build and publish both supported architectures: +To publish by hand, **both architectures are mandatory** — ClickHouse node pools +are frequently arm64, and an amd64-only image will not schedule there: ```bash docker buildx build --platform linux/amd64,linux/arm64 \ - -f docker/Dockerfile -t /s3gc: --push . + -f docker/Dockerfile -t ghcr.io/altinity/s3gc: --push . +docker buildx imagetools inspect ghcr.io/altinity/s3gc: # expect amd64 AND arm64 ``` -The CI workflow publishes only from trusted pushes to protected `master`; -pull requests run tests but do not receive registry credentials. +> Buildx builder containers cache `/etc/resolv.conf` at creation time. A builder +> left running across a network or VPN change fails with +> `lookup registry-1.docker.io: i/o timeout` while the host resolves fine. +> Recreate the builder, or create one with `--driver-opt network=host`. + +The CI workflow runs tests on every pull request and publishes from pushes to +`master` and version tags, authenticating to GHCR with the automatic +`GITHUB_TOKEN`. ## Kubernetes diff --git a/deploy/kubernetes/README.md b/deploy/kubernetes/README.md index 3afe5f6..23348e7 100644 --- a/deploy/kubernetes/README.md +++ b/deploy/kubernetes/README.md @@ -7,23 +7,67 @@ production work, always run: collect → dry-run → approved delete → verify ``` -The Job reaches ClickHouse through its in-cluster Service; no laptop tunnel is +The Job reaches ClickHouse through an in-cluster Service; no laptop tunnel is needed. +> **`CHHOST` must be a PER-REPLICA Service, never the load-balanced one.** +> The auxiliary table is a node-local `ReplacingMergeTree`, not a Replicated +> table. A load-balanced Service round-robins, so `collect` can write the table +> on one replica while `dry-run`/`delete` land on another and find nothing. +> Use `chi---0-0` (replica 0), not `clickhouse-`, and use +> the **same** host for every phase of a cleanup. + ## Before the first Job - Use an immutable, multi-architecture image digest: - `altinity/s3gc@sha256:`. -- Create or reuse a namespace-local ServiceAccount and registry pull Secret. + `ghcr.io/altinity/s3gc@sha256:`. CI prints the exact `IMAGE=` line in + its job summary. Pin by digest, never by tag — tags get re-pushed. +- **No pull Secret is required**: the image is public. Leave + `IMAGE_PULL_SECRET` empty and the renderer omits the `imagePullSecrets` block. + Set it only when pulling from a private mirror. +- The image must be multi-arch. ClickHouse node pools are often arm64 (one + customer cluster is 5x arm64 + 1x amd64), and an amd64-only image simply will + not schedule there. +- Create or reuse a namespace-local ServiceAccount. - Create a runtime Secret named by `CREDENTIALS_SECRET`: - static S3: `S3GC_CHUSER`, `S3GC_CHPASS`, `S3GC_S3ACCESSKEY`, and `S3GC_S3SECRETKEY`; set `S3USEIAM=false`. - workload identity: only `S3GC_CHUSER` and `S3GC_CHPASS`; set - `S3USEIAM=true` and use an identity-enabled ServiceAccount. + `S3USEIAM=true` and use an identity-enabled ServiceAccount. Note the + template sets `automountServiceAccountToken: false`, so IAM mode also needs + a ServiceAccount that actually projects a token. - Confirm the ClickHouse Service name, cluster macro, expected replica count, S3 bucket/prefix, and disk name. Use a unique `COLLECTTABLEPREFIX` per bucket/prefix cleanup. +### Minimum ClickHouse grants + +```sql +GRANT SELECT ON system.* TO s3gc; -- remote_data_paths, one, disks, tables +GRANT SELECT, INSERT, CREATE TABLE ON .* TO s3gc; -- the auxiliary table +GRANT REMOTE ON *.* TO s3gc; -- clusterAllReplicas() +``` + +`S3 ON *.*` is **not** needed — s3gc lists the bucket with its own client, not +the `s3()` table function. `TRUNCATE` is only used when `--keepdata` is absent. +A user cannot self-grant `REMOTE`; grant option only passes on privileges it +already holds. + +### Values that vary per cluster, and bite when wrong + +- **`S3PATH` may legitimately be empty** — some buckets keep blobs at the root. + A wrong prefix silently lists nothing and reports a clean bucket. +- **The disk is not always called `s3`** — GCS-backed clusters commonly use + `gcs`. `S3DISKNAME` sets both the anti-join scope and the aux table name, so + the wrong value makes *every* blob look orphaned. +- **A `*_cache` disk is a filesystem cache over the same blobs**, not a second + reference scope; scope the anti-join to the underlying object disk. +- **`SAMPLES` must match the value used at collect time** — the aux table is + `PARTITION BY CRC32(objpath) % SAMPLES`, and a mismatch loses partition + pruning (measured: ~2 min vs ~26 min per sample). s3gc now warns on mismatch. +- **GCS has no batch delete** — s3gc detects a `storage.googleapis.com` endpoint + and falls back to one request per object, which is markedly slower. + Never commit credentials, rendered customer manifests, or customer `.env` files to this repository. diff --git a/deploy/kubernetes/example.env b/deploy/kubernetes/example.env index 3324c30..37f07ab 100644 --- a/deploy/kubernetes/example.env +++ b/deploy/kubernetes/example.env @@ -2,8 +2,11 @@ # Credentials must be supplied separately by the named Kubernetes Secret. JOB_NAME=s3gc-example-dry-run NAMESPACE=clickhouse -IMAGE=altinity/s3gc@sha256:0000000000000000000000000000000000000000000000000000000000000000 -IMAGE_PULL_SECRET=altinity-dockerhub-pull +# Always digest-pinned; CI prints the exact line in its job summary. +IMAGE=ghcr.io/altinity/s3gc@sha256:0000000000000000000000000000000000000000000000000000000000000000 +# The published image is PUBLIC, so leave this empty: the renderer then omits +# the imagePullSecrets block entirely. Set it only for a private mirror. +IMAGE_PULL_SECRET= PHASE=dry-run # Development automation only: set PHASE=dev-automation and # DELETE_CONFIRMATION=DELETE_ORPHANS to run collect, dry-run, and delete in one Job. diff --git a/deploy/kubernetes/render.py b/deploy/kubernetes/render.py index 465e4a3..6be8241 100644 --- a/deploy/kubernetes/render.py +++ b/deploy/kubernetes/render.py @@ -89,6 +89,27 @@ def validate(values: dict[str, str]) -> None: raise ValueError("ORDER_BY_OBJPATH must be true or false") +def drop_empty_image_pull_secret(manifest: str) -> str: + """Remove the imagePullSecrets block when no secret was configured. + + The published image is public, so most deployments need no pull secret at + all — and rendering `- name: ""` would be both meaningless and rejected. + string.Template has no conditionals, so this is done after substitution. + """ + lines = manifest.splitlines(keepends=True) + out = [] + index = 0 + while index < len(lines): + if lines[index].strip() == "imagePullSecrets:" and index + 1 < len(lines): + following = lines[index + 1].strip() + if following in ('- name: ""', "- name: ''", "- name:"): + index += 2 + continue + out.append(lines[index]) + index += 1 + return "".join(out) + + def main() -> int: if len(sys.argv) != 2: print(f"usage: {Path(sys.argv[0]).name} CONFIG.env", file=sys.stderr) @@ -96,7 +117,8 @@ def main() -> int: try: values = read_values(Path(sys.argv[1])) validate(values) - sys.stdout.write(Template(TEMPLATE.read_text()).substitute(values)) + rendered = Template(TEMPLATE.read_text()).substitute(values) + sys.stdout.write(drop_empty_image_pull_secret(rendered)) except (OSError, ValueError, KeyError) as exc: print(f"render error: {exc}", file=sys.stderr) return 64 diff --git a/s3gc.py b/s3gc.py index b808655..56723c6 100644 --- a/s3gc.py +++ b/s3gc.py @@ -49,6 +49,23 @@ def strtobool(value): return 0 raise ValueError(f"invalid truth value {value!r}") + +def coerce_bool(value): + """Normalise anything an option may arrive as into a real bool. + + Flags declared with action="store_true" are set to a real bool on the command + line, but jsonargparse populates them from the environment as the RAW STRING. + Every non-empty string is truthy in Python, so S3GC_DRYRUN_FLAG=false used to + mean *true*. Treat unset/empty as false and parse the usual spellings. + """ + if isinstance(value, bool): + return value + if value is None or value == "": + return False + if isinstance(value, (int, float)): + return bool(value) + return bool(strtobool(str(value))) + parser = ArgumentParser( usage=usage, env_prefix="S3GC", default_env=True, exit_on_error=False ) @@ -324,13 +341,6 @@ def strtobool(value): default=False, help="Order anti-join output by object path (costly for large Kubernetes Jobs)", ) -parser.add_argument( - "--order-by-objpath-flag", - dest="order_by_objpath", - type=bool, - default=False, - help="Order anti-join output by object path (costly for large Kubernetes Jobs)", -) parser.add_argument( "--s3-connect-timeout", dest="s3_connect_timeout", @@ -398,7 +408,7 @@ def strtobool(value): "--useremoveobjects", "--use-remove-objects", dest="use_remove_objects", - type=bool, + type=coerce_bool, default=True, help="use remove_objects (not supported by GCE). Set it to false to use remove_object", ) @@ -475,6 +485,41 @@ def strtobool(value): args = parser.parse_args() +# Every flag declared with action="store_true" arrives from the environment as a +# raw string, and every non-empty string is truthy — so S3GC_S3USEIAM=false used +# to select the IAM credential provider and hang a Kubernetes Job indefinitely. +# Normalise all boolean options in one place, immediately after parsing, so the +# rest of the program can rely on real bools. +BOOLEAN_DESTS = ( + "s3secure_flag", + "s3useiam", + "use_remove_objects", + "keepdata_flag", + "collectonly_flag", + "usecollected_flag", + "dryrun_flag", + "order_by_objpath", + "createdatabase_flag", + "drop_collecttable_flag", + "verbose_flag", + "debug_flag", + "silent_flag", + "listoptions", +) + +for _dest in BOOLEAN_DESTS: + if not hasattr(args, _dest): + continue + _raw = getattr(args, _dest) + try: + setattr(args, _dest, coerce_bool(_raw)) + except ValueError: + parser.error( + f"invalid boolean value {_raw!r} for {_dest} " + f"(environment variable S3GC_{_dest.upper()}); " + "use one of true/false, yes/no, on/off, 1/0" + ) + if args.listoptions: with redirect_stdout(StringIO()) as f: try: @@ -631,6 +676,17 @@ def connect_to_s3(): f"Connecting to S3, host:port={args.s3ip}:{args.s3port}, authentication={authentication}, " f"secure={args.s3secure_flag}, region={args.s3region}" ) + + # Google Cloud Storage's S3-compatible API has no batch DeleteObjects, so + # remove_objects() fails there. Switch to the per-object path automatically + # rather than letting every delete fail at run time. + if "storage.googleapis.com" in args.s3ip and args.use_remove_objects: + logger.warning( + "GCS endpoint detected: batch remove_objects is not supported there, " + "falling back to per-object remove_object. This is markedly slower " + "(one request per object); pass --use-remove-objects false to silence this." + ) + args.use_remove_objects = False global minio_client connection_options = { "secure": args.s3secure_flag, @@ -723,7 +779,10 @@ def do_collect(): try: obj = next(objects) delta = datetime.datetime.now(datetime.timezone.utc) - obj.last_modified - hours = int(delta.seconds / 3600) + # total_seconds(), not .seconds: the latter is the sub-day + # remainder (0..86399), so any object older than a day reported + # at most 23 hours and --age 24 collected nothing at all. + hours = int(delta.total_seconds() // 3600) if hours >= args.age: objs.append([obj.object_name, obj.size, obj.last_modified, True]) total_size += obj.size @@ -747,6 +806,34 @@ def do_collect(): ) +def check_samples_match_partitioning(): + """Warn when --samples disagrees with the aux table's PARTITION BY. + + The table is created as PARTITION BY CRC32(objpath) % at COLLECT + time. Running the use phase with a different --samples silently loses + partition pruning: on one production cluster the matching case scanned a + sample in ~2 min where the mismatching case took ~26 min. + """ + try: + rows = ch_client.query( + "SELECT partition_key FROM system.tables " + f"WHERE database = currentDatabase() AND name = '{tname.strip('`').split('.')[-1]}'" + ).result_rows + except Exception as exc: + logger.debug(f"could not read partition_key for {tname}: {exc}") + return + if not rows or not rows[0][0]: + return + partition_key = rows[0][0] + expected = f"% {args.samples}" + if "CRC32" in partition_key and expected not in partition_key.replace(" ", " "): + logger.warning( + f"--samples {args.samples} does not match the auxiliary table's " + f"partitioning ({partition_key}). Partition pruning will be lost; " + "use the same --samples value that the collect phase used." + ) + + def do_use(): if not args.dryrun_flag: preflight_cluster() @@ -765,9 +852,18 @@ def do_use(): logger.info(f"exception selecting from {tname}, {exc}") pass if num_rows == 0: - logger.info(f"auxiliary table {tname} does not exist or empty, nothing to do") + # Exiting 0 here reads as success, but with --usecollected an absent or + # empty auxiliary table means the collect never ran, ran against another + # host, or was truncated. The table is a NODE-LOCAL ReplacingMergeTree, so + # a load-balanced ClickHouse Service can collect on one replica and land + # here on the other. Fail loudly instead of reporting a clean bucket. + raise RuntimeError( + f"auxiliary table {tname} does not exist or is empty on {args.chhost}. " + "Run the collect phase first, and make sure every phase targets the SAME " + "replica: the table is node-local, so a load-balanced Service will not do." + ) - graceful_exit() + check_samples_match_partitioning() def make_antijoin(calc_only=False, sample=None): after_condition = f"AND s3o.objpath > {args.useafter} " if args.useafter else "" @@ -888,9 +984,25 @@ def make_antijoin(calc_only=False, sample=None): f"{len(errors)} S3 deletion error(s); successful deletes were checkpointed" ) + # "this attempt", not "this run": a resumed run leaves earlier attempts' + # deletions out of these counters, so the line understated one aps1 run by + # 16.61 TiB. The cumulative truth is the tombstone count in the aux table. logger.info( - f"{num_removed} objects of total size {total_size} {'are removed' if not args.dryrun_flag else 'would be removed but for dryrun flag'}" + f"{num_removed} objects of total size {total_size} " + f"{'are removed' if not args.dryrun_flag else 'would be removed but for dryrun flag'} " + "in this attempt" ) + if not args.dryrun_flag: + try: + cumulative = ch_client.query( + f"SELECT count(), sum(size) FROM {tname} FINAL WHERE active = false" + ).result_rows[0] + logger.info( + f"cumulative for this auxiliary table: {cumulative[0]} objects / " + f"{cumulative[1]} bytes tombstoned" + ) + except Exception as exc: # never fail a completed run over a status query + logger.info(f"could not read cumulative tombstone count: {exc}") if not args.keepdata_flag and not args.dryrun_flag: logger.info(f"truncating {tname}") diff --git a/tests/test_s3gc.py b/tests/test_s3gc.py index 82d479c..61d5edc 100644 --- a/tests/test_s3gc.py +++ b/tests/test_s3gc.py @@ -322,3 +322,257 @@ def reconnect(): assert s3gc_module["remove_objects_reconnecting"]([("object-a", 10, "time")]) == [] assert attempts == ["failed", "success"] + + +# --------------------------------------------------------------------------- +# Regression tests for defects found in production (SUP-30408). +# --------------------------------------------------------------------------- + + +def _load_with_env(monkeypatch, **env): + """Load s3gc.py with the given S3GC_* environment, as a Kubernetes Job would.""" + import runpy + + for key, value in env.items(): + monkeypatch.setenv(key, value) + monkeypatch.setattr(sys, "argv", [str(Path(__file__).resolve().parents[1] / "s3gc.py")]) + return runpy.run_path( + str(Path(__file__).resolve().parents[1] / "s3gc.py"), run_name="s3gc_test" + ) + + +@pytest.mark.parametrize( + "value, expected", + [ + ("false", False), + ("False", False), + ("no", False), + ("off", False), + ("0", False), + ("", False), + ("true", True), + ("True", True), + ("yes", True), + ("on", True), + ("1", True), + ], +) +def test_coerce_bool_parses_env_spellings(s3gc_module, value, expected): + assert s3gc_module["coerce_bool"](value) is expected + + +def test_coerce_bool_passes_through_real_bools_and_none(s3gc_module): + assert s3gc_module["coerce_bool"](True) is True + assert s3gc_module["coerce_bool"](False) is False + assert s3gc_module["coerce_bool"](None) is False + + +def test_coerce_bool_rejects_nonsense(s3gc_module): + with pytest.raises(ValueError): + s3gc_module["coerce_bool"]("maybe") + + +@pytest.mark.parametrize( + "dest, env_name", + [ + ("s3useiam", "S3GC_S3USEIAM"), + ("s3secure_flag", "S3GC_S3SECURE_FLAG"), + ("dryrun_flag", "S3GC_DRYRUN_FLAG"), + ("keepdata_flag", "S3GC_KEEPDATA_FLAG"), + ("order_by_objpath", "S3GC_ORDER_BY_OBJPATH"), + ("verbose_flag", "S3GC_VERBOSE_FLAG"), + ], +) +def test_boolean_env_false_is_false(monkeypatch, dest, env_name): + """S3GC_*=false used to be the truthy string 'false'. + + S3GC_S3USEIAM=false selected the IAM credential provider and hung a + Kubernetes Job indefinitely with no error, no exception and no log line. + """ + module = _load_with_env(monkeypatch, **{env_name: "false"}) + assert getattr(module["args"], dest) is False + + +@pytest.mark.parametrize("value", ["true", "1", "yes"]) +def test_boolean_env_true_is_true(monkeypatch, value): + module = _load_with_env(monkeypatch, S3GC_S3USEIAM=value) + assert module["args"].s3useiam is True + + +def test_boolean_env_zero_is_false(monkeypatch): + """'0' must not be truthy either, and must not raise (type=bool would).""" + module = _load_with_env(monkeypatch, S3GC_S3USEIAM="0") + assert module["args"].s3useiam is False + + +def test_bare_cli_flag_still_enables(monkeypatch): + """Coercion must not break `--dryrun` used as a bare flag.""" + import runpy + + root = Path(__file__).resolve().parents[1] + monkeypatch.setattr(sys, "argv", [str(root / "s3gc.py"), "--dryrun"]) + module = runpy.run_path(str(root / "s3gc.py"), run_name="s3gc_test") + assert module["args"].dryrun_flag is True + + +def test_collect_age_filter_uses_total_seconds(s3gc_module, args_factory, monkeypatch): + """--age 24 must keep a 30-day-old object. + + The filter used timedelta.seconds (the sub-day remainder, 0..86399), so + computed age never exceeded 23 h and --age 24 collected nothing at all, + leaving an empty aux table and a dry-run that reported a clean bucket. + """ + import datetime + + namespace = s3gc_module["do_collect"].__globals__ + now = datetime.datetime.now(datetime.timezone.utc) + + class Obj: + def __init__(self, name, age): + self.object_name = name + self.size = 1 + self.last_modified = now - age + + old = Obj("thirty-days-old", datetime.timedelta(days=30, hours=5)) + fresh = Obj("one-hour-old", datetime.timedelta(hours=1)) + + class Minio: + def list_objects(self, bucket, prefix, recursive, start_after): + return iter([old, fresh]) + + class CH: + def __init__(self): + self.rows = [] + + def command(self, query): + return None + + def insert(self, table, rows, column_names): + self.rows.extend(rows) + + ch = CH() + monkeypatch.setitem( + namespace, + "args", + args_factory( + age=24, + collectbatchsize=10, + total=None, + collectafter="", + s3path="", + s3bucket="bucket", + createdatabase_flag=False, + drop_collecttable_flag=False, + ), + ) + monkeypatch.setitem(namespace, "minio_client", Minio()) + monkeypatch.setitem(namespace, "ch_client", ch) + monkeypatch.setitem(namespace, "tname", "`aux`") + + s3gc_module["do_collect"]() + + collected = [row[0] for row in ch.rows] + assert "thirty-days-old" in collected + assert "one-hour-old" not in collected + + +def test_usecollected_without_aux_table_fails_loudly( + s3gc_module, args_factory, monkeypatch +): + """An absent/empty aux table used to exit 0 — indistinguishable from success. + + That is exactly what a load-balanced CHHOST produces, because the aux table + is a node-local ReplacingMergeTree. + """ + namespace = s3gc_module["do_use"].__globals__ + + class EmptyCH(FakeCH): + def command(self, query): + return 0 + + monkeypatch.setitem(namespace, "args", args_factory(dryrun_flag=True, chhost="replica-1")) + monkeypatch.setitem(namespace, "ch_client", EmptyCH()) + monkeypatch.setitem(namespace, "tname", "`aux`") + + with pytest.raises(RuntimeError, match="does not exist or is empty"): + s3gc_module["do_use"]() + + +def test_samples_mismatch_warns(s3gc_module, args_factory, monkeypatch, caplog): + """--samples must match the aux table's PARTITION BY or pruning is lost.""" + namespace = s3gc_module["check_samples_match_partitioning"].__globals__ + + class PartitionedCH: + def query(self, query): + return QueryResult("CRC32(objpath) % 4") + + monkeypatch.setitem(namespace, "args", args_factory(samples=3)) + monkeypatch.setitem(namespace, "ch_client", PartitionedCH()) + monkeypatch.setitem(namespace, "tname", "`aux`") + + with caplog.at_level("WARNING"): + s3gc_module["check_samples_match_partitioning"]() + + assert "does not match" in caplog.text + + +def test_gcs_endpoint_disables_batch_delete(s3gc_module, args_factory, monkeypatch): + """GCS has no batch DeleteObjects; remove_objects() fails there.""" + namespace = s3gc_module["connect_to_s3"].__globals__ + parsed = args_factory( + s3ip="storage.googleapis.com", + s3port=443, + use_remove_objects=True, + s3useiam=False, + s3secure_flag=True, + s3accesskey="k", + s3secretkey="s", + s3region="auto", + s3sslcertfile="", + s3_connect_timeout=15, + s3_read_timeout=120, + s3_retries=3, + ) + monkeypatch.setitem(namespace, "args", parsed) + monkeypatch.setitem(namespace, "Minio", lambda *a, **k: object()) + + s3gc_module["connect_to_s3"]() + + assert parsed.use_remove_objects is False + + +def test_renderer_omits_empty_image_pull_secret(tmp_path): + """A public image needs no pull secret; `- name: ""` would be meaningless.""" + config_path = tmp_path / "public.env" + config_path.write_text((ROOT / "deploy/kubernetes/example.env").read_text()) + + result = subprocess.run( + [sys.executable, str(ROOT / "deploy/kubernetes/render.py"), config_path], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0 + assert "imagePullSecrets" not in result.stdout + assert 'name: ""' not in result.stdout + + +def test_renderer_keeps_configured_image_pull_secret(tmp_path): + """A private mirror must still be able to set one.""" + source = (ROOT / "deploy/kubernetes/example.env").read_text() + config_path = tmp_path / "private.env" + config_path.write_text( + source.replace("IMAGE_PULL_SECRET=", "IMAGE_PULL_SECRET=my-mirror-pull") + ) + + result = subprocess.run( + [sys.executable, str(ROOT / "deploy/kubernetes/render.py"), config_path], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0 + assert "imagePullSecrets:" in result.stdout + assert "- name: \"my-mirror-pull\"" in result.stdout From 7e3fa783307798138ba0d8bdef58cc89d2429358 Mon Sep 17 00:00:00 2001 From: "Diego Nieto (lesandie)" Date: Thu, 6 Aug 2026 11:08:00 +0200 Subject: [PATCH 09/16] Added AGENTS and CLAUDE instructions --- AGENTS.md | 33 +++++++++++++++++++++ CLAUDE.md | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100644 AGENTS.md create mode 100644 CLAUDE.md diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..cf755ee --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,33 @@ +# Codex guide — s3gc + +Read `CLAUDE.md` before making substantive changes in this repository. +`CLAUDE.md` is the full contributor guide and the primary source of truth. + +This file is intentionally short so agent tooling can find the critical rules +quickly, then defer to `CLAUDE.md` for complete repository guidance. + +## Critical rules + +1. **Read `CLAUDE.md` first.** Treat it as required repository context, not + optional background reading. +2. **Protect destructive-operation safeguards.** `s3gc` deletes orphaned S3 + objects only after collection, dry-run review, explicit confirmation, and + the applicable ClickHouse cluster/replica preflight. Do not weaken these + controls without explicit approval and matching tests and documentation. +3. **Required checks are offline.** Run the relevant pytest suite and + Kubernetes renderer/manifest dry-run checks described in `CLAUDE.md`. + Automated tests must not contact live ClickHouse or object storage, or + delete objects. +4. **No secrets or customer data in Git.** Never commit credentials, customer + configuration, target-cluster details, or rendered customer manifests. Use + Kubernetes Secrets or workload identity for production credentials. +5. **Keep deployments immutable and least-privileged.** Preserve digest-pinned + images, non-root/read-only container settings, and renderer validation. +6. **Keep dependencies deliberate.** Production dependencies belong in + `requirements.txt`; testing-only dependencies belong in + `requirements-dev.txt`. Do not add either casually. + +## Working rule + +When `AGENTS.md` and `CLAUDE.md` differ, update them to match, but follow the +more complete guidance in `CLAUDE.md` for the current task. diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..3c1e494 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,87 @@ +# Contributor guide — s3gc + +`s3gc` is a Python command-line tool and Kubernetes Job workflow for finding +and deleting orphaned objects from ClickHouse S3 disks and compatible object +storage. It is an operationally destructive tool: quality is held by offline +tests, explicit delete controls, and conservative deployment defaults. + +## Hard rules + +1. **The deletion lifecycle is non-negotiable.** The normal production flow is + `collect → dry-run → explicit customer approval → delete → verify`. Preserve + the delete confirmation token, ClickHouse cluster and expected-replica + preflight, deletion checkpoints, and the rule that a failed delete job does + not retry automatically. A behavior change in this path needs regression + tests and matching README/deployment documentation. + +2. **Tests stay offline by default.** Install both requirements files, then + run `pytest -v` for relevant changes. Tests must use fakes, local + subprocesses, and manifest dry-runs; they must not contact live ClickHouse, + S3-compatible storage, or delete objects. The `dev_cluster` marker is + explicitly environment-dependent and never runs in CI. Do not add live + credentials or a live-delete test path to the default suite. + +3. **No secrets or customer artifacts in Git.** Do not commit S3 keys, + ClickHouse passwords, customer `.env` files, rendered customer manifests, + target-cluster details, or command output containing them. Keep credentials + in an approved secret manager, Kubernetes Secret, or workload identity. + `deploy/kubernetes/example.env` is a non-secret template only. + +4. **Kubernetes deployment stays immutable and least-privileged.** + `deploy/kubernetes/render.py` must continue to reject unpinned images and + invalid phase/confirmation input. Use image digests, never mutable tags. + Preserve the Job template's non-root user and read-only root filesystem; + do not embed credentials in the image or manifest. + +5. **Dependencies are deliberate and reproducible.** Runtime dependencies are + pinned in `requirements.txt`; test-only dependencies are pinned in + `requirements-dev.txt`. Use both files when preparing a development or CI + environment. Add or update a dependency only when it is necessary for the + requested capability, and test the resulting workflow. + +## Repository map + +| Path | Purpose | +| --- | --- | +| `s3gc.py` | CLI arguments, ClickHouse inventory/anti-join, S3 collection and deletion, safety preflight, and checkpoints. | +| `tests/` | Pytest regression tests using fakes and local subprocesses. | +| `docker/Dockerfile` | Minimal Python 3.11 production image. | +| `docker/kubernetes-entrypoint.sh` | Phase dispatcher and delete/dev-automation confirmation gate. | +| `deploy/kubernetes/render.py` | Validates a non-secret environment file and renders the Job manifest. | +| `deploy/kubernetes/job.yaml.tmpl` | Kubernetes Job template with security context and environment wiring. | +| `deploy/kubernetes/example.env` | Non-secret rendering example; copy it outside the repository for a real run. | +| `.github/workflows/container.yml` | CI test, render, manifest validation, and container publication workflow. | + +## Required checks + +For a change to Python, shell, renderer, manifest, dependencies, or deployment +workflow, run the relevant checks after installing the pinned development +requirements: + +```bash +python3.11 -m venv .venv +.venv/bin/python -m pip install -r requirements.txt -r requirements-dev.txt +.venv/bin/python -m pytest -v +.venv/bin/python deploy/kubernetes/render.py deploy/kubernetes/example.env > /tmp/s3gc-job.yaml +kubectl apply --dry-run=client -f /tmp/s3gc-job.yaml +``` + +Add a regression test in the same change as each behavior or safety fix. Cover +failure paths as well as the intended path, especially delete confirmation, +cluster/replica preflight, S3 delete errors, checkpointing, boolean environment +parsing, and renderer input validation. Do not claim or impose a coverage +percentage until coverage tooling and an enforceable threshold are introduced. + +## Working discipline + +- Keep the command-line and `S3GC_*` environment interfaces compatible unless + the task explicitly authorizes a breaking operational change. +- Treat the renderer, entrypoint, README, and Kubernetes guide as part of the + same operator-facing contract. Update the affected documentation in the same + change as an operational behavior change. +- Surface out-of-scope safety defects rather than silently changing them. State + the file and risk, and defer the fix unless it is necessary to keep the + current task safe. +- When using subagents for discovery or review, make them read-only unless the + task explicitly authorizes writes. Inspect the working tree after any agent + batch before continuing. From 0884ddbdf98ab0d7e54209de192aafdc531453c8 Mon Sep 17 00:00:00 2001 From: "Diego Nieto (lesandie)" Date: Thu, 6 Aug 2026 11:13:08 +0200 Subject: [PATCH 10/16] Add CHANGELOG.md recording what changed and why MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Development history was only reconstructable from commit messages and a customer support ticket, which mixes two audiences: the ticket tracks a customer engagement, while this repository needs the engineering history. CHANGELOG.md separates them. It records the reasoning and the evidence behind defects found by running the tool against real clusters — the measurements are the expensive part to reconstruct, and without them a later reader cannot tell a deliberate design decision from an accident. Per CLAUDE.md rule 3 it carries no customer names, cluster identifiers or credentials; findings are described in terms of the behaviour they expose. Documents the unreleased work on this branch (boolean environment parsing, --age, fail-loud on a missing auxiliary table, GCS fallback, samples warning, cumulative tally, public multi-arch GHCR images, optional pull secret) plus the known gaps: collect still has no resume, and the GHCR package must be marked public once after the first publish. README's history section now points here; CLAUDE.md lists the file in the repository map and asks for it to be updated alongside behaviour, safety and deployment changes. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 142 +++++++++++++++++++++++++++++++++++++++++++++++++++ CLAUDE.md | 5 ++ README.md | 13 ++--- 3 files changed, 151 insertions(+), 9 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..7c98864 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,142 @@ +# Changelog + +Notable changes to `s3gc`, newest first. + +This file records **what changed and why**, with enough context that someone +picking the repository up later — human or agent — can tell a deliberate design +decision from an accident. Defects found by running the tool against real +clusters carry their evidence, because the reasoning is usually the expensive +part to reconstruct. + +Format loosely follows [Keep a Changelog](https://keepachangelog.com/). +Customer names, cluster identifiers and credentials never appear here (see +`CLAUDE.md`, rule 3); findings are described in terms of the behaviour they +expose. + +## [Unreleased] + +Changes below are on `feature/kubernetes-job-runner` and not yet released. + +### Fixed + +- **Boolean options were unusable from the environment, and one of them hung a + production Job indefinitely.** Flags declared with `action="store_true"` are + populated by `jsonargparse` from the environment as the **raw string**, and + every non-empty string is truthy in Python — so `S3GC_S3USEIAM=false` meant + *true*. The affected Job selected `IamAwsProvider()` instead of the static + keys in its Secret and wedged in the IMDS credential loop: no error, no + exception, no log line, zero rows after five minutes, no `:443` connection + ever opened, 0.34 s of CPU — only `activeDeadlineSeconds` ended it. Removing + the variable made the same image, Secret and manifest work immediately at + ~3,500 objects/s. + + Thirteen flags shared the defect and four are set by `job.yaml.tmpl` + (`S3GC_S3USEIAM`, `S3GC_S3SECURE_FLAG`, `S3GC_ORDER_BY_OBJPATH`, + `S3GC_VERBOSE_FLAG`); two were correct only because `"true"` happens to be + truthy, and `S3GC_S3SECURE_FLAG=false` would have silently stayed on TLS. + + *Why not `type=bool`:* it raises on `0`, `1` and empty values, and it would + force bare flags such as `--collectonly` to take an argument, which + `docker/kubernetes-entrypoint.sh` and every documented invocation rely on. + Instead all boolean options are coerced once after parsing through the + existing `strtobool` helper, accepting `true/false`, `yes/no`, `on/off`, + `1/0`, empty and unset. The `--order-by-objpath-flag` twin argument, a + previous one-off workaround for this same defect, is retired. + +- **`--age` silently collected nothing for anything older than a day.** The + filter used `timedelta.seconds`, the sub-day remainder (0..86399), so computed + age never exceeded 23 h: a 30 d 5 h old object reported **5**. Harmless at the + default `age=0`, which is why it went unnoticed — but `--age 24`, the natural + choice by analogy with `--useage 24`, would produce an empty auxiliary table + and a dry-run reporting a clean bucket. Now uses `total_seconds()`. + +- **`--usecollected` against a missing or empty auxiliary table exited 0**, + which is indistinguishable from success. That is exactly what a load-balanced + ClickHouse Service produces, because the auxiliary table is a *node-local* + `ReplacingMergeTree`: collect writes it on one replica and a later phase looks + for it on another. It now fails loudly and explains the replica-pinning + requirement. + +- **GCS endpoints now fall back to per-object deletion automatically.** Google + Cloud Storage's S3-compatible API has no batch `DeleteObjects`, so + `remove_objects()` fails there. Detected from the endpoint, with a warning + that the per-object path is markedly slower. + +### Added + +- **Warning when `--samples` disagrees with the auxiliary table's + `PARTITION BY`.** The table is created as `PARTITION BY CRC32(objpath) % + ` at collect time, so a different value during the use phase loses + partition pruning. Measured on production-scale data: ~2 min per sample when + matched against ~26 min when not. + +- **Cumulative deletion total alongside the per-attempt one.** The closing + `N objects … are removed` line counts only the process that printed it, which + understated one resumed run by 16.61 TiB. It now says "in this attempt" and + logs the auxiliary table's cumulative tombstone count. + +- **30 regression tests** covering the boolean matrix (per flag, per spelling, + plus bare-CLI compatibility), the age filter, the fail-loud path, the samples + warning, the GCS fallback, and renderer pull-secret handling. + +### Changed + +- **Images are published publicly to `ghcr.io/altinity/s3gc`** instead of a + private Docker Hub repository, matching `altinity-mcp` and + `altinity-sql-browser`. CI authenticates with the automatic `GITHUB_TOKEN`, + so there is no registry secret to manage or rotate. + + *Why it matters operationally:* a private image forces whoever runs a Job to + copy a registry credential into the target namespace as an `imagePullSecret` + and remember to delete it afterwards. A public image removes that step + entirely. + +- **`IMAGE_PULL_SECRET` is now optional.** `render.py` omits the + `imagePullSecrets` block when the value is empty, rather than emitting a + meaningless `- name: ""`. Set it only for a private mirror. + +- **Multi-architecture builds are mandatory, not advisory.** ClickHouse node + pools are frequently arm64 — one observed pool was 5× arm64 and 1× amd64, + where an amd64-only image can only ever schedule on a sixth of the capacity. + CI builds `linux/amd64,linux/arm64` in a single step. + +- **CI validates the rendered manifest** with `kubectl apply --dry-run=client` + in addition to running the tests and the renderer. + +### Documentation + +- `CHHOST` must be a **per-replica** Service, never the load-balanced one, and + every phase of a cleanup must use the same host. +- The minimum ClickHouse grant set: `SELECT ON system.*`; + `SELECT, INSERT, CREATE TABLE ON .*`; `REMOTE ON *.*`. Notably **not** + `S3 ON *.*` — `s3gc` lists buckets with its own client, not the `s3()` table + function — and not `TRUNCATE` unless `--keepdata` is omitted. +- Sharding recipe for `--collectonly`, which has no resume: a crash re-lists + from the beginning, which is expensive on multi-million-object buckets. + Re-running a shard is safe because the auxiliary table is a + `ReplacingMergeTree` keyed on `objpath`. +- Per-cluster values that cause silent failure when wrong: `S3PATH` may + legitimately be empty (bucket-root layouts); the disk is not always named + `s3` (GCS-backed clusters commonly use `gcs`, which changes both the + anti-join scope and the auxiliary table name); a `*_cache` disk is a + filesystem cache over the same blobs, not a second reference scope. +- Buildx builder containers cache `/etc/resolv.conf` at creation, so a builder + left running across a network change fails with + `lookup registry-1.docker.io: i/o timeout` while the host resolves fine. + +### Known gaps + +- **`--collectonly` still has no resume.** The sharding recipe covers it + operationally; a `--collectafter` checkpoint would remove the need. +- **The GHCR package must be marked public once** in the organisation's package + settings after the first publish, otherwise pulls still require + authentication and the pull-secret benefit is not realised. + +## v0.2 — 2025-01-31 + +- Added an option to avoid batch deletion for services such as GCS. + +## v0.1 — 2024-06-12 + +- Added object last-modified timestamps to the auxiliary table. +- Added the object age option. diff --git a/CLAUDE.md b/CLAUDE.md index 3c1e494..95b364f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -51,6 +51,7 @@ tests, explicit delete controls, and conservative deployment defaults. | `deploy/kubernetes/job.yaml.tmpl` | Kubernetes Job template with security context and environment wiring. | | `deploy/kubernetes/example.env` | Non-secret rendering example; copy it outside the repository for a real run. | | `.github/workflows/container.yml` | CI test, render, manifest validation, and container publication workflow. | +| `CHANGELOG.md` | What changed and **why**, including evidence for defects found in production use. Update it in the same change as any behaviour, safety, or deployment change. | ## Required checks @@ -76,6 +77,10 @@ percentage until coverage tooling and an enforceable threshold are introduced. - Keep the command-line and `S3GC_*` environment interfaces compatible unless the task explicitly authorizes a breaking operational change. +- Record behaviour, safety and deployment changes in `CHANGELOG.md` as part of + the same change. Write down *why*, and keep the evidence for defects found in + production — the reasoning is the expensive part to reconstruct later. Never + put customer names, cluster identifiers or credentials there. - Treat the renderer, entrypoint, README, and Kubernetes guide as part of the same operator-facing contract. Update the affected documentation in the same change as an operational behavior change. diff --git a/README.md b/README.md index 9f66820..b12ec5c 100644 --- a/README.md +++ b/README.md @@ -250,13 +250,8 @@ because it can intentionally delete development objects. ## History and roadmap -### v0.2 — 2025-01-31 +See [`CHANGELOG.md`](CHANGELOG.md) for the full history, including why each +change was made and the evidence behind defects found in production use. -- Added an option to avoid batch deletion for services such as GCS. - -### v0.1 — 2024-06-12 - -- Added object last-modified timestamps to the auxiliary table. -- Added the object age option. - -Planned: concurrency and asynchronous collection/deletion. +Planned: concurrency and asynchronous collection/deletion; a `--collectafter` +checkpoint so an interrupted collect can resume instead of re-listing. From a897e948ca9871ad25f1b7fc9ae0d5f4ad9b02cc Mon Sep 17 00:00:00 2001 From: "Diego Nieto (lesandie)" Date: Thu, 6 Aug 2026 11:53:54 +0200 Subject: [PATCH 11/16] Correct the boolean defect's scope; route twins through coerce_bool Verifying the fix against a development cluster showed the original scope claim was wrong. The codebase already paired each action="store_true" flag with a type=bool twin sharing the same dest, and eleven of the thirteen boolean flags had one and behaved correctly. Only --s3useiam and --listoptions lacked a twin, so the practical defect was --s3useiam alone. A dev cluster log from the earlier image proves the point: S3GC_S3SECURE_FLAG=false produced secure=False, a real bool, not the truthy string. Restores the --order-by-objpath-flag twin removed in 61b0ad2, since dropping it was an unnecessary command-line break, and routes all eleven surviving twins through coerce_bool instead of bool. type=bool rejects 0, 1 and empty values with an ArgumentError; coerce_bool accepts them, so the environment and the command line now agree on what a boolean looks like. The post-parse coercion stays as the uniform safety net: the twin pattern is easy to forget when adding a flag, which is exactly how --s3useiam broke. Adds migration notes for pull secrets being registry-scoped (a Docker Hub secret does not apply to ghcr.io, which surfaces as ImagePullBackOff with a 401 on the anonymous token), the one-time package visibility flip, and the changed meaning of S3GC_S3USEIAM=false. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 65 +++++++++++++++++++++++++++++++++------------------- s3gc.py | 29 ++++++++++++++--------- 2 files changed, 60 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c98864..b0c2d7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,29 +19,26 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. ### Fixed -- **Boolean options were unusable from the environment, and one of them hung a - production Job indefinitely.** Flags declared with `action="store_true"` are - populated by `jsonargparse` from the environment as the **raw string**, and - every non-empty string is truthy in Python — so `S3GC_S3USEIAM=false` meant - *true*. The affected Job selected `IamAwsProvider()` instead of the static - keys in its Secret and wedged in the IMDS credential loop: no error, no - exception, no log line, zero rows after five minutes, no `:443` connection - ever opened, 0.34 s of CPU — only `activeDeadlineSeconds` ended it. Removing - the variable made the same image, Secret and manifest work immediately at - ~3,500 objects/s. - - Thirteen flags shared the defect and four are set by `job.yaml.tmpl` - (`S3GC_S3USEIAM`, `S3GC_S3SECURE_FLAG`, `S3GC_ORDER_BY_OBJPATH`, - `S3GC_VERBOSE_FLAG`); two were correct only because `"true"` happens to be - truthy, and `S3GC_S3SECURE_FLAG=false` would have silently stayed on TLS. - - *Why not `type=bool`:* it raises on `0`, `1` and empty values, and it would - force bare flags such as `--collectonly` to take an argument, which - `docker/kubernetes-entrypoint.sh` and every documented invocation rely on. - Instead all boolean options are coerced once after parsing through the - existing `strtobool` helper, accepting `true/false`, `yes/no`, `on/off`, - `1/0`, empty and unset. The `--order-by-objpath-flag` twin argument, a - previous one-off workaround for this same defect, is retired. +- **`--s3useiam` was missing its `type=bool` twin, and the resulting misparse hung a + production Job indefinitely.** The codebase pairs each `action="store_true"` flag + with a second `type=bool` argument sharing the same `dest`, because + `jsonargparse` populates `store_true` flags from the environment as the **raw + string** and every non-empty string is truthy in Python. Eleven of the thirteen + boolean flags had that twin and behaved correctly; `--s3useiam` did not, so + `S3GC_S3USEIAM=false` meant *true*. + + The affected Job selected `IamAwsProvider()` instead of the static keys in its + Secret and wedged in the IMDS credential loop: no error, no exception, no log + line, zero rows after five minutes, no `:443` connection ever opened, 0.34 s of + CPU — only `activeDeadlineSeconds` ended it. Removing the variable made the same + image, Secret and manifest work immediately at ~3,500 objects/s. + + *Fix:* rather than add a twelfth twin — the pattern is easy to forget, which is + exactly how this defect arose — all boolean options are now coerced once after + parsing through the existing `strtobool` helper. The twins are retained for + command-line compatibility and now share the same coercion, which also means + they accept `0`, `1` and empty values; `type=bool` rejected those with an + `ArgumentError`. Unset and empty both mean false. - **`--age` silently collected nothing for anything older than a day.** The filter used `timedelta.seconds`, the sub-day remainder (0..86399), so computed @@ -124,6 +121,28 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. left running across a network change fails with `lookup registry-1.docker.io: i/o timeout` while the host resolves fine. +### Migration notes + +- **Pull secrets are registry-scoped.** Moving from Docker Hub to GHCR silently + invalidates an existing `imagePullSecret` even though its *name* still looks + right: a secret holding `index.docker.io` credentials does not apply to + `ghcr.io`, so the kubelet falls back to an anonymous token and the pod sits in + `ImagePullBackOff` with `failed to fetch anonymous token: 401 Unauthorized`. + With the public image the correct action is to **remove** the secret reference + (leave `IMAGE_PULL_SECRET` empty), not to repoint it. + +- **The GHCR package must be made public once**, in the organisation's package + settings. It cannot be done through the REST API — the visibility endpoint + returns 404 and the standard token lacks `write:packages`. Until it is flipped, + every pull still needs credentials and the benefit above is not realised. + +- **`S3GC_S3USEIAM=false` now means false.** Deployments that set it expecting + static credentials were previously getting the IAM chain instead — and, on a + cluster without a usable workload identity, an indefinite hang. After this + change they get what they asked for. No action is needed unless a deployment + was relying on the broken behaviour to reach IAM, in which case set it to + `true` explicitly. + ### Known gaps - **`--collectonly` still has no resume.** The sharding recipe covers it diff --git a/s3gc.py b/s3gc.py index 56723c6..deeeb11 100644 --- a/s3gc.py +++ b/s3gc.py @@ -141,7 +141,7 @@ def coerce_bool(value): parser.add_argument( "--s3secureflag", "--s3-secure-flag", - type=bool, + type=coerce_bool, dest="s3secure_flag", default=False, help="S3 secure mode", @@ -186,7 +186,7 @@ def coerce_bool(value): parser.add_argument( "--keepdataflag", "--keep-data-flag", - type=bool, + type=coerce_bool, dest="keepdata_flag", default=False, help="keep auxiliary data in ClickHouse table", @@ -202,7 +202,7 @@ def coerce_bool(value): parser.add_argument( "--collectonlyflag", "--collect-only-flag", - type=bool, + type=coerce_bool, dest="collectonly_flag", default=False, help="put object names to auxiliary table", @@ -218,7 +218,7 @@ def coerce_bool(value): parser.add_argument( "--usecollectedflag", "--use-collected-flag", - type=bool, + type=coerce_bool, dest="usecollected_flag", default=False, help="auxiliary data is already collected in ClickHouse table", @@ -279,7 +279,7 @@ def coerce_bool(value): "--dryrunflag", "--dryrun-flag", "--dry-run-flag", - type=bool, + type=coerce_bool, dest="dryrun_flag", default=False, help="Calculate objects to remove without actual removing", @@ -341,6 +341,13 @@ def coerce_bool(value): default=False, help="Order anti-join output by object path (costly for large Kubernetes Jobs)", ) +parser.add_argument( + "--order-by-objpath-flag", + dest="order_by_objpath", + type=coerce_bool, + default=False, + help="Order anti-join output by object path (costly for large Kubernetes Jobs)", +) parser.add_argument( "--s3-connect-timeout", dest="s3_connect_timeout", @@ -384,7 +391,7 @@ def coerce_bool(value): "--create-database-flag", "--createdatabase-flag", dest="createdatabase_flag", - type=bool, + type=coerce_bool, default=False, help="create database for collecttable", ) @@ -400,7 +407,7 @@ def coerce_bool(value): "--drop-collecttable-flag", "--dropcollecttable-flag", dest="drop_collecttable_flag", - type=bool, + type=coerce_bool, default=False, help="drop collecttable and recreate; beware of ClickHouse DROP TABLE constraints", ) @@ -423,7 +430,7 @@ def coerce_bool(value): parser.add_argument( "--interactive-flag", dest="interactive_flag", - type=bool, + type=coerce_bool, default=True, help="confirm deleting", ) @@ -437,7 +444,7 @@ def coerce_bool(value): parser.add_argument( "--verboseflag", "--verbose-flag", - type=bool, + type=coerce_bool, dest="verbose_flag", default=False, help="debug output", @@ -452,7 +459,7 @@ def coerce_bool(value): parser.add_argument( "--debugflag", "--debug-flag", - type=bool, + type=coerce_bool, dest="debug_flag", default=False, help="trace output (more verbose)", @@ -464,7 +471,7 @@ def coerce_bool(value): "--silentflag", "--silent-flag", dest="silent_flag", - type=bool, + type=coerce_bool, default=False, help="no log", ) From 97a6cd0844cf96b6702efda02bd0524870698da6 Mon Sep 17 00:00:00 2001 From: "Diego Nieto (lesandie)" Date: Thu, 6 Aug 2026 13:34:43 +0200 Subject: [PATCH 12/16] CHANGELOG: record the env-file migration and the dev validation Two omissions from the merge entry. The rendering env file is a breaking change that was not written down: the Job template no longer emits S3GC_S3USEIAM and render.py now requires S3AUTH and S3PROFILE, so an env file saved before this change fails with "missing required values: S3AUTH, S3PROFILE". Encountered while re-testing with an env file from the previous session, which is exactly how an operator will meet it. The note distinguishes the rendering key from the S3GC_S3USEIAM environment variable, which the script still honours as a deprecated alias, so an already-deployed Job manifest keeps working. Adds a Verified section so the entries above are provenance rather than claims: all three phases ran end to end against a development cluster with the image built from this branch, pulled anonymously with no imagePullSecret, and auth=iam confirms workload identity still resolves after the credential rewrite. States plainly that the GCS per-object fallback remains unit-tested only, pending a real endpoint. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7acd9c8..b34fbb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -166,6 +166,19 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. ### Migration notes +- **Saved rendering env files must be updated: `S3USEIAM` → `S3AUTH` (+ `S3PROFILE`).** + `job.yaml.tmpl` no longer emits `S3GC_S3USEIAM`, and `render.py` now *requires* + `S3AUTH` and `S3PROFILE`, so an env file kept from before this change fails with: + + ``` + render error: missing required values: S3AUTH, S3PROFILE + ``` + + Replace `S3USEIAM=true` with `S3AUTH=iam`, or `S3USEIAM=false` with `S3AUTH=static`, + and add an empty `S3PROFILE=`. Note this only affects the **rendering** env file; + the `S3GC_S3USEIAM` *environment variable* is still honoured by the script itself as + a deprecated alias, so a Job manifest already deployed keeps working. + - **Pull secrets are registry-scoped.** Moving from Docker Hub to GHCR silently invalidates an existing `imagePullSecret` even though its *name* still looks right: a secret holding `index.docker.io` credentials does not apply to @@ -186,6 +199,17 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. was relying on the broken behaviour to reach IAM, in which case set it to `true` explicitly. +### Verified + +All three phases were exercised end to end against a development ClickHouse cluster using the +image built from this branch, pulled anonymously from the public registry with no +`imagePullSecret`: `collect` (188 objects), `dry-run` (exactly the 16 seeded orphan +fixtures), `delete` (cluster preflight, per-batch checkpoints, cumulative total) and a +verifying `dry-run` reporting zero. `auth=iam` in the log confirms workload identity still +resolves after the credential-resolution rewrite, and the referenced tables were untouched. +Unit tests cover the `static`/`aws` modes; the GCS per-object fallback is still only +unit-tested, pending a real GCS endpoint. + ### Known gaps - **`--collectonly` still has no resume.** The sharding recipe covers it From 5a385b28ad042cde021ccde1f15e85c97174920d Mon Sep 17 00:00:00 2001 From: "Diego Nieto (lesandie)" Date: Thu, 6 Aug 2026 16:46:16 +0200 Subject: [PATCH 13/16] Remove S3GC_S3USEIAM --- CHANGELOG.md | 52 +++++-------------------------------- README.md | 12 ++++----- deploy/kubernetes/README.md | 1 - s3gc.py | 32 +++-------------------- tests/test_s3gc.py | 42 +++++++++--------------------- 5 files changed, 26 insertions(+), 113 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b34fbb7..90aa44e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,27 +19,6 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. ### Fixed -- **`--s3useiam` was missing its `type=bool` twin, and the resulting misparse hung a - production Job indefinitely.** The codebase pairs each `action="store_true"` flag - with a second `type=bool` argument sharing the same `dest`, because - `jsonargparse` populates `store_true` flags from the environment as the **raw - string** and every non-empty string is truthy in Python. Eleven of the thirteen - boolean flags had that twin and behaved correctly; `--s3useiam` did not, so - `S3GC_S3USEIAM=false` meant *true*. - - The affected Job selected `IamAwsProvider()` instead of the static keys in its - Secret and wedged in the IMDS credential loop: no error, no exception, no log - line, zero rows after five minutes, no `:443` connection ever opened, 0.34 s of - CPU — only `activeDeadlineSeconds` ended it. Removing the variable made the same - image, Secret and manifest work immediately at ~3,500 objects/s. - - *Fix:* rather than add a twelfth twin — the pattern is easy to forget, which is - exactly how this defect arose — all boolean options are now coerced once after - parsing through the existing `strtobool` helper. The twins are retained for - command-line compatibility and now share the same coercion, which also means - they accept `0`, `1` and empty values; `type=bool` rejected those with an - `ArgumentError`. Unset and empty both mean false. - - **`--age` silently collected nothing for anything older than a day.** The filter used `timedelta.seconds`, the sub-day remainder (0..86399), so computed age never exceeded 23 h: a 30 d 5 h old object reported **5**. Harmless at the @@ -78,11 +57,9 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. | `aws` | boto3 chain / `--s3profile` | **yes** | | `iam` | MinIO workload identity (IRSA/IMDS/ECS) | no | - `--s3profile` implies `aws`; **`--s3useiam` is now a deprecated alias for - `--s3auth=iam`** and still works, with a warning — every existing manifest and - Secret keeps working unchanged. Contradictory combinations are rejected rather than - silently resolved, so nobody ends up authenticating with an identity they did not - ask for. + `--s3profile` implies `aws`. Contradictory combinations are rejected rather + than silently resolved, so nobody ends up authenticating with an identity they + did not ask for. - **Operator-facing S3 listing errors**, also from PR #2: a failed listing now names the required permission (`s3:ListBucket` on the bucket ARN, **even for @@ -143,6 +120,9 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. - **CI validates the rendered manifest** with `kubectl apply --dry-run=client` in addition to running the tests and the renderer. +- Removed the deprecated S3 IAM selector. `S3AUTH=static|aws|iam` is now the + only supported authentication interface. + ### Documentation - `CHHOST` must be a **per-replica** Service, never the load-balanced one, and @@ -166,19 +146,6 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. ### Migration notes -- **Saved rendering env files must be updated: `S3USEIAM` → `S3AUTH` (+ `S3PROFILE`).** - `job.yaml.tmpl` no longer emits `S3GC_S3USEIAM`, and `render.py` now *requires* - `S3AUTH` and `S3PROFILE`, so an env file kept from before this change fails with: - - ``` - render error: missing required values: S3AUTH, S3PROFILE - ``` - - Replace `S3USEIAM=true` with `S3AUTH=iam`, or `S3USEIAM=false` with `S3AUTH=static`, - and add an empty `S3PROFILE=`. Note this only affects the **rendering** env file; - the `S3GC_S3USEIAM` *environment variable* is still honoured by the script itself as - a deprecated alias, so a Job manifest already deployed keeps working. - - **Pull secrets are registry-scoped.** Moving from Docker Hub to GHCR silently invalidates an existing `imagePullSecret` even though its *name* still looks right: a secret holding `index.docker.io` credentials does not apply to @@ -192,13 +159,6 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. returns 404 and the standard token lacks `write:packages`. Until it is flipped, every pull still needs credentials and the benefit above is not realised. -- **`S3GC_S3USEIAM=false` now means false.** Deployments that set it expecting - static credentials were previously getting the IAM chain instead — and, on a - cluster without a usable workload identity, an indefinite hang. After this - change they get what they asked for. No action is needed unless a deployment - was relying on the broken behaviour to reach IAM, in which case set it to - `true` explicitly. - ### Verified All three phases were exercised end to end against a development ClickHouse cluster using the diff --git a/README.md b/README.md index 53c012a..ae98daf 100644 --- a/README.md +++ b/README.md @@ -92,7 +92,6 @@ or interactive shell rather than saving them in a file: export S3GC_CHPASS='' export S3GC_S3ACCESSKEY='' export S3GC_S3SECRETKEY='' -export S3GC_S3USEIAM=false ``` Every `S3GC_*` boolean accepts `true/false`, `yes/no`, `on/off`, `1/0`, or an @@ -108,9 +107,8 @@ Select one with `S3GC_S3AUTH` (or `--s3auth`): | `aws` | boto3 credential chain, optionally `S3GC_S3PROFILE` | **yes** | AWS SSO / named profiles on a workstation | | `iam` | MinIO workload identity provider | no | EKS IRSA, EC2 instance profile, ECS task role | -`S3GC_S3PROFILE` implies `aws`. `S3GC_S3USEIAM=true` is a **deprecated alias** -for `S3GC_S3AUTH=iam` — it still works and logs a deprecation warning. -Contradictory combinations are rejected rather than silently resolved. +`S3GC_S3PROFILE` implies `aws`. Contradictory combinations are rejected rather +than silently resolved. Prefer `iam` over `aws` inside Kubernetes: it refreshes temporary credentials through MinIO's provider and keeps `boto3` out of the request path. @@ -147,7 +145,7 @@ aws s3api list-objects-v2 --bucket --prefix --max-keys 1 --pro ```bash export S3GC_CHPASS='' -export S3GC_S3AUTH=iam # or the deprecated S3GC_S3USEIAM=true +export S3GC_S3AUTH=iam ``` #### GCS and other stores without batch delete @@ -188,7 +186,7 @@ done ### IAM role support -With `S3GC_S3USEIAM=true`, `s3gc` uses MinIO's AWS IAM credential provider. +With `S3GC_S3AUTH=iam`, `s3gc` uses MinIO's AWS IAM credential provider. It obtains and refreshes temporary credentials from one of these environments: - an EKS Pod using IRSA/workload identity (`AWS_WEB_IDENTITY_TOKEN_FILE` and @@ -196,7 +194,7 @@ It obtains and refreshes temporary credentials from one of these environments: - an EC2 instance with an attached instance profile; or - an ECS task with task-role credentials. -Setting `S3GC_S3USEIAM=true` on an ordinary workstation is not enough. The +Setting `S3GC_S3AUTH=iam` on an ordinary workstation is not enough. The current provider does **not** read AWS CLI profiles, `aws sso login` state, `~/.aws/config`, or `AWS_PROFILE`. For a direct local run, use static S3 keys or run the script from an identity-enabled EC2/EKS/ECS environment. diff --git a/deploy/kubernetes/README.md b/deploy/kubernetes/README.md index 07c935b..56a9dbf 100644 --- a/deploy/kubernetes/README.md +++ b/deploy/kubernetes/README.md @@ -49,7 +49,6 @@ needed. | `aws` | boto3 chain, optionally `S3PROFILE` | **yes** | rarely in-cluster; this is a workstation SSO path | `S3PROFILE` requires `S3AUTH=aws` and the renderer rejects other combinations. -`S3GC_S3USEIAM=true` still works as a deprecated alias for `S3AUTH=iam`. Prefer `iam`: it hands MinIO the credential provider, so temporary credentials refresh during a long collect or delete instead of expiring mid-run. diff --git a/s3gc.py b/s3gc.py index 33cf7f2..d2aabe0 100644 --- a/s3gc.py +++ b/s3gc.py @@ -192,22 +192,6 @@ def coerce_bool(value): default="s3", help="S3 disk name", ) -parser.add_argument( - "--s3useiam", - "--s3-use-iam", - action="store_true", - dest="s3useiam", - default=False, - help="DEPRECATED alias for --s3auth=iam. Use the workload identity credential chain", -) -parser.add_argument( - "--s3useiamflag", - "--s3-use-iam-flag", - type=coerce_bool, - dest="s3useiam", - default=False, - help="DEPRECATED alias for --s3auth=iam. Use the workload identity credential chain", -) parser.add_argument( "--keepdata", "--keep-data", @@ -526,13 +510,11 @@ def coerce_bool(value): args = parser.parse_args() # Every flag declared with action="store_true" arrives from the environment as a -# raw string, and every non-empty string is truthy — so S3GC_S3USEIAM=false used -# to select the IAM credential provider and hang a Kubernetes Job indefinitely. -# Normalise all boolean options in one place, immediately after parsing, so the -# rest of the program can rely on real bools. +# raw string, and every non-empty string is truthy. Normalise all boolean +# options in one place, immediately after parsing, so the rest of the program +# can rely on real bools. BOOLEAN_DESTS = ( "s3secure_flag", - "s3useiam", "use_remove_objects", "keepdata_flag", "collectonly_flag", @@ -782,14 +764,6 @@ def resolve_s3_credentials(): if auth_mode not in ("static", "aws"): raise ValueError(f"s3profile implies s3auth=aws, which conflicts with s3auth={auth_mode}") auth_mode = "aws" - if args.s3useiam: - logger.warning( - "s3useiam is deprecated; use s3auth=iam. Continuing with s3auth=iam." - ) - if auth_mode not in ("static", "iam"): - raise ValueError(f"s3useiam implies s3auth=iam, which conflicts with s3auth={auth_mode}") - auth_mode = "iam" - if auth_mode == "aws": return resolve_aws_s3_credentials() if auth_mode == "iam": diff --git a/tests/test_s3gc.py b/tests/test_s3gc.py index e11ab17..bae2da6 100644 --- a/tests/test_s3gc.py +++ b/tests/test_s3gc.py @@ -1,3 +1,4 @@ +from argparse import ArgumentError import os import subprocess import sys @@ -86,7 +87,6 @@ def make_args(**overrides): # S3 auth surface (static | aws | iam) "s3auth": "static", "s3profile": "", - "s3useiam": False, "s3accesskey": "", "s3secretkey": "", "s3sessiontoken": "", @@ -383,7 +383,6 @@ def test_coerce_bool_rejects_nonsense(s3gc_module): @pytest.mark.parametrize( "dest, env_name", [ - ("s3useiam", "S3GC_S3USEIAM"), ("s3secure_flag", "S3GC_S3SECURE_FLAG"), ("dryrun_flag", "S3GC_DRYRUN_FLAG"), ("keepdata_flag", "S3GC_KEEPDATA_FLAG"), @@ -392,27 +391,11 @@ def test_coerce_bool_rejects_nonsense(s3gc_module): ], ) def test_boolean_env_false_is_false(monkeypatch, dest, env_name): - """S3GC_*=false used to be the truthy string 'false'. - - S3GC_S3USEIAM=false selected the IAM credential provider and hung a - Kubernetes Job indefinitely with no error, no exception and no log line. - """ + """S3GC_*=false must not remain the truthy string 'false'.""" module = _load_with_env(monkeypatch, **{env_name: "false"}) assert getattr(module["args"], dest) is False -@pytest.mark.parametrize("value", ["true", "1", "yes"]) -def test_boolean_env_true_is_true(monkeypatch, value): - module = _load_with_env(monkeypatch, S3GC_S3USEIAM=value) - assert module["args"].s3useiam is True - - -def test_boolean_env_zero_is_false(monkeypatch): - """'0' must not be truthy either, and must not raise (type=bool would).""" - module = _load_with_env(monkeypatch, S3GC_S3USEIAM="0") - assert module["args"].s3useiam is False - - def test_bare_cli_flag_still_enables(monkeypatch): """Coercion must not break `--dryrun` used as a bare flag.""" import runpy @@ -423,6 +406,15 @@ def test_bare_cli_flag_still_enables(monkeypatch): assert module["args"].dryrun_flag is True +def test_removed_s3useiam_cli_flag_is_rejected(monkeypatch): + import runpy + + root = Path(__file__).resolve().parents[1] + monkeypatch.setattr(sys, "argv", [str(root / "s3gc.py"), "--s3useiam"]) + with pytest.raises(ArgumentError, match="Unrecognized arguments: --s3useiam"): + runpy.run_path(str(root / "s3gc.py"), run_name="s3gc_test") + + def test_collect_age_filter_uses_total_seconds(s3gc_module, args_factory, monkeypatch): """--age 24 must keep a 30-day-old object. @@ -531,7 +523,6 @@ def test_gcs_endpoint_disables_batch_delete(s3gc_module, args_factory, monkeypat s3ip="storage.googleapis.com", s3port=443, use_remove_objects=True, - s3useiam=False, s3secure_flag=True, s3accesskey="k", s3secretkey="s", @@ -594,7 +585,7 @@ def test_renderer_keeps_configured_image_pull_secret(tmp_path): def _resolve(s3gc_module, monkeypatch, **overrides): namespace = s3gc_module["resolve_s3_credentials"].__globals__ args = types.SimpleNamespace( - s3auth="static", s3profile="", s3useiam=False, + s3auth="static", s3profile="", s3accesskey="", s3secretkey="", s3sessiontoken="", s3region="eu-central-1", ) for key, value in overrides.items(): @@ -641,14 +632,6 @@ def test_iam_mode_defers_to_the_provider(s3gc_module, monkeypatch): assert result[4] == "iam" -def test_s3useiam_is_a_deprecated_alias_for_iam(s3gc_module, monkeypatch, caplog): - """Every validated Kubernetes deployment sets S3GC_S3USEIAM=true.""" - with caplog.at_level("WARNING"): - result = _resolve(s3gc_module, monkeypatch, s3useiam=True) - assert result[4] == "iam" - assert "deprecated" in caplog.text - - def test_s3profile_implies_aws_mode(s3gc_module, monkeypatch): calls = [] namespace = s3gc_module["resolve_s3_credentials"].__globals__ @@ -669,7 +652,6 @@ def test_unknown_auth_mode_is_rejected(s3gc_module, monkeypatch): "overrides", [ {"s3auth": "iam", "s3profile": "sso"}, # profile implies aws, conflicts with iam - {"s3auth": "aws", "s3useiam": True}, # s3useiam implies iam, conflicts with aws ], ) def test_contradictory_auth_settings_error(s3gc_module, monkeypatch, overrides): From ab4936a7b2d95dde5552ed3ae5ce4494e083fc6c Mon Sep 17 00:00:00 2001 From: "Diego Nieto (lesandie)" Date: Thu, 6 Aug 2026 17:05:58 +0200 Subject: [PATCH 14/16] Splitted CHANGELOG WIP to TODO and updated agents to take this into account --- AGENTS.md | 10 ++++++---- CHANGELOG.md | 8 -------- CLAUDE.md | 10 +++++++++- TODO.md | 15 +++++++++++++++ 4 files changed, 30 insertions(+), 13 deletions(-) create mode 100644 TODO.md diff --git a/AGENTS.md b/AGENTS.md index cf755ee..b36b386 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,15 +1,17 @@ # Codex guide — s3gc -Read `CLAUDE.md` before making substantive changes in this repository. -`CLAUDE.md` is the full contributor guide and the primary source of truth. +Read `CLAUDE.md`, `CHANGELOG.md`, and `TODO.md` before making substantive +changes in this repository. `CLAUDE.md` is the full contributor guide and the +primary source of truth; the changelog provides recent historical context and +the TODO file records unshipped follow-ups. This file is intentionally short so agent tooling can find the critical rules quickly, then defer to `CLAUDE.md` for complete repository guidance. ## Critical rules -1. **Read `CLAUDE.md` first.** Treat it as required repository context, not - optional background reading. +1. **Read `CLAUDE.md` first, then `CHANGELOG.md` and `TODO.md`.** Treat them + as required repository context, not optional background reading. 2. **Protect destructive-operation safeguards.** `s3gc` deletes orphaned S3 objects only after collection, dry-run review, explicit confirmation, and the applicable ClickHouse cluster/replica preflight. Do not weaken these diff --git a/CHANGELOG.md b/CHANGELOG.md index 90aa44e..aaaaefb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -170,14 +170,6 @@ resolves after the credential-resolution rewrite, and the referenced tables were Unit tests cover the `static`/`aws` modes; the GCS per-object fallback is still only unit-tested, pending a real GCS endpoint. -### Known gaps - -- **`--collectonly` still has no resume.** The sharding recipe covers it - operationally; a `--collectafter` checkpoint would remove the need. -- **The GHCR package must be marked public once** in the organisation's package - settings after the first publish, otherwise pulls still require - authentication and the pull-secret benefit is not realised. - ## v0.2 — 2025-01-31 - Added an option to avoid batch deletion for services such as GCS. diff --git a/CLAUDE.md b/CLAUDE.md index 95b364f..952609b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,6 +39,11 @@ tests, explicit delete controls, and conservative deployment defaults. environment. Add or update a dependency only when it is necessary for the requested capability, and test the resulting workflow. +6. **Read project history and backlog before substantive changes.** Review + `CHANGELOG.md` for recent behaviour and operational evidence, then `TODO.md` + for deliberately deferred work. Do not treat a TODO item as already + implemented or use the changelog as a backlog. + ## Repository map | Path | Purpose | @@ -51,7 +56,8 @@ tests, explicit delete controls, and conservative deployment defaults. | `deploy/kubernetes/job.yaml.tmpl` | Kubernetes Job template with security context and environment wiring. | | `deploy/kubernetes/example.env` | Non-secret rendering example; copy it outside the repository for a real run. | | `.github/workflows/container.yml` | CI test, render, manifest validation, and container publication workflow. | -| `CHANGELOG.md` | What changed and **why**, including evidence for defects found in production use. Update it in the same change as any behaviour, safety, or deployment change. | +| `CHANGELOG.md` | Shipped behaviour and **why**, including evidence for defects found in production use. Update it in the same change as any behaviour, safety, or deployment change. | +| `TODO.md` | Unshipped engineering and operational follow-ups. Move completed work to the changelog when it lands. | ## Required checks @@ -81,6 +87,8 @@ percentage until coverage tooling and an enforceable threshold are introduced. the same change. Write down *why*, and keep the evidence for defects found in production — the reasoning is the expensive part to reconstruct later. Never put customer names, cluster identifiers or credentials there. +- Record pending engineering or operational work in `TODO.md`, not in the + changelog. Remove or update the TODO item when the work lands. - Treat the renderer, entrypoint, README, and Kubernetes guide as part of the same operator-facing contract. Update the affected documentation in the same change as an operational behavior change. diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000..43ca0eb --- /dev/null +++ b/TODO.md @@ -0,0 +1,15 @@ +# TODO + +Unshipped engineering and operational follow-ups. Completed behaviour belongs +in `CHANGELOG.md`; this file is the forward-looking backlog. + +- [ ] Add resumable `--collectonly` collection checkpoints. A crash currently + restarts listing from the beginning; `--collectafter` would allow a large + bucket collection to resume safely. +- [ ] Add an opt-in `dev_cluster` GCS end-to-end test. It must require explicit + environment configuration, create an isolated tiered-policy fixture, run + `collect → dry-run → delete → verify`, clean up ClickHouse and object-storage + fixtures, and remain excluded from CI. +- [ ] Make the `ghcr.io/altinity/s3gc` package public in the organisation's + package settings after its first publish, so Kubernetes pulls need no + registry credentials. From 1176a3cf95a5fd6671160b8d82c40484e359595b Mon Sep 17 00:00:00 2001 From: "Diego Nieto (lesandie)" Date: Fri, 7 Aug 2026 09:52:56 +0200 Subject: [PATCH 15/16] TDD approach for AI agents --- .github/workflows/container.yml | 4 +- AGENTS.md | 13 +++--- CHANGELOG.md | 12 ++++-- CLAUDE.md | 27 ++++++++----- README.md | 72 ++++++++++++++------------------- TODO.md | 7 +++- deploy/kubernetes/README.md | 11 +---- 7 files changed, 74 insertions(+), 72 deletions(-) diff --git a/.github/workflows/container.yml b/.github/workflows/container.yml index 19c84a9..63b01ce 100644 --- a/.github/workflows/container.yml +++ b/.github/workflows/container.yml @@ -29,7 +29,9 @@ jobs: with: python-version: "3.11" - run: python -m pip install --disable-pip-version-check -r requirements.txt -r requirements-dev.txt - - run: pytest -v + # Development-cluster tests require explicit credentials and are never + # part of the offline pull-request gate. + - run: pytest -v -m "not dev_cluster" # The renderer is part of the contract: it refuses images that are not # digest-pinned, so a broken template breaks every deployment. - run: python deploy/kubernetes/render.py deploy/kubernetes/example.env > /tmp/s3gc-job.yaml diff --git a/AGENTS.md b/AGENTS.md index b36b386..ea94093 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,7 +3,8 @@ Read `CLAUDE.md`, `CHANGELOG.md`, and `TODO.md` before making substantive changes in this repository. `CLAUDE.md` is the full contributor guide and the primary source of truth; the changelog provides recent historical context and -the TODO file records unshipped follow-ups. +the TODO file records possible, interesting, and deliberately deferred +improvements. This file is intentionally short so agent tooling can find the critical rules quickly, then defer to `CLAUDE.md` for complete repository guidance. @@ -16,10 +17,12 @@ quickly, then defer to `CLAUDE.md` for complete repository guidance. objects only after collection, dry-run review, explicit confirmation, and the applicable ClickHouse cluster/replica preflight. Do not weaken these controls without explicit approval and matching tests and documentation. -3. **Required checks are offline.** Run the relevant pytest suite and - Kubernetes renderer/manifest dry-run checks described in `CLAUDE.md`. - Automated tests must not contact live ClickHouse or object storage, or - delete objects. +3. **Test coverage and required checks are mandatory.** For every feature, + bug fix, or material behaviour change, add a focused test when no existing + test covers it; use test-driven development for behaviour changes. Run the + relevant offline pytest suite and Kubernetes renderer/manifest dry-run + checks described in `CLAUDE.md`. Automated tests must not contact live + ClickHouse or object storage, or delete objects. 4. **No secrets or customer data in Git.** Never commit credentials, customer configuration, target-cluster details, or rendered customer manifests. Use Kubernetes Secrets or workload identity for production credentials. diff --git a/CHANGELOG.md b/CHANGELOG.md index aaaaefb..0cea754 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -120,6 +120,9 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. - **CI validates the rendered manifest** with `kubectl apply --dry-run=client` in addition to running the tests and the renderer. +- **CI explicitly excludes `dev_cluster` tests.** The pull-request suite stays + offline even after opt-in environment-dependent coverage is added. + - Removed the deprecated S3 IAM selector. `S3AUTH=static|aws|iam` is now the only supported authentication interface. @@ -127,10 +130,11 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. - `CHHOST` must be a **per-replica** Service, never the load-balanced one, and every phase of a cleanup must use the same host. -- The minimum ClickHouse grant set: `SELECT ON system.*`; - `SELECT, INSERT, CREATE TABLE ON .*`; `REMOTE ON *.*`. Notably **not** - `S3 ON *.*` — `s3gc` lists buckets with its own client, not the `s3()` table - function — and not `TRUNCATE` unless `--keepdata` is omitted. +- S3 authentication guidance now keeps static, AWS SSO/profile, and workload + identity modes together; the Kubernetes guide calls out dedicated ClickHouse + user provisioning and only the required table and system-table grants. +- Contributor guidance now requires test-driven, focused coverage for every + feature, defect, and material operational behaviour change. - Sharding recipe for `--collectonly`, which has no resume: a crash re-lists from the beginning, which is expensive on multi-million-object buckets. Re-running a shard is safe because the auxiliary table is a diff --git a/CLAUDE.md b/CLAUDE.md index 952609b..c2ab090 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,35 +14,41 @@ tests, explicit delete controls, and conservative deployment defaults. not retry automatically. A behavior change in this path needs regression tests and matching README/deployment documentation. -2. **Tests stay offline by default.** Install both requirements files, then +2. **Test coverage and TDD are mandatory.** For every new feature, bug fix, or + material operational behaviour change, identify the test that covers it; if + none exists, add a focused test in the same change. Start behaviour changes + with a failing test, and add a regression test for every defect. Purely + editorial changes are exempt. + +3. **Tests stay offline by default.** Install both requirements files, then run `pytest -v` for relevant changes. Tests must use fakes, local subprocesses, and manifest dry-runs; they must not contact live ClickHouse, S3-compatible storage, or delete objects. The `dev_cluster` marker is explicitly environment-dependent and never runs in CI. Do not add live credentials or a live-delete test path to the default suite. -3. **No secrets or customer artifacts in Git.** Do not commit S3 keys, +4. **No secrets or customer artifacts in Git.** Do not commit S3 keys, ClickHouse passwords, customer `.env` files, rendered customer manifests, target-cluster details, or command output containing them. Keep credentials in an approved secret manager, Kubernetes Secret, or workload identity. `deploy/kubernetes/example.env` is a non-secret template only. -4. **Kubernetes deployment stays immutable and least-privileged.** +5. **Kubernetes deployment stays immutable and least-privileged.** `deploy/kubernetes/render.py` must continue to reject unpinned images and invalid phase/confirmation input. Use image digests, never mutable tags. Preserve the Job template's non-root user and read-only root filesystem; do not embed credentials in the image or manifest. -5. **Dependencies are deliberate and reproducible.** Runtime dependencies are +6. **Dependencies are deliberate and reproducible.** Runtime dependencies are pinned in `requirements.txt`; test-only dependencies are pinned in `requirements-dev.txt`. Use both files when preparing a development or CI environment. Add or update a dependency only when it is necessary for the requested capability, and test the resulting workflow. -6. **Read project history and backlog before substantive changes.** Review +7. **Read project history and backlog before substantive changes.** Review `CHANGELOG.md` for recent behaviour and operational evidence, then `TODO.md` - for deliberately deferred work. Do not treat a TODO item as already - implemented or use the changelog as a backlog. + for possible, interesting, and deliberately deferred improvements. Do not + treat a TODO item as already implemented or use the changelog as a backlog. ## Repository map @@ -57,7 +63,7 @@ tests, explicit delete controls, and conservative deployment defaults. | `deploy/kubernetes/example.env` | Non-secret rendering example; copy it outside the repository for a real run. | | `.github/workflows/container.yml` | CI test, render, manifest validation, and container publication workflow. | | `CHANGELOG.md` | Shipped behaviour and **why**, including evidence for defects found in production use. Update it in the same change as any behaviour, safety, or deployment change. | -| `TODO.md` | Unshipped engineering and operational follow-ups. Move completed work to the changelog when it lands. | +| `TODO.md` | Possible, interesting, and deliberately deferred engineering or operational improvements. Move completed work to the changelog when it lands. | ## Required checks @@ -87,8 +93,9 @@ percentage until coverage tooling and an enforceable threshold are introduced. the same change. Write down *why*, and keep the evidence for defects found in production — the reasoning is the expensive part to reconstruct later. Never put customer names, cluster identifiers or credentials there. -- Record pending engineering or operational work in `TODO.md`, not in the - changelog. Remove or update the TODO item when the work lands. +- Record possible, interesting, or pending engineering and operational + improvements in `TODO.md`, not in the changelog. Remove or update the TODO + item when the work lands. - Treat the renderer, entrypoint, README, and Kubernetes guide as part of the same operator-facing contract. Update the affected documentation in the same change as an operational behavior change. diff --git a/README.md b/README.md index ae98daf..a1fa81b 100644 --- a/README.md +++ b/README.md @@ -85,18 +85,6 @@ export S3GC_AGE=24 export S3GC_USEAGE=24 ``` -For static S3 credentials, inject the following values from a secret manager -or interactive shell rather than saving them in a file: - -```bash -export S3GC_CHPASS='' -export S3GC_S3ACCESSKEY='' -export S3GC_S3SECRETKEY='' -``` - -Every `S3GC_*` boolean accepts `true/false`, `yes/no`, `on/off`, `1/0`, or an -empty value for false. Unset also means false. - ### S3 authentication modes Select one with `S3GC_S3AUTH` (or `--s3auth`): @@ -110,8 +98,19 @@ Select one with `S3GC_S3AUTH` (or `--s3auth`): `S3GC_S3PROFILE` implies `aws`. Contradictory combinations are rejected rather than silently resolved. -Prefer `iam` over `aws` inside Kubernetes: it refreshes temporary credentials -through MinIO's provider and keeps `boto3` out of the request path. +#### Static credentials + +Inject static credentials from a secret manager or interactive shell rather +than saving them in a file: + +```bash +export S3GC_CHPASS='' +export S3GC_S3ACCESSKEY='' +export S3GC_S3SECRETKEY='' +``` + +Every `S3GC_*` boolean accepts `true/false`, `yes/no`, `on/off`, `1/0`, or an +empty value for false. Unset also means false. #### AWS SSO or a named profile @@ -148,6 +147,14 @@ export S3GC_CHPASS='' export S3GC_S3AUTH=iam ``` +`iam` uses MinIO's AWS IAM credential provider and refreshes temporary +credentials from EKS IRSA/workload identity, an EC2 instance profile, or an ECS +task role. Prefer it over `aws` inside Kubernetes: it keeps `boto3` out of the +request path and avoids credentials expiring during a long collect or delete. + +It does not read AWS CLI profiles, `aws sso login` state, `~/.aws/config`, or +`AWS_PROFILE`; use `aws` mode for that workstation workflow. + #### GCS and other stores without batch delete GCS has no batch `DeleteObjects`. `s3gc` detects a `storage.googleapis.com` @@ -165,6 +172,16 @@ export S3GC_S3DISKNAME=gcs .venv/bin/python ./s3gc.py --verbose --use-remove-objects=false ``` +### Safe split workflow + +Collection makes an auxiliary table; the second command reads it and reports +candidates without deleting objects: + +```bash +.venv/bin/python s3gc.py --collectonly --keepdata +.venv/bin/python s3gc.py --usecollected --dry-run +``` + ### Collect has no resume — shard large buckets A crashed or interrupted `--collectonly` restarts its listing from the @@ -184,33 +201,6 @@ for shard in 0 1 2 3 4 5 6 7 8 9 a b c d e f g h i j k l m n o p q r s t u v w x done ``` -### IAM role support - -With `S3GC_S3AUTH=iam`, `s3gc` uses MinIO's AWS IAM credential provider. -It obtains and refreshes temporary credentials from one of these environments: - -- an EKS Pod using IRSA/workload identity (`AWS_WEB_IDENTITY_TOKEN_FILE` and - `AWS_ROLE_ARN`); -- an EC2 instance with an attached instance profile; or -- an ECS task with task-role credentials. - -Setting `S3GC_S3AUTH=iam` on an ordinary workstation is not enough. The -current provider does **not** read AWS CLI profiles, `aws sso login` state, -`~/.aws/config`, or `AWS_PROFILE`. For a direct local run, use static S3 keys -or run the script from an identity-enabled EC2/EKS/ECS environment. - -The static-key path accepts an access key and secret key only; it does not yet -accept an AWS session token. Therefore, do not copy temporary -`aws sts assume-role` credentials into the static-key variables. - -Run the safe, split workflow directly. Collection makes an auxiliary table; -the second command reads it and reports candidates without deleting objects: - -```bash -.venv/bin/python s3gc.py --collectonly --keepdata -.venv/bin/python s3gc.py --usecollected --dry-run -``` - The same variables can be passed as flags (for example, `--ch-host` or `--s3-bucket`). Run `.venv/bin/python s3gc.py --help` for the complete flag and environment-variable reference. Avoid direct deletion for diff --git a/TODO.md b/TODO.md index 43ca0eb..dc68c6d 100644 --- a/TODO.md +++ b/TODO.md @@ -1,7 +1,8 @@ # TODO -Unshipped engineering and operational follow-ups. Completed behaviour belongs -in `CHANGELOG.md`; this file is the forward-looking backlog. +Possible, interesting, and deliberately deferred engineering or operational +improvements. Completed behaviour belongs in `CHANGELOG.md`; this file is the +forward-looking backlog. - [ ] Add resumable `--collectonly` collection checkpoints. A crash currently restarts listing from the beginning; `--collectafter` would allow a large @@ -13,3 +14,5 @@ in `CHANGELOG.md`; this file is the forward-looking backlog. - [ ] Make the `ghcr.io/altinity/s3gc` package public in the organisation's package settings after its first publish, so Kubernetes pulls need no registry credentials. +- [ ] Require the `Container / test` GitHub Actions check before pull-request + merges in the repository branch-protection settings. diff --git a/deploy/kubernetes/README.md b/deploy/kubernetes/README.md index 56a9dbf..56f0fec 100644 --- a/deploy/kubernetes/README.md +++ b/deploy/kubernetes/README.md @@ -22,9 +22,6 @@ needed. - Use an immutable, multi-architecture image digest: `ghcr.io/altinity/s3gc@sha256:`. CI prints the exact `IMAGE=` line in its job summary. Pin by digest, never by tag — tags get re-pushed. -- **No pull Secret is required**: the image is public. Leave - `IMAGE_PULL_SECRET` empty and the renderer omits the `imagePullSecrets` block. - Set it only when pulling from a private mirror. - The image must be multi-arch. ClickHouse node pools are often arm64 (one customer cluster is 5x arm64 + 1x amd64), and an amd64-only image simply will not schedule there. @@ -55,17 +52,13 @@ refresh during a long collect or delete instead of expiring mid-run. ### Minimum ClickHouse grants +Create or provision a dedicated ClickHouse user for `s3gc`, then grant it: + ```sql GRANT SELECT ON system.* TO s3gc; -- remote_data_paths, one, disks, tables GRANT SELECT, INSERT, CREATE TABLE ON .* TO s3gc; -- the auxiliary table -GRANT REMOTE ON *.* TO s3gc; -- clusterAllReplicas() ``` -`S3 ON *.*` is **not** needed — s3gc lists the bucket with its own client, not -the `s3()` table function. `TRUNCATE` is only used when `--keepdata` is absent. -A user cannot self-grant `REMOTE`; grant option only passes on privileges it -already holds. - ### Values that vary per cluster, and bite when wrong - **`S3PATH` may legitimately be empty** — some buckets keep blobs at the root. From 0f1e7c5edf8d6c0c28d86e594fa9fe80a8f15e2f Mon Sep 17 00:00:00 2001 From: "Diego Nieto (lesandie)" Date: Fri, 7 Aug 2026 10:04:09 +0200 Subject: [PATCH 16/16] use Kubeconform for CICD tet validation --- .github/workflows/container.yml | 12 +++++++++--- CHANGELOG.md | 6 ++++-- CLAUDE.md | 6 ++++-- README.md | 4 +++- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/.github/workflows/container.yml b/.github/workflows/container.yml index 63b01ce..7c70bac 100644 --- a/.github/workflows/container.yml +++ b/.github/workflows/container.yml @@ -34,9 +34,15 @@ jobs: - run: pytest -v -m "not dev_cluster" # The renderer is part of the contract: it refuses images that are not # digest-pinned, so a broken template breaks every deployment. - - run: python deploy/kubernetes/render.py deploy/kubernetes/example.env > /tmp/s3gc-job.yaml - - name: Rendered manifest must be valid Kubernetes - run: kubectl apply --dry-run=client -f /tmp/s3gc-job.yaml + - run: python deploy/kubernetes/render.py deploy/kubernetes/example.env > "$GITHUB_WORKSPACE/.s3gc-job.yaml" + # Client-side kubectl validation requires a Kubernetes API server for + # OpenAPI discovery. Kubeconform validates the rendered standard-resource + # manifest against pinned Kubernetes schemas without cluster access. + - name: Rendered manifest must conform to Kubernetes schemas + uses: docker://ghcr.io/yannh/kubeconform@sha256:85dbef6b4b312b99133decc9c6fc9495e9fc5f92293d4ff3b7e1b30f5611823c + with: + entrypoint: /kubeconform + args: -strict -summary /github/workspace/.s3gc-job.yaml publish: needs: test diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cea754..5d23e83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -117,8 +117,10 @@ Changes below are on `feature/kubernetes-job-runner` and not yet released. where an amd64-only image can only ever schedule on a sixth of the capacity. CI builds `linux/amd64,linux/arm64` in a single step. -- **CI validates the rendered manifest** with `kubectl apply --dry-run=client` - in addition to running the tests and the renderer. +- **CI validates the rendered manifest without a Kubernetes cluster.** Strict, + digest-pinned Kubeconform schema validation replaces client-side `kubectl`, + which attempts OpenAPI discovery against a nonexistent API server on GitHub + runners. - **CI explicitly excludes `dev_cluster` tests.** The pull-request suite stays offline even after opt-in environment-dependent coverage is added. diff --git a/CLAUDE.md b/CLAUDE.md index c2ab090..6f7e953 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -74,9 +74,11 @@ requirements: ```bash python3.11 -m venv .venv .venv/bin/python -m pip install -r requirements.txt -r requirements-dev.txt -.venv/bin/python -m pytest -v +.venv/bin/python -m pytest -v -m "not dev_cluster" .venv/bin/python deploy/kubernetes/render.py deploy/kubernetes/example.env > /tmp/s3gc-job.yaml -kubectl apply --dry-run=client -f /tmp/s3gc-job.yaml +docker run --rm --entrypoint /kubeconform -v /tmp:/tmp:ro \ + ghcr.io/yannh/kubeconform@sha256:85dbef6b4b312b99133decc9c6fc9495e9fc5f92293d4ff3b7e1b30f5611823c \ + -strict -summary /tmp/s3gc-job.yaml ``` Add a regression test in the same change as each behavior or safety fix. Cover diff --git a/README.md b/README.md index a1fa81b..a2f9321 100644 --- a/README.md +++ b/README.md @@ -280,7 +280,9 @@ cluster resource: ```bash python3 deploy/kubernetes/render.py deploy/kubernetes/example.env > /tmp/s3gc-job.yaml -kubectl apply --dry-run=client -f /tmp/s3gc-job.yaml +docker run --rm --entrypoint /kubeconform -v /tmp:/tmp:ro \ + ghcr.io/yannh/kubeconform@sha256:85dbef6b4b312b99133decc9c6fc9495e9fc5f92293d4ff3b7e1b30f5611823c \ + -strict -summary /tmp/s3gc-job.yaml ``` The unit suite does not contact ClickHouse, S3, or Kubernetes. The reserved