Repository navigation
Add mantid-python-script-lsp to staging - #300
Merged
Merged
Conversation
New component (base + envs/staging) and its ArgoCD Application, staging only. This is the service the editor's Python squiggles come from: it reports syntax errors that would stop the runner and names that are not part of Mantid. The image digest was verified in its final container before pushing -- /ready goes 200 in under 8 s having run a real algorithm, and /analyse flagged a mistyped framework name at the correct columns. - The Service is named mantid-python-script-lsp-service in the fia namespace because that is FIA-API's default SCRIPT_LSP_URL, so the two connect with no wiring in the fia-api manifests. Renaming it or moving namespace needs an explicit SCRIPT_LSP_URL. - No HTTPRoute: the only caller is FIA-API, and the service has no authentication of its own, so it stays inside the cluster. - startupProbe on /ready with a 60 s ceiling, liveness on /healthz. Liveness must not consult the framework, or a pod that warms slowly on a cold node is restarted for it and slow starts become restart loops. - 2 replicas, 1 CPU/1Gi requests against a 4 CPU/2Gi limit: 8 CPUs of ceiling across the deployment, burstable rather than reserved, since steady state is 260 MB RSS and a check on a cap-sized script costs ~25 ms. The CPU limit is also what sets concurrency -- the service defaults to one analysis process per core of the pod's cgroup quota, four here -- so ANALYSIS_WORKERS is left unset rather than pinned to a second number that would have to track the limit by hand. - MAX_SCRIPT_BYTES is set explicitly to FIA-API's value: if the two caps disagree, a script FIA-API forwards gets refused downstream and the editor shows a refusal the user cannot act on. - No envs/production and no prod Application yet: this has to soak on staging first. There is no variants/ layer either, because nothing currently differs between environments; when production arrives and needs its own replica count or limits, that is where they go, per the golden rule in AGENTS.md. - The app-of-apps recurses over apps/, so the Application needs no registration entry.
TEMPORARY, and the one commit here that must not be promoted. The script editor validates through FIA-API rather than the analysis service directly, so testing it needs a FIA-API build that has the route. The digest is FIA-API c8bfb58 plus the route commits -- c8bfb58 is exactly the commit behind the digest this file held, so this is a fast-forward and reverts nothing. Why it must not be promoted: prod_release.yml copies envs/staging/fia-api.yml straight over envs/production/fia-api.yml on every push to main, so while this stands, the staging_to_prod PR would offer a branch build to production. Once the PR merges, that service's own CI rewrites this line with a main build and the warning goes away with it. Revert this commit if the review would rather staging stay on a main build.
Raised in review of the mantid-python-script-lsp staging change, where a first draft carried explanatory comments through the Deployment, Service, kustomize overlays and Application. Removed them all, and wrote down the rule so the next change does not put them back. Said as "mostly", because an absolute would be wrong about this repo -- 61 of the 295 manifests under components/ and apps/ carry a comment -- and the ones that survive are the kind worth keeping: a value that does something its own key does not say, like `request: 0s` in the plotting-service HTTPRoute meaning "no ceiling" rather than "instant". The reasoning worth keeping is that a manifest's structure already carries what it is and where it runs, so prose about it costs more than it explains, and that a comment in this repo about another repo's behaviour is the first thing to go stale -- the cross-service constraints belong in the repo that owns the value, and the "why" of a particular digest or replica count belongs in its commit message.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
kube-linter found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
The PR process told an agent to ask the user when no issue number was evident, which reads as though a PR without one is incomplete. It is not: this repo's main history is mostly machine-generated [CD] bumps, and component work is normally raised in review rather than filed first. Point at where the trail actually goes instead -- sibling PR links in the description, or the Closes # line saying there is no issue -- and rule out the two tempting ways to satisfy the template: inventing an issue, or attaching an unrelated one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes # (no issue filed for this work)
Part of adding Python validation to the script editor, across three repos:
fiaisis/mantid-python-script-lsp#1 (the service itself) and
fiaisis/FIA-API#717 (the
POST /script/analyseroute the editor calls).Description
Deploys mantid-python-script-lsp to staging: the service that backs the Python squiggles in the
script editor. It reports syntax errors that would stop the runner before it starts, and names that are
not part of the Mantid framework.
Merging this to
mainis what deploys it — ArgoCD syncsapps/automatically.New
components/mantid-python-script-lsp/{base,envs/staging}andapps/mantid-python-script-lsp/:Servicenamedmantid-python-script-lsp-servicein thefianamespace, which is what FIA-API'sdefault
SCRIPT_LSP_URLresolves to, so the two connect with nothing to configure on the FIA-API side.httproute.yml: the only caller is FIA-API, and the service has no authentication of its own.startupProbeon/ready(60 s ceiling),readinessProbeon/ready,livenessProbeon/healthz.Liveness deliberately does not consult the Mantid framework, so a pod that warms slowly on a cold node
is not restarted for it.
deployment. The CPU limit also sets concurrency: the service defaults to one analysis process per core
of the pod's cgroup quota, four here.
envs/production,variants/and a prod Application are deliberately absent — this soaks on stagingfirst, and nothing currently differs between environments.
Changes
components/fia-api/envs/staging/fia-api.ymlto a digest containingPOST /script/analyse,which the editor calls rather than the analysis service directly. It is FIA-API
c8bfb58plus the routecommits, and
c8bfb58is exactly the commit behind the digest this file held, so it is a fast-forwardthat reverts nothing. This is the one line here that must not be promoted:
prod_release.ymlcopiesthis file straight over
envs/production/fia-api.yml, so while it stands thestaging_to_prodPR wouldoffer a branch build to production. Merging fiaisis/FIA-API#717 replaces this digest with a main build
automatically; please do not merge
staging_to_prodbefore that happens.Documents in
AGENTS.mdthat manifests here are mostly not commented, from review feedback on thischange.
Verified before pushing
Both images were run as containers first: the analysis service reached
/ready200 in 7.8 s havingcreated and deleted a real workspace, and
/analyseflagged a mistyped framework name at the correctcolumns. FIA-API boots with no database, registers
/script/analyse, and returns 401 unauthenticated.ghcr.io/fiaisis/mantid-python-script-lsp@sha256:c83f2b01f2113c5010a4c1e207ee4dab645ee681c61c46406dfed113d03704e8ghcr.io/fiaisis/fia-api@sha256:91be6c4b03a0a5fb8a718f2a8d02d1b3e6cf82d7b327890ff066dfbae0fb3a8fkubectl kustomizerenders cleanly forcomponents/mantid-python-script-lsp/{base,envs/staging}andcomponents/fia-api/{envs/staging,envs/production}, and production still renders its own FIA-API digest.kube-linteris not installed locally, so the CI SARIF scan is the first pass on it.