fix(legacy_openedx): keep the code location loadable when Vault is down - #2526
Open
blarghmatey wants to merge 3 commits into
Open
fix(legacy_openedx): keep the code location loadable when Vault is down#2526blarghmatey wants to merge 3 commits into
blarghmatey wants to merge 3 commits into
Conversation
`_job_default_config` returned `{}` when Vault could not be reached at
code-location load time, and that empty mapping was handed straight to
`to_job(config=...)`.
`config={}` does not mean "no default". Dagster treats any mapping as a
default run config and validates it against the job's schema, so an
empty one fails with
Missing required config entries ['ops', 'resources'] at the root
The validation runs while the repository is being constructed, so the
error escaped as an unhandled exception in the gRPC server process --
`mechanism: excepthook`, `handled: no` -- taking down the entire
legacy_openedx code location: all three jobs and all three schedules,
not just the residential one named in the message.
The function exists precisely to degrade gracefully when Vault is
unavailable, so the fallback was defeating its own purpose. Returning
`None` skips the default-config validation entirely and leaves the
launchpad unpopulated, which is the intended degraded behaviour.
Reproduced against the unfixed code with Vault pointed at a closed
port: `defs.get_repository_def()` raises the DagsterInvalidConfigError
above; with the fix it returns all 3 jobs. Both cases are covered by
the new legacy_openedx_tests/test_definitions.py.
Fixes DAGSTER-F
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ssj5MFfDB33GBZod1PYst9
Contributor
There was a problem hiding this comment.
Pull request overview
Keeps the legacy_openedx Dagster code location loadable when Vault is unavailable.
Changes:
- Returns
Noneinstead of an invalid empty default configuration. - Adds regression tests for degraded repository loading.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
legacy_openedx/definitions.py |
Corrects graceful default-config fallback behavior. |
legacy_openedx_tests/test_definitions.py |
Tests repository loading without Vault. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ed one The fixture set environment variables and then imported legacy_openedx.definitions, but everything it was setting up happens at import time. A cached sys.modules entry would hand back a module built under whatever environment imported it first, so the second and later tests in a session asserted against stale module state and the suite passed or failed on import order rather than on behaviour. Evicts the module from sys.modules via monkeypatch, which also restores the previous entry on teardown so the eviction cannot leak into other tests. Adds a test that imports twice and asserts the two module objects differ -- it fails with `assert <module ...> is not <module ...>` when the eviction is removed. Raised by Sentry review on #2526. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ssj5MFfDB33GBZod1PYst9
The previous commit fixed the module-cache problem for legacy_openedx.definitions but not for its dependency. ol_orchestrate.lib.constants resolves DAGSTER_ENV and VAULT_ADDRESS from the environment at import time, and definitions imports those *values*, so evicting only the leaf let it rebind to constants resolved by whatever imported them first. The consequence is worse than a stale value: VAULT_ADDRESS would fall back to the real vault-qa host, turning an immediate connection refusal into a network timeout and quietly no longer testing the degraded path at all. The suite passes today only because nothing in this test session imports constants before the fixture does -- exactly the order dependence being removed. Adds a test that primes the cache with constants resolved against a different address first, the way any earlier import would, then asserts the module under test still sees the closed port. It fails with `- http://127.0.0.1:1 / + https://vault-qa.odl.mit.edu` when constants is dropped from MODULES_TO_EVICT. Raised by Sentry review on #2526. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ssj5MFfDB33GBZod1PYst9
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.
What are the relevant tickets?
Sentry: DAGSTER-F
Description (What does it do?)
Part of a set of PRs working through the Sentry backlog from #2518.
This issue logged only one event, which badly understates it:
mechanism: excepthook,handled: no, on thedagster-user-code-...-legacy-openedxpod. It is not a step failure — it is the gRPC server for the whole code location failing to start._job_default_configreturns a default run config so the launchpad is pre-populated for ad-hoc runs, and is written to degrade gracefully when Vault is unreachable at load time. On that degraded path it returned{}:But
config={}does not mean "no default". Dagster treats any mapping as a default run config and validates it against the job's schema, so an empty one fails:That validation runs inside
build_caching_repository_data_from_list→job.partitions_def→_resolve_configs, i.e. while the repository is being constructed. So the fallback meant to keep the location alive was the thing killing it — and it takes down all three jobs and all three schedules, not just the residential one named in the message.Returning
Noneskips default-config validation entirely and leaves the launchpad unpopulated, which is the degraded behaviour the function was written to provide.How can this be tested?
Reproduced directly, with Vault pointed at a closed port so
authenticate_vaultfails fast:DagsterInvalidConfigError: ... missing required fields or contains invalid entriesrepository OK: 3 jobsNote the failure does not reproduce on plain
import— it needs the repository construction step, which is what the gRPC server does at startup.Both cases are covered by the new
legacy_openedx_tests/test_definitions.py(this code location had no tests before):pre-commit run --files <changed>is clean.Additional Context
Worth confirming after deploy that the
legacy_openedxlocation is actually up in production — the single Sentry event suggests it crashed once rather than crash-looping, which would mean it came back when Vault recovered and this is a latent trap rather than a current outage. Themitxonline_edx_course_pipelinerun in DAGSTER-E four minutes earlier indicates the location was healthy immediately before.