Catalog how the direct engine handles every settable field - #6452
Draft
denik wants to merge 120 commits into
Draft
Catalog how the direct engine handles every settable field#6452denik wants to merge 120 commits into
denik wants to merge 120 commits into
Conversation
Recreating a resource re-keys the parent but not its permissions/grants child: the child's state entry keeps the old id, DoRead echoes it back, and the planner re-proposes the same update on every deploy. The ACL itself is applied to the recreated object, so this is stale bookkeeping rather than lost access, and terraform converges -- hence the per-engine plan files. Co-authored-by: Isaac
model_with_permissions.yml.tmpl was model.yml.tmpl plus a permissions block, so the two tested the same resource twice. Invariant configs carry permissions or grants by default; a permissions-free variant is not a separate case. continue_293 now excludes the model config outright, since model permissions did not work until 0.297.0. Co-authored-by: Isaac
Moves every field of every single-resource invariant config through each ordered pair of a small value set -- absent included, so add and remove are just the pairs with absent on one side -- and records whether the plan proposed anything, the apply succeeded, and the next plan was clean. Runs in-process against CalculatePlan/Apply rather than shelling out to bundle deploy, so 5502 observations across 22 resource types take 35s instead of being dominated by bundle file sync. Permissions and grants are stripped first; a bad outcome does not fail the test, it lands in out.<resource_type>.fields.txt, and the committed goldens are what turn an SDK bump into a visible diff. Co-authored-by: Isaac
Every field of catalog.UpdateSchema is omitempty and DoUpdate forwards only the config's own ForceSendFields, so removing the comment sends an empty PATCH body and UC keeps the old value. The deploy reports success and every later plan re-proposes the same update. Co-authored-by: Isaac
The earlier approach: "# ACTION:" annotations in the invariant configs drive apply_update and apply_remote_update, which plan, apply and re-plan one field at a time through the full CLI. apply_update is now covered far more thoroughly by bundle/direct/dresources_test. apply_remote_update is not -- it drifts the remote by restoring a state snapshot, which the Go catalog does not do yet -- so this is kept for that half. Drop the commit if the remote case lands there instead. Co-authored-by: Isaac
A harness rebuilt inside a field-level subtest goes on being used by later fields, but it captured that subtest's t.Context(), which is cancelled the moment the subtest returns -- 56 observations came back as "context canceled" from the SDK rate limiter rather than a real verdict. Thread the resource-type context in instead. With those unmasked, model serving telemetry turned out to be rejected 36 times: the block only validates against a catalog and schema that exist and are writable. Skip the subtree, which needed skip keys to support a "<prefix>.*" form. Co-authored-by: Isaac
…text cloud: true was the wrong default -- a resource type should be covered everywhere, and the expensive ones still have to be covered. Replaced with slow: true, which only drops a type under -short: the PR cloud leg skips them, the nightly does not. Files that said nothing but cloud: true are gone. t.Context() is cancelled just before Cleanup functions run, so every harness destroy was issuing its API calls on a dead context and silently leaking the resource; a harness rebuilt inside a field subtest hit the same thing once that subtest returned. context.WithoutCancel keeps the values and drops the deadline. Reports move to output/, so the package directory reads as seven entries instead of thirty. Co-authored-by: Isaac
It drives CalculatePlan and Apply, so it tests the engine rather than any one resource implementation, and it belongs beside the engine instead of under dresources. The old dresources_test directory name also misapplied Go's _test package-name convention to a directory. Value libraries move to testdata/fields, which the go tool ignores by name. Goldens stay in output/. Every file in the package is a _test.go, so GoFiles is empty and none of this reaches the binary. Co-authored-by: Isaac
The committed output/<type>.txt now lists just the results worth looking at, one line each, with from and to as their own columns. output/<type>.full.txt keeps everything plus the summary and the not-covered list, and is gitignored since it moves whenever a passing row does. 304 findings out of 5442 observations. A SUPPRESSED whose reason is "empty" is not one of them: both sides were empty, so nothing the user expressed was lost. DRIFT becomes POST_DEPLOY_DRIFT, which is what it measures. Transition subtests are named absent_to_true rather than with an arrow, so a single case re-runs without shell quoting, and now -v prints the post-deploy plan behind any finding. Dropped the slow flag: every resource type runs everywhere. Renaming an app is a real operation, so it is no longer skipped for being slow. It records DEPLOY_ERROR "timed out: app is deleting", which is the finding. A per-operation deadline backs that up with a TIMEOUT verdict. Main fixed clearing a schema comment, so that test becomes clear_field: it now asserts comment converges and records that custom_max_retention_hours does not. Co-authored-by: Isaac
They were committed by accident: output/<type>.txt is the file worth diffing. Co-authored-by: Isaac
They were committed by accident: output/<type>.txt is the file worth diffing. Co-authored-by: Isaac
Slices and maps are covered two ways. The container becomes a field whose values are the config's own and that value minus its last entry, so with absent in the set one field covers adding and removing an entry as well as the whole container -- all with data the backend already accepted. Separately, a pattern like tasks[*].description is expanded against the deployed config to the indices that exist, so fields inside an element are tested like any other. A pattern with nothing behind it is reported as not covered rather than silently passing. base: in a value library seeds a block the invariant config does not declare, so its fields become reachable. A job's git_source needs a provider, a url and exactly one ref, and cannot be built up one field at a time from nothing; seeded, its skip list drops from nine entries to the two that are genuinely a different side of the same oneof. Value pairs are now walked as a single chain instead of staged one at a time. The values form a complete digraph, so one Eulerian circuit covers every ordered pair exactly once and each move starts where the last ended -- one deploy per transition instead of two, which took jobs from 381s to 233s. Walk order is shuffled from a seed derived from the field path: fixed per field so goldens stay byte-stable, different between fields so the suite is not always taking the same path. TestTransitionsCoverEveryPairInOneChain is what guarantees nothing is left for a second chain to pick up. Configs of a resource type now run in parallel, so the report is guarded. out.full.txt indents the evidence under each finding: the post-deploy plan for drift, the whole API error for a rejection. Co-authored-by: Isaac
One config per resource type -- the simplest one -- and whatever structure that config lacks is seeded through testdata/fields instead. A slice or map is a field in its own right, whose values are the config's own and that value minus its last entry, so with absent in the set one field covers adding and removing an entry as well as the whole container. Patterns like tasks[*].description expand against the deployed config, so fields inside an element are tested like any other, and what no config declares is named rather than passed over. Findings are classified until each verdict names one mechanism: UPDATE_IGNORED (write accepted, remote unmoved on two reads) separated from STALE_READ (unmoved on the first read only), COLLATERAL_DRIFT (updating this field left another one drifting -- 119 rows were one field wearing other fields' names), BASELINE_DRIFT (drifts with nothing changed), and OK_INERT / INERT_NOT_HONOURED, which check the ignore_local_changes declarations rather than trusting them. Order comes from git HEAD so a finding cannot be retried away, and the pairs are walked as one Eulerian circuit, halving the deploys. Three bugs in the harness, each of which quietly under-reported coverage: map wildcards never expanded, since splitPattern emitted the wildcard twice; the not-covered list was a union across configs, so it named fields other configs were testing; and neither a map key nor a slice element could be removed, since dyn's path parser reads brackets as indices and deletePath only handled keys. libs/testserver grows SettleAsyncImmediately, off by default so acceptance keeps exercising the waiters. The SDK's poller has a hardcoded one-second backoff with no exported way to shorten it, so one in-progress read per update cost serving endpoints more wall time than every other resource type combined: 512s to 5s. 7052 observations over 22 resource types in 76s. Co-authored-by: Isaac
The generic "x"/"y" defaults are rejected outright on a real workspace for a field that names another object, and the retention period is validated in days, so those fields reported nothing but BACKEND_ERROR on cloud. Co-authored-by: Isaac
FindStructFieldByKeyType already recursed into embedded structs, but Get and Set stopped after one level, so a field of resources.PostgresProject -- which embeds a config struct that embeds the SDK spec -- was reported as not found. Both now walk embedding recursively and track the struct that declares the field, which is also the one whose ForceSendFields governs it: an outer struct that shadows the name (PostgresProjectConfig) tracks only its own fields. Co-authored-by: Isaac
The suite now writes values through libs/structs (structaccess over the resource's own Go type, structpath for paths) and syncs the result into the dynamic tree the way a mutator does, instead of manipulating dyn.Value directly. Nothing in the package depends on libs/dyn any more. That also removes every hand-written substitute for something dyn lacked: a path parser that could hold a map key, a delete-by-path, a parent-creating walk. And it makes "absent" the distinction the API actually sees -- the zero value with the field out of ForceSendFields -- so a list element or map entry now becomes settable where it previously reported UNSETTABLE: 91 of those are gone and the suite records 252 more observations, in a third of the wall time. The order seed is a constant now rather than HEAD. Some verdicts depend on the order -- a field the API cannot clear leaves the remote holding an old value -- and committing a report changes HEAD, so a HEAD-derived seed could never validate at the commit that carried the report. Co-authored-by: Isaac
MLflow refuses to clear a description, and the update sends the field unconditionally, so a config that drops it fails on a real workspace while the fake server accepted it. readplan-permissions changed the description instead of removing it, since removing it can no longer converge. Co-authored-by: Isaac
From the AWS comparison: a skip key naming a field inside a slice
("aliases[*].id") never matched the concrete "aliases[0].id" it expands to, so the
field was tested anyway and the backend rejected every value.
Keys are now matched the way the planner matches its own field rules, which also
makes naming a block skip everything beneath it -- so the ".*" suffix is gone.
Co-authored-by: Isaac
From the AWS comparison: an alias belongs to a model version and is managed through its own API, so UC does not echo aliases on a plain GET -- which is why the direct engine passes include_aliases=false and declares the field input_only. The fake server returned them anyway, so the remote appeared to hold what the config asked for and the field's real behaviour was hidden. registered_models now matches AWS row for row. Co-authored-by: Isaac
From the AWS comparison: an experiment name is an absolute workspace path and an artifact location needs a scheme, so the generic "x" was rejected outright. The UC trace location needs its catalog and schema together, like a job's git_source, so it is seeded rather than built up one field at a time. experiments matches AWS row for row. Co-authored-by: Isaac
…on values
From the AWS comparison. A retention period is validated in days (0, or 7 to 30), so
the generic 1 and 2 were rejected. The rest need workspace state this suite does not
provision: a Delta Sharing provider and share, a connection, an external location, a
customer-managed key.
options is skipped for a different reason worth noting: UpdateCatalog rejects the
field outright ("UpdateCatalog options can not be provided"), and the engine sends it
on every update whenever the config declares it -- so seeding it made every other
field of the catalog fail its update too.
catalogs matches AWS row for row.
Co-authored-by: Isaac
From the AWS comparison: external_locations cannot deploy against a real workspace at all -- it needs a storage credential with cloud IAM behind it -- so every field reported one BASE_ERROR and the golden diverged for a reason that says nothing about the engine. The invariant suite already excludes the same config from its cloud run. A type now declares "local_only: <reason>" in its value library and is skipped on cloud; its local golden stands, since a cloud run neither confirms nor contradicts it. Co-authored-by: Isaac
Two fixes from the AWS comparison, both about the suite rather than the engine.
An alert or dashboard is created inside ${workspace.resource_path} and the backend
404s on a missing parent, which is why a real deploy runs deploy.ResourcePathMkdir
first. This suite plans and applies directly, so it now runs that step too --
otherwise the whole type reported one BASE_ERROR that said nothing about any field.
A seeded value naming tester@databricks.com only exists on the fake server, so the
base fragment is now expanded with the same $VARS the corpus configs use.
Also: a base error's evidence went through firstError, which truncates to 140
characters -- so the full report, whose whole purpose is to carry the backend's own
words, showed a cut-off message.
Co-authored-by: Isaac
…brary From the AWS comparison. An enum field was getting the generic "x" and "y", which a real backend rejects or silently ignores, so the field reported nothing about whether the engine handles a change to it -- the alerts report was full of those. Every generated SDK enum declares its own values through a Values() method, so they are read from there instead, which fixes enum fields across every resource type. The alerts library also skips what the suite cannot provision: a second warehouse or workspace folder (and the backend refuses to move an alert between folders at all), and a registered notification destination. A subscription and a run_as now name the workspace's own user. Co-authored-by: Isaac
…texts Both from the adversarial review. containerValues recorded a slice or map with its elements shallow-copied, so the pointers inside an element were shared with the live resource: editing a field under clusters.init_scripts[0].workspace then reached into the value saved for the init_scripts container's own transitions, and that container was tested against corrupted data. A plan the caller never applies was dropping its pendingApply on the floor, leaking the operation context and its ten-minute timer -- thousands of them on a cloud run. Such a caller now goes through readPlan, and the two paths that plan and then return early cancel explicitly. Co-authored-by: Isaac
…_REACHED From the adversarial review: a transition assumed its setup deploy established the starting value, checking only that the API did not error. When the API accepts a write and ignores it -- clearing a field it refuses to clear -- the field kept its old value and the move was then recorded under a label describing something that never happened: "absent to 168" while the remote still held 720. The starting value is now confirmed before the move under test. A transition that cannot be set up is retried once on a fresh resource, since a resource created without the field genuinely starts absent, and only then recorded as START_NOT_REACHED -- its own verdict, because unlike BASE_ERROR nothing was rejected. Also from the review: isRequired compared a concrete path against the generated required-field data, which is keyed by pattern, so a required field inside a slice (tasks[*].task_key) looked optional and got an absent transition no user can deploy. Co-authored-by: Isaac
Both from the adversarial review. The committed report listed only findings, so a change in passing behaviour was invisible: a field that starts being recreated instead of updated moved one OK to OK_RECREATE and nothing in the compared file mentioned either. That matters most for the cloud run, which is held to the same golden -- a real workspace behaving unlike the fake server in a passing row could not fail the test. The counts of every verdict are now part of the committed file. A missing golden was also treated as an empty one, so adding a resource type without generating its report, or deleting a report, silently passed. Co-authored-by: Isaac
… lists From the adversarial review: a field that ended up with no transitions disappeared without a trace -- no verdict, no coverage gap. Two shapes did this. A type with no generic value (an `any` field like serialized_dashboard) got none, and a required field the library gives a single value has no second value to move to and no absent to move from, so volumes catalog_name, schema_name and volume_type ran nothing while counting as covered. Both are reported as not covered now. While there: a list of scalars the config leaves empty had nothing to trim, so the whole field and everything under it was a gap. Two elements of its own type need no invention and cover adding, growing, shrinking and removing -- 75 gaps closed and 444 more observations. Co-authored-by: Isaac
From the adversarial review, which found the verdict documented but never produced. I first implemented it as "the path does not exist in the adapter's state type", and it was unsound: secret_scopes renames its fields in PrepareState (name to scope, backend_type to scope_backend_type), so three fields that deploy fine -- OK_RECREATE on every transition -- were relabelled as never reaching the API. A name-identity check is not a reachability check. There is no sound way to derive this from the types: what makes an alert's file_path invisible is that the engine reads the file during initialize and its contents become other fields, so the path itself is genuinely absent from the state and NOT_OBSERVABLE is accurate. Documented that case there and removed the verdict. Co-authored-by: Isaac
From the AWS comparison: an AZURE_KEYVAULT scope is backed by a real vault, so the API rejects one created without its metadata. The fake server accepted it, so the suite reported the backend type as freely settable. secret_scopes now matches AWS row for row. Co-authored-by: Isaac
Reading a row's field out of a fixed-width column was wrong: a pipeline's ingestion_definition paths reach 130 characters, so the column held a truncated path matching nothing, and the row vanished from both sides of a sampled comparison -- the one thing that comparison must never do. Matched by prefix now, which cannot truncate. Dropping every COLLATERAL_DRIFT row from a sampled comparison was too broad. The row is dropped because its cause is a field this run may not have sampled, so now only those are: a sampled field turning from OK into COLLATERAL_DRIFT is a regression like any other. oneLine truncated by byte index, and the ellipsis it inserts for an elided URL is multi-byte, so a cut could land inside a rune and put invalid UTF-8 in a golden. By rune now, with a test that fails on the old code. Two findings are declined, with reasons. The fake server is still weaker than the warehouse API in ways the catalog does not exercise -- a whitespace-only name, min_num_clusters above max, an edit setting max to 41 -- and modelling those would be speculative: the three that are modelled are the three a cloud run actually rejected. And the reviewer would rather the postgres types were made to work on cloud than declared local_only; that means provisioning a project and branch per run and modelling a soft delete whose whole effect is to make the local run fail too. The report says what blocks them in the API's own words, which is the honest state. A sampled run still cannot see a field move between two passing verdicts, since the compared golden holds only findings. That is documented in the README rather than fixed: catching it needs the full report committed, which is the thing this design deliberately does not do. Co-authored-by: Isaac
The generic integer values 1 and 2 are both refused: "The cluster autotermination time cannot be less than 10 minutes." The fake server stored them, so all six rows for the field were measuring the fake and nothing else. Values are 10 and 20 now, and the fake applies the range on create and on edit, so the constraint holds locally too -- verified by putting 1 back and watching it fail. Zero is also accepted and means never, but the CLI defaults the field to 60, so 0 is what a user writes to turn autotermination off rather than a second ordinary value. Every acceptance config already uses 30 or 60, so nothing there changes. The type's remaining cloud failures are this workspace's capacity -- INSTANCE_GROUP_MAX_CAPACITY_REACHED from the instance pool the fixture references -- which is why clusters is still not marked local_only: one workspace running out of instances is not a property of the suite. Co-authored-by: Isaac
The jobs cloud run found the same shape nine times over, each in the API's own words: "Invalid trigger settings. One of 'schedule', 'trigger' or 'continuous' can be set." "Field 'new_settings.trigger.table_update.table_names' is required, expected non-empty collection!" "Missing required field: new_settings.trigger.model.condition" "dependencies and java_dependencies cannot be provided at the same time" "Sparse checkout pattern must a valid file path" "Node type x is not supported." "S3 cluster log destination is provided without region" trigger is a choice -- file_arrival, table_update, model, periodic -- and base seeds periodic, so the other three members cannot be reached by moving a leaf inside them; covering those needs a second fixture per member, which is a coverage change rather than a fix. The rest are blocks the API accepts only whole, or that hold cloud-specific node type ids the suite has one variable for. 136 of the 377 diverging rows were these, all of them recording what the fake server allows rather than what a workspace does. They are SKIPPED with the reason now. Co-authored-by: Isaac
…the seeded one An init script entry carries exactly one of workspace, volumes, s3, abfss, gcs, dbfs or file, and both the pipelines and clusters fixtures seed the workspace member -- their own comments say so. The suite was then setting leaves inside the sibling arms, which leaves an arm with no destination of its own. The backend accepts that and ignores it; the fake server stored it and echoed it back, so 80 rows across the two types read OK where a workspace ignores the write entirely. Skipped with that reason, which also subsumes the narrower s3.kms_key skip each fixture carried. Covering an arm needs a fixture that seeds it, the same conclusion as the job trigger members. clusters is the type this could not have been found on directly -- its cloud run dies on the workspace's instance pool capacity -- so the pipelines run is what exposed it. Both fixtures carry the same cluster spec, which is why fixing one without the other would have left the same latent gap. Co-authored-by: Isaac
The suite drove the 22 types that happened to have a value library and listed the other 12 in a coverage report. Now it iterates SupportedResources itself: a type the engine supports with no library is a failure, not a line in a report, so adding a resource type and forgetting the catalog cannot pass. configs.txt is gone with the report it existed for. The 12 that were missing are mostly children -- a postgres branch needs a project, a database catalog needs an instance, a vector search index needs an endpoint -- so a fixture can now declare `deps`, resources of other types deployed alongside the one under test and referenced from base the way a bundle normally does. No field of a dep is ever tested; it exists so the resource under test can. Their definitions come from the invariant configs, which already prove them deployable. Two mechanics had to give way for that. A fixture may declare `variables`, because a secret's value has to be a variable reference -- the config format refuses a literal, to keep secrets out of config files -- and the value is passed through BUNDLE_VAR_<name> the way a user would, since a default in the config resolves before that validation runs. And dollar-expansion no longer eats the bundle's own interpolation: a dotted name is a bundle reference and is restored verbatim, an undotted one is a variable the suite provides. That collision is why none of these types could be folded before. 687 new observations. Eight of the twelve are local_only with a reason -- a job run would start a real job per transition, a quality monitor needs a table with data, the postgres chain cannot rebuild a soft-deleted project name. Co-authored-by: Isaac
A subtest named after a truncated email address with a digest on the end is unreadable, and no easier to retype than absent_to_v1. A value whose own label is not already a clean short token now gets an alias -- v1, v2 -- numbered by sorted label so it does not move when the run order does, and the legend at the end of the full report says which value each one is. The report rows use the same alias, so a row and its subtest always agree. Indexed paths lose their brackets in the subtest name: tasks_0, not tasks[0]. Brackets are a character class to the test filter regex, so a filter copied out of the test output quietly meant something else -- the old name matched with the index read as a one-character class. Co-authored-by: Isaac
…s the variables "cannot create the resource holding it" left the reader to work out what "it" was: the starting value the transition needed. The row's own from column names the value, so the detail just has to say that is what failed. Also fixes the variables loop from the previous commit, where the context was shadowed inside it, so BUNDLE_VAR_<name> never reached the deploy. The secrets fixture passed anyway -- the validation that needs the reference runs before resolution -- which is exactly how a silent one of these survives. Co-authored-by: Isaac
apply_update and apply_remote_update, apply_mutation.py, state_snapshot.py, the "# ACTION:" annotations they read, and verify_no_drift.py's --expect-change: all of it existed to move one field per config through the acceptance harness, which is what bundle/direct/autotest does now, exhaustively and without a golden per case. Keeping both would mean maintaining two answers to the same question. The invariant suite keeps everything else, no_drift included, and still passes. The configs keep the fields the annotations happened to sit on -- postgres_endpoint's suspend_timeout_duration is there because an SDK duration used to look like an empty struct to isEmptyStruct (#6377), which is worth saying whether or not anything mutates it. Co-authored-by: Isaac
…ed it "Phase: load" under an unrelated transition was the puzzle. The phases run once per harness -- one occurrence for clusters' 412 transitions -- but libs/log's default handler writes to stderr, which belongs to no subtest, so a harness built by one resource type appeared beneath whichever parallel type had printed last. Routed through t.Log: the framework attributes it, and a passing run prints nothing at all instead of a hundred stray INFO lines. Nothing is lost -- the same lines appear under -v, and under the test that made them. Co-authored-by: Isaac
Three acceptance-side changes that came along for the ride, moved to a follow-up note instead: - postgres_endpoint's suspend_timeout_duration, added as a mutation target. It is independently useful -- the catalog cannot cover a duration at all, so this is the only place one is deployed and the only guard for #6377 -- which is exactly why it belongs in its own change rather than here. - schema's comment, also a mutation target, and redundant: the catalog covers the field, and this branch adds an acceptance test for clearing it. - folding model_with_permissions into model, which existed so the catalog would not drive two configs of one type. The catalog no longer reads the invariant corpus, so the reason is gone. The invariant configs are now identical to main. Co-authored-by: Isaac
Both document real engine behaviour the catalog found -- a schema field that never converges when cleared, and a recreate that leaves the grants child keyed to the old id -- and both stand on their own without the catalog. Recorded in the follow-up note with the diagnosis, and recoverable from this branch's history, so nothing is lost by landing them separately. What is left in acceptance/ is only what the testserver fidelity fixes force: the bind fixture, the Azure KeyVault scope case, the serving-endpoint notifications goldens, and the model description edit. Co-authored-by: Isaac
Removing my own clear_field test took its sibling with it: managed_properties is an upstream test and was never part of this branch. Co-authored-by: Isaac
…data The Azure KeyVault case declared "keyvault_metadata", which is the bundle schema's name for it; the raw API takes "backend_azure_keyvault". The CLI said so -- "Warning: unknown field: keyvault_metadata" -- and dropped it, so the request had no metadata in it at all. That went unnoticed while the fake server accepted a KeyVault scope without metadata. Once it started requiring it, as the API does, the create failed and the script aborted with it: the delete and the case after it never ran. With the right field name the create succeeds again, the whole script runs, and the last case reaches the API and gets its real rejection. Co-authored-by: Isaac
Three files moved under the branch. One needed a real decision: structaccess/get.go -- main replaced .Interface().([]string) with reflect.TypeAssert in code this branch had already deleted, so the branch's version stands and main's form is applied to the three sites the branch still has. Consistent with the same change elsewhere in libs/structs. Rebasing was the wrong shape here: the conflict lands in a function the branch rewrites several times over, so it recurs commit by commit with no intermediate state worth resolving. One golden moves, in the branch's favour. #6441 masks a Lakebase map field as a whole rather than per entry, which is exactly the bug the catalog had recorded: four postgres_projects custom_tags rows go from BASE_ERROR to OK. Co-authored-by: Isaac
#6443 masks a Lakebase oneof member under its group name, and #6089 adds ./task doctest. The first lands on rows this catalog was already recording as errors: nine of them, across postgres_projects, postgres_branches and postgres_endpoints, become OK or SUPPRESSED. Which is the point of the exercise -- those rows named the bug in the API's own words ("Unknown field path in update_mask") and now name its absence. Co-authored-by: Isaac
email_notifications was in the serving endpoint's base for no reason: the leaves are reachable without it, because the suite grows the container itself. Measured -- 32 field rows either way; the only difference was one BASELINE_DRIFT row. Seeding it made the endpoint drift from the moment it existed, and every other field for that type was then measured against a resource already pending. The behaviour it exposed is real and stays recorded, in acceptance/bundle/resources/model_serving_endpoints/update/email-notifications, where the create is shown not applying the notifications and the update applying them. Also fixes eleven comments across seven fixtures that still explained themselves in terms of "the corpus config". The catalog stopped reading the invariant corpus when the fixtures were folded, so those sentences described a mechanism that no longer exists -- including a skip whose stated reason was that a corpus config "is not the simplest one", a rule that no longer exists either. Co-authored-by: Isaac
…rest output/ carried both the goldens and the full reports, with the latter gitignored by pattern -- so 34 tracked files sat among 91, and telling them apart meant knowing the pattern. Now output/ is exactly the 34 goldens and logs/ is ignored wholesale, which is a rule rather than a pattern to remember. The full report is written straight to logs/<type>.full.txt, and its previous copy is kept as .backup: reading what a change did to a report means diffing the two, and the full report is not in git to diff against. Co-authored-by: Isaac
…not testable
The cloud run could not create a secret at all: "Cannot create 1 Secret(s) in Schema ... (estimated
count: 100, limit: 100)". The limit is per schema and main.default on a shared workspace is full of
other runs' leftovers, the oldest from three weeks ago. The fixture now creates its own schema as a
dep, which is empty by construction.
That exposed the next layer: catalog_name and schema_name name objects that have to exist, so the
generic x and y are refused ("Catalog 'x' does not exist", "Schema 'main.x' does not exist"). The fake
server takes any string, so those four rows had only ever measured the fake. Skipped with the reason.
secrets now matches on cloud.
Co-authored-by: Isaac
… and a clear the API drops
budget_policy_id names an account-level policy ("Failed to validate budget policy x"), which a test
workspace does not have and cannot be given from here. Skipped, as on apps.
target_qps is omitempty, so a config that clears it drops it from the body and the backend keeps the
value it had. The fake server was assigning the zero value instead, which made the field look freely
clearable; two rows now record the clear being ignored, as the cloud run does. Setting and changing
the value still works on both.
Co-authored-by: Isaac
…suite You were right that a poller was the cost. The fake server returns RUNNING on the first poll of a run and the terminal state on the second, and the engine's waiter uses the SDK's backoff -- attempt times a second, plus jitter -- so every run costs about a second of sleeping. A job run is immutable, so 109 of its 131 observations recreate it: roughly 110 runs, roughly 110 seconds. job_runs was 183s of the suite's 185s. SettleAsyncImmediately already existed for exactly this and job runs did not consult it. Now they do: the run is terminated and reported terminal on the first poll. job_runs 183s -> 2s, the whole suite 185s -> 11s. Verdicts are unchanged, which is the point -- the wait was pure latency. The acceptance suite leaves the simulation on, so the waiter and its state logging stay covered there. My first attempt at this made it slower, 183s -> 261s, because it skipped the whole block including the call that terminates the run: the run then stayed RUNNING forever and every create waited out the cap instead of one backoff. Measured both ways before believing either. Also adds a header to each committed report, since it is read in a diff with nothing around it to say what the columns are or that a bad outcome here is recorded rather than failed. No counts, times or dates in it: a header that moved when something unrelated changed would be noise in every review. Co-authored-by: Isaac
… the backend keeps
Removing email_notifications from base cost more than the drift row it saved: with no value to clone,
the container's values came from the generic slice defaults, so the suite sent one- and two-element
lists of "x" and "y" and the API refused all 22 rows ("Invalid email format: x"). The list needed its
own declared values, not just the address inside it.
Declared alphabetically, because the backend returns the list sorted. The first attempt used
[notify, alerts] and every two-address list read back as [alerts, notify], so the endpoint drifted on
the ordering alone, for good -- a real property, and one that makes the field untestable if the
fixture fights it. Written down where the values are.
Co-authored-by: Isaac
…to be real
The clusters cloud run failed at the create: "Init scripts failed ... Tree node with path
/Users/<user>/init.sh does not exist". Three fixtures named that path -- clusters, jobs, pipelines --
and nothing ever created the file; the mock server accepts any path, so it had never mattered. They now
point at a real script the bundle uploads.
It lives in the suite's own testdata/data, not the shared invariant data directory: everything there is
uploaded by every invariant config too, so putting it there changed "Files: 2 uploaded" to 3 across
that whole suite. Found by running it.
policy_id has to name a cluster policy that exists ("'x' is not a valid cluster policy ID"), so its six
rows had only ever measured the fake. Skipped with the reason.
Also stops the previous commit's .backup files landing in output/, which is meant to hold only what is
committed -- and had duly committed 34 of them. They belong beside the logs they are backups of, where
git is not already the history.
Co-authored-by: Isaac
… be real
The clusters cloud run failed at the create even after the previous commit pointed the init script at
the file the bundle uploads. The reason is in the harness's own doc comment: this suite deliberately
uploads nothing, because a sync per permutation would dominate the run. So no destination a fixture
could name is ever there, and the mock server's acceptance of any path is the only thing that made the
field look testable. init_scripts is skipped in all three fixtures that seeded it, with that reason,
and the data directory the previous commit added is gone with it.
user_api_scopes needed real OAuth scopes: the API refuses anything else outright ("The specified scope
x is not a valid scope"), and the mock server takes any string, so eight rows had only measured the
fake. sql and dashboards.genie both verified against the API, declared for the list as well as the
element.
Co-authored-by: Isaac
…s reason The cloud runs found 400 diverging rows across jobs, pipelines, apps and clusters, and every one is a field the mock server accepts and a real workspace does not. Skipped, one entry per field or block, with the API's own words in the comment above it. Two shapes: Rejected outright -- a cross-field constraint (a job's continuous block against the seeded trigger, an environment spec that needs exactly two of three, a shared job cluster needing more than one task), a value format (metadata and notebook paths must be absolute, a cluster label must be default or maintenance, a webhook id must be a UUID), or a field that does not apply to the resource this fixture builds (is_single_node and use_ml_runtime need a cluster kind; an event log needs a UC pipeline). Accepted and then ignored -- jobs' email_notifications, edit_mode, format, health.rules, max_concurrent_runs, trigger.pause_status; pipelines' dry_run, maven libraries, some cluster attributes. These are the interesting ones: the API takes the write and drops it while the mock server applies it, so a local row said OK where a workspace says nothing happened. Modelling each in libs/testserver is the real fix, and the skip reason records the finding until then. vector_search_indexes keeps one field and skips the rest. Every field of an index is immutable, so each transition recreates it behind an endpoint that has to be provisioned first: 102 rows took three hours and then hit the test timeout. primary_key is the cheapest that still means something -- required, so two transitions rather than six, both through create, recreate and converge. genie_spaces needed no change. Its title rows diverged because the backend changed mid-sweep: the failing run saw a space default-named New Agent and refused to rename it, and a probe an hour later got New Agent with a timestamp and renamed it fine. It passes on a re-run. Co-authored-by: Isaac
…ce said jobs went from 199 diverging rows to none: - email_notifications was seeded in base and the create does not apply it, so the job drifted from the moment it existed and two rows recorded that rather than a field. Removed from base; the field was already skipped for transitions, so seeding it bought nothing else. - clearing the name does not converge. The CLI defaults a nameless job to Untitled, the remote holds that, and the plan keeps proposing an update; the mock server stores the cleared value and settles. The field's other five transitions are fine on both. pipelines went from 161 to none: notification alerts and recipients are validated (Invalid ... notifications: x), serverless cannot be set on a pipeline that declares clusters, the name cannot be cleared, a parameter is accepted and not applied, and the autoscale mode's drift is attributed to num_workers, which cannot move with it. sql_warehouses had one row: a new warehouse is STARTING rather than RUNNING, so a config asking for started: false is already satisfied on cloud and suppressed, where the mock server plans a stop. Modelling it needs the mock server to move a warehouse from STARTING to RUNNING across reads, which the engine's waiter depends on, so this one is timing rather than behaviour. Co-authored-by: Isaac
The autotest package is already in the three integration tasks, with no -sample, so a PR's integration run was attempting the full cloud sweep inside a 2h timeout -- and the full sweep takes longer than that. Sampling by default fixes it; running everything has to stay possible, which is what this marker is for. A cloud run now tests two fields per type unless the commit title contains AUTOTEST_ALL, and every field when it does. Locally nothing changes: a full run is 11 seconds, so there is nothing to save, and ./task test keeps comparing the committed goldens in full. -sample still overrides both, and the nightly task passes -sample 0 explicitly now that the cloud default is not "all". Co-authored-by: Isaac
A commit whose title carries the marker, so the gate is exercised the way a PR would use it: this commit's own integration run should drive every field rather than a sample. Co-authored-by: Isaac
Down from 51 diverging rows to none: - schema is refused on a storage-backed pipeline, which base declares; the API points at its migrate-to-dpm documentation. - both autoscale bounds have their drift attributed to num_workers, which base sets and no transition moves with them -- the same reason autoscale.mode was already skipped. - parameters was seeded in base and the create does not apply it, so the pipeline drifted from the moment it existed. Dropped from base; the field was already skipped, so seeding it bought only that drift row. Same shape as jobs' email_notifications. jobs is clean on cloud now, from 199 diverging rows to none. Co-authored-by: Isaac
CI has been red since the commit that taught libs/testserver the SQL warehouse create validations: TestAll/sql_warehouses builds a resource from the zero value, and a warehouse with no name is now refused, as the API refuses it. The fix belongs in the test, which has a per-type table for exactly this: a name, a cluster_size and max_num_clusters, the three the API requires. I should have caught this when the validations landed. I ran the field catalog and the acceptance suite and not the package whose own unit test creates every resource type from nothing. Co-authored-by: Isaac
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
1 participant
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.
Adds
bundle/direct/autotest: a per-field catalog of how the direct engine handles every field a usercan set. For each field it walks every ordered pair of a small value set —
absentincluded, so addingand removing a field are just the pairs with
absenton one side — and records three things per move:did the plan propose a change, did the apply succeed, did the next plan converge.
The outcome is a verdict word in a committed golden. A bad outcome is recorded, not failed, so the
report is a map of the engine's behaviour rather than a wall of failures; what fails the test is a
verdict changing. All 34 resource types the engine supports, ~3900 observations.
Both a local run against
libs/testserverand a real-workspace run are held to the same golden, so adivergence means the fake server does not match the API. That is where most of this PR's findings come
from — 14 fake-server fidelity fixes, each carrying the API's own error text:
cluster_size, andmax_num_clustersin 1..40, and thebackend defaults
enable_photonandspot_instance_policycreated with neither gets storage, and the later update is then refused
email_notifications; an update doesautotermination_minuteshas a 10-minute minimumtarget_qpson a vector search endpoint is omitempty, so the API drops a clearThree of those exposed acceptance fixtures asserting behaviour the API does not have. All were
Cloud = false, so nothing had ever checked them:bind/pipelines/updatebound a storage-defaultedpipeline to a config asking for a catalog;
create-scopesent the bundle schema's field name to a rawAPI call, so the CLI dropped the metadata and the whole script aborted once the fake required it; and
the serving-endpoint fix sharpened a test whose own comment describes a TF bug the fake had been hiding
half of.
Engine findings the goldens record: 27
postgres_projectsrows name a field inupdate_maskand thenomit it from the body (#6441 and #6443 fixed 13 of these while this branch was open);
pipelines.catalogis absent fromrecreate_on_changes, so the engine sends an update the API refuses.Running it
./task testcovers the local run. Two new tasks for the real-workspace runs:./task autotest-cloud— every field; nightly./task autotest-cloud-pr—-sample 2, for PRs: two fields per type, picked from HEAD sosuccessive commits cover different ground while one run's picks follow from its SHA
output/holds the 34 committed goldens and nothing else;logs/holds the full report per type andis gitignored wholesale.
State of the cloud verification
In progress. 14 of the 34 types declare
local_onlywith a reason — an external location needsIAM-backed storage, an instance pool cannot be deleted again, a postgres project holds its name after
deletion, a job run would start a real job per transition. Of the 20 that can be driven, verification
against the current goldens is under way; each one that diverged so far has been a fixture value the
fake accepts and the API refuses, or a fake-server gap, and is fixed in this branch.
Test-only; no changelog fragment.
This pull request and its description were written by Isaac.