Skip to content

Migrate runs into the beamline/proposal/session tree - #22

Merged
ZohebShaikh merged 8 commits into
mainfrom
node-structure-migration
Oct 20, 2026
Merged

ZohebShaikh merged 8 commits into
mainfrom
node-structure-migration

Conversation

@ZohebShaikh

@ZohebShaikh ZohebShaikh commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Moves runs from the flat catalog root into root/<beamline>/<proposal>/<session>/<run>, the layout blueapi now writes into, and updates the policy so it can read the tags on those nodes.

Policy (src/dls_tiled/policy.py)

  • DiamondAccessBlob now accepts beamline-only and beamline+proposal blobs (for creating container nodes); proposal is the full code, e.g. cm12345.
  • Tags that aren't a JSON blob are parsed as composite tags (beamline:i22,proposal:12345,session:12345-1) instead of raising.

Migration (database_migration/)

  • migrate_node_structure.py: upgrades the tiled schema, then moves each root run under new containers carrying the same tags tiled.rego stamps, and retags everything under it with its session tag. Runs it can't place are left and listed. Has --dry-run; safe to rerun. Both scripts carry their dependencies inline: uv run --script migrate_node_structure.py ... (tiled pinned to the runtime image's 0.2.18).
  • postgres_tools.py: read-only role creation and backup.
  • README.md: the full runbook, including restore. Restore steps say to scale tiled to zero first, otherwise it creates the schema in the empty database and pg_restore fails.
  • test_integration.py + integration/: seeds a catalog (or restores a production dump with PROD_DUMP), migrates it, and reads it back through dls-tiled with real Keycloak tokens and OPA.

Before running against prod

Prod's alembic_version is 4cf6011a8db5, but its schema already has the webhooks tables, ix_nodes_parent_id and the ragged enum value (head, e8956581ecd5). tiled catalog upgrade-database therefore fails with relation "webhooks" already exists. Stamp it first:

UPDATE alembic_version SET version_num = 'e8956581ecd5' WHERE version_num = '4cf6011a8db5';

Testing

  • tests/test_policy.py and test_migrate_node_structure.py pass (37 tests).
  • test_integration.py runs in CI (new migration job): without PROD_DUMP it seeds a small catalog in the old flat layout, migrates it, and reads it back through the compose stack. Passes locally with docker (4 tests).

🤖 Generated with Claude Code

ZohebShaikh and others added 3 commits October 16, 2026 15:42
Beamline and proposal nodes are created with only some of the access blob
fields, and existing nodes carry composite tags like
"beamline:i22,proposal:12345" rather than a JSON blob. Both now become OPA
input instead of failing validation.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Runs at the catalog root move under new beamline, proposal and session
containers, and are retagged with session tags so the new policy can see
them. Includes postgres backup, restore and read-only role tooling, and an
integration test that restores a production dump and reads it back through
dls-tiled.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Without PROD_DUMP the test now seeds a small catalog in the old flat layout,
so it needs nothing but docker compose and uv. The migration scripts carry
their dependencies inline and run with `uv run --script`, with tiled pinned
to the runtime image's version.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.38%. Comparing base (c1dcb1e) to head (5a39180).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #22      +/-   ##
==========================================
+ Coverage   95.16%   95.38%   +0.21%     
==========================================
  Files           5        5              
  Lines         766      801      +35     
==========================================
+ Hits          729      764      +35     
  Misses         37       37              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ZohebShaikh and others added 5 commits October 17, 2026 11:40
user_session now returns a node's full tag list (its own tag, then its
ancestors' grant tags), so init_node stamps all of them rather than
wrapping a single tag.

filters caches each access token's user_sessions answer, for 1 minute for
users and 7.5 minutes for service accounts (tokens without a fedid, as
tiled.rego tells them apart), so a change in authz takes up to that long to
reach tiled. A token that can't be read gets the user TTL, and a missing
decision isn't cached. The tests for both move here from blueapi's copy of
this policy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The migration now stamps the tag lists the current policy does: each
node's own tag plus its ancestors' grant tags, and visible: tags on the
beamline and proposal containers. Nodes already in the tree with only
their own tag (from blueapi under an earlier policy, or an earlier run of
this script) get the full list too.

With --authz-data (the authz bundle's data.json), nodes still tagged with
a bare numeric session id, such as runs without start.instrument_session
that can't be moved, are retagged in place from that session; otherwise
only admins could see them. Unknown ids are counted.

The retag is one join through a temp table: the GIN index on access_blob
can't serve "access_blob -> 'tags' = ...", so an UPDATE per tag scanned
nodes once per tag. The OPA in the integration stack moves to
authz-policy 0.0.27-alpha, which has the matching policy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
edge-static-debug floated with every OPA build. Pin 1.21.2, the version
authz now builds and tests its policy with; the debug variant is kept
because entrypoint.sh needs a shell.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
0.0.28-alpha is built from authz 452bd95: the same decisions as
0.0.27-alpha, with the policy written for OPA 1.21.2 (string
interpolation, so it needs OPA 1.x to load).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ZohebShaikh
ZohebShaikh marked this pull request as ready for review October 20, 2026 07:58
@ZohebShaikh
ZohebShaikh requested a review from fajinyuan October 20, 2026 07:58

@fajinyuan fajinyuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall these changes in logics are OK, but there are needs to further clarify terminology used in these. I feel very confused by proposal, proposal number, session, visit. Please see my separate comments below.
Some test codes are not in the tests folder - why?
In the future each tool should be raised as a separate PR with ticket referenced to keep PR small for reviewers to concentrate on.
I also see default password/username are used in here - why?

bundles:
diamond-policies:
service: ghcr
resource: ghcr.io/zohebshaikh/authz-policy:0.0.28-alpha

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the path conatin personal 'zohebshaikh'?

image: keycloak/keycloak:26.4
environment:
- KC_BOOTSTRAP_ADMIN_PASSWORD=admin
- KC_BOOTSTRAP_ADMIN_USERNAME=admin

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these username and password used for long running or just short run?


Before: root/<run uid>
After: root/<beamline>/<proposal>/<instrument session>/<run uid>
e.g. root/i22/cm12345/cm12345-1/<run uid>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why cm12345 is repeated here? we only need beamline/instrument-session/ or beamline/proposal/visit

CLOSURE_REVISION = "e05e918092c3"
# Same as blueapi's INSTRUMENT_SESSION_RE, e.g. "cm12345-1"
INSTRUMENT_SESSION_RE = re.compile(
r"^(?P<proposal>[a-z]{2}(?P<proposal_number>\d+))-(?P<visit>\d+)$"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is OK, but I don't understand why we have to split proposal code into proposal and proposal_number? it seems to me over-complicate things here.

)


# A node's own tag as tiled.rego's beamline_tag/proposal_tag/session_tag build it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this means i22/cm12345/1 i.e. beamline/proposal/visit?
We should be trying to keep the depth of the tree as short as possible but still need to be unique.

Currently I am confused with the terminology used. I thought proposal is 2 letter plus 5 digits, session is visit, so session code should be something like cm12345-1 i.e made of proposal plus visit.

return {"tags": [beamline_tag, f"visible:{beamline_tag}"]}
proposal_tag = f"{beamline_tag},proposal:{proposal_number}"
if visit is None:
return {"tags": [proposal_tag, beamline_tag, f"visible:{proposal_tag}"]}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why proposal need to be split into proposal tag and proposal number?

# requires-python = ">=3.11"
# dependencies = []
# ///
"""Back up a Postgres pod in Kubernetes, or add a read-only role to it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally, in the future things like these should be raised as separate PR as the tool can be reviewed stand alone!
That is what I means by committing small and frequently to reduce time required from reviewers.

to leave it up for poking at.

What it checks, for every run the migration moves:
- it is at <beamline>/<proposal>/<session>/<run> with its metadata unchanged

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plrease think about this - will i22/cm12345-1/ provide uniqueness for the path?

@ZohebShaikh
ZohebShaikh merged commit 639cd61 into main Oct 20, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants