Repository navigation
Add postgres_data_apis bundle resource - #7160
ruslantalpa wants to merge 17 commits into
Conversation
Co-authored-by: Isaac <no-reply@databricks.com>
|
Commit: Regression test reportSelected 14 changed tests (limit=20, 0 not selected) 650 tests: regression=10 unreleased=0 coverage=640 failing=0
10 more failure details omitted, see the full report in the run. |
Co-authored-by: Isaac <no-reply@databricks.com>
3be15c5 to
1a763c3
Compare
Integration test reportCommit: ab7d1a8
Top 46 slowest tests (at least 2 minutes):
|
Add the `postgres_data_apis` resource, which enables and configures the
Data API (PostgREST) of a Lakebase Postgres database.
The Data API is a per-database singleton with no ID of its own: its
resource name is "{parent}/data-api", and `parent` is a provided id
field, so changing it recreates. Create, update, and delete are
long-running operations. Updates send the full spec with
update_mask=spec, so a field removed from the config resets to its
server default.
spec is INPUT_ONLY in the API, so GET does not return it and remote
spec changes are not detected (ignore_remote_changes spec:input_only,
generated). Deleting the resource turns the Data API off; the database
and its data are unaffected.
The resource is labelled Private Preview, matching the API's launch
stage, rather than the Beta its postgres_* siblings are pinned to.
Co-authored-by: Isaac <no-reply@databricks.com>
Observed against a real workspace: - status reports the configuration in effect, server defaults included: db_aggregates_enabled=true, db_schemas=["public"], jwt_role_claim_key=".sub", openapi_mode=OPEN_API_MODE_DISABLED and server_cors_allowed_origins=["none"]. A bool that was never set and is false is omitted; once set it is reported, false included. - update_mask=spec keeps the current value of a field the request omits; it does not reset it to the default. - Errors: 400 INVALID_PARAMETER_VALUE "Field 'data_api' is required and must contain at least one subfield with a non-default value!" for an empty spec, 409 ALREADY_EXISTS "Data API is already enabled for this database", and 404 NOT_FOUND "Data API is not enabled for this database" for get, update and delete when it is not enabled. The acceptance tests now also print server_cors_allowed_origins, and the update test shows that a field removed from the config keeps its value. Co-authored-by: Isaac <no-reply@databricks.com>
GET does not return spec (it is INPUT_ONLY), so the plan could not see a remote spec change, and a bound Data API kept its old configuration until the bundle config changed. status reports every spec field in effect, so DoRead now fills the remote spec from it. The generated spec:input_only rules no longer apply, so apitypes.yml drops the type for postgres_data_apis and the field rules are hand-written: every spec field is a backend_default with no allowed values. A field the config omits is left to the server: create applies the server default, and update_mask=spec keeps the current value of an omitted field, so a field removed from the config keeps its last value. Leaf update masks would reset it, but resetting db_max_rows or jwt_role_claim_key that way leaves the Data API unable to serve requests, so the mask stays at spec. Status omits a false bool that was never set, so the remote spec forces db_aggregates_enabled and server_timing_enabled: an explicit false in the config compares against false, not against an unknown value. With this, binding an existing Data API plans an update that applies the configured spec, and an out-of-band spec change shows up in the plan. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
The real API reports the serving URL in status.url:
https://{read-write endpoint host}/api/2.0/workspace/{workspace_id}/rest/{postgres_database}.
The fake now fills it in GET and in the create and update operation
responses, from the branch's read-write endpoint host, the fake
workspace ID and the database's Postgres name.
The new url_reference acceptance test has a job parameter that references
${resources.postgres_data_apis.<key>.status.url} and shows it resolves on
the first deploy. It also records that an in-place update of the Data API
currently plans an update of the job too.
Co-authored-by: Isaac <no-reply@databricks.com>
status.url is built from the read-write endpoint host, the workspace and
the database's Postgres name. An update of the Data API changes none of
them, but without this the reference resolver delays any
${resources.postgres_data_apis.<key>.status.url} reference during an
in-place update, so every dependent got a no-op update planned too.
The url_reference acceptance test now shows that changing db_max_rows
plans an update of the Data API only, and the next plan is clean.
Co-authored-by: Isaac <no-reply@databricks.com>
The invariant config now also sets db_aggregates_enabled: false, server_timing_enabled: true, jwt_cache_max_lifetime: 300s and openapi_mode: OPEN_API_MODE_IGNORE_PRIVILEGES, so no_drift checks the status readback for a forced false bool, a true bool, a duration and an enum. Every optional field is tagged CAN_REMOVE, the bools included. Removing either bool on its own plans an update and converges: the server keeps the last value, status reports it, and the field is a backend default. Co-authored-by: Isaac <no-reply@databricks.com>
The resource's schema description was inherited from the embedded SDK
spec type ("Desired PostgREST configuration (input)."). Override it to
say what the resource is, that a database has at most one Data API,
where the serving URL is (status.url) and how durations are written
(300s).
Also override the postgres.OpenApiMode description, which mentioned a
value that may be added later, with what the two supported values do.
Co-authored-by: Isaac <no-reply@databricks.com>
The comment above the PostgresDataApi entry split gofmt's alignment block, which re-indented the unrelated PostgresBranch and PostgresCatalog lines. Make it a trailing comment, so the diff against main is only the added entry. Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
Co-authored-by: Isaac <no-reply@databricks.com>
The Integration Tests failed on AWS for every postgres_data_apis test in the DMS=true variant. Deployment history rejects the new resource-key prefix: failed to create deployment version: operations.resource_key must have a known resource type prefix (e.g. 'jobs.', 'pipelines.'): postgres_data_apis.my_data_api (400 INVALID_PARAMETER_VALUE) The testserver's dmsResourceTypes is meant to mirror the real service, so listing postgres_data_apis there hid the failure locally. Remove it, so the fake rejects the prefix the same way the real service does, and exclude DMS=true for the postgres_data_apis resource tests and the invariant config. This matches how the features resource handles the same gap. Co-authored-by: Isaac <no-reply@databricks.com>
Main split the refschema output per resource type and added lifecycle.depends_on, so add output/postgres_data_apis.txt and the lifecycle.depends_on required field for postgres_data_apis. Co-authored-by: Isaac <no-reply@databricks.com>
Main dropped the bind-wide DMS=[""] matrix now that bind works with deployment history, so the bind test started running with DMS=true. Deployment history does not accept the postgres_data_apis prefix yet, so exclude DMS=true here as the resource tests already do. Co-authored-by: Isaac <no-reply@databricks.com>
300f4d8 to
b83bc7f
Compare
|
|
||
| # The Data API needs a read-write endpoint on the branch; the "production" | ||
| # branch created with the project comes with one. | ||
| postgres_roles: |
There was a problem hiding this comment.
This is an implicit dep on the branch; please include it in the config with replace_existing to pull it in.
Same for the other acc tests.
There was a problem hiding this comment.
Done in a07dc42: all the Data API acceptance tests (basic, update, out-of-band, recreate, url_reference) and the invariant config now declare postgres_branches.production with replace_existing: true (mirroring postgres_branches/replace_existing), and the role and database reference it. The bind test keeps its literal parent, since its setup script creates the project outside the bundle.
| // so a field removed from the config keeps its last value. Leaf paths would | ||
| // reset an omitted field instead, but resetting db_max_rows or | ||
| // jwt_role_claim_key that way leaves the Data API unable to serve requests. | ||
| var dataApiUpdateMask = []string{"spec"} |
There was a problem hiding this comment.
(found by my agent) Once a field has been set, removing it from the config never takes effect. The plan still shows update and the deploy prints Updated, but the PATCH leaves the field as it was, and the next plan is clean (update/output.txt shows exactly this for server_cors_allowed_origins). For fields whose default is "unset", such as db_max_rows, jwt_cache_max_lifetime and db_extra_search_path, the user also can't write the default back (the fake ignores db_max_rows: 0 even when it is force-sent), so a row cap can never be removed through the bundle. Is that acceptable for Private Preview, or should the plan at least not report an update that does nothing?
There was a problem hiding this comment.
cc @denik
This is the same as the update mask issue with other Postgres resources. Omitting a field from the request payload (on removal) conflicts with the "presence based" update mask interpretation.
There was a problem hiding this comment.
Yes, this is intentional for Private Preview. A leaf mask would reset the field, but resetting db_max_rows or jwt_role_claim_key currently breaks serving because of a known backend issue, so we mask spec and accept that removing a field keeps its last value. I added a Badness note to the update test (ab7d1a8), like postgres_endpoints/remove_suspend_timeout. Happy to switch to whatever approach comes out of the general Postgres update-mask discussion.
| waiter, err := r.client.Postgres.CreateDataApi(ctx, postgres.CreateDataApiRequest{ | ||
| Parent: config.Parent, | ||
| DataApi: postgres.DataApi{ | ||
| Spec: &config.DataApiDataApiSpec, |
There was a problem hiding this comment.
(found by my agent) parent is the only required field, so a config that only sets parent (that is, "enable it with defaults") passes validate and then fails at deploy with "Field 'data_api' is required and must contain at least one subfield with a non-default value!" (libs/testserver/postgres.go mirrors this). Every test config sets db_schemas, so this case isn't covered. Should validation or the schema description say that at least one setting is needed, or should create send something by default?
There was a problem hiding this comment.
Good catch. Fixed in 46d77f5: bundle validate now rejects a Data API that sets only parent (it fails before anything is deployed), the schema description says at least one setting is required, and there's a parent_only acceptance test. The check reads the dynamic config, so a setting given as a ${...} reference counts and lifecycle doesn't. I'd rather not send a default from the CLI, since that would hard-code a server default.
| # database's Postgres name, none of which a Data API update can change, so a | ||
| # ${resources.postgres_data_apis.<key>.status.url} reference resolves during | ||
| # an in-place update instead of planning a no-op update of its dependents. | ||
| - field: status.url |
There was a problem hiding this comment.
(found by my agent) Unlike every other stable output field, status.url is not built from this resource's ID. It comes from the endpoint host and the database's postgres_database, and postgres_databases updates postgres_database in place (DoUpdate builds the mask from the changes). If someone renames it, the URL changes without any Data API update. A job referencing status.url would then get the URL read during plan, which is the old one. Is that case handled somewhere, or should the comment at least say the URL is stable only with respect to this resource's own updates?
There was a problem hiding this comment.
Not handled, but the stable_output_fields entry isn't what causes it. When only the database is renamed, the Data API itself is unchanged (Skip), and a Skip target's fields are already read from the plan-time remote (canReadRemoteCache in LookupReferencePreDeploy), so a dependent job gets the old URL either way and picks up the new one on the next deploy. Reworded the comment in ab7d1a8 to say the URL is stable only with respect to the Data API's own updates. (I haven't checked a rename against a live workspace.)
The roles and databases used to hard-code `.../branches/production`, an implicit dependency on the branch the project creates. Take over that branch with `replace_existing: true` (as in postgres_branches/replace_existing) in every Data API acceptance test and in the invariant config, and reference it from the role and database parents. The bind test is unchanged: its parent is a literal created by the setup script, so there is no bundle dependency. Co-authored-by: Isaac <no-reply@databricks.com>
The Data API create call requires at least one setting with a non-default
value, so a config with only `parent` passed `bundle validate` and then
failed at deploy. Add a validate-time check that rejects such an entry
before anything is deployed, and say in the schema description that a
setting besides `parent` is required.
The check reads the dynamic config, so a setting given as a `${...}`
reference counts as set and `lifecycle` does not. The new local-only
`parent_only` acceptance test shows the error for validate and deploy.
Co-authored-by: Isaac <no-reply@databricks.com>
Add a Badness note to the update acceptance test: removing a previously set field plans an update, but update_mask=spec merges, so the server keeps the last value and the next plan is clean. Reword the status.url stable_output_fields comment: the URL is stable only with respect to the Data API's own updates. Renaming the database's postgres_database changes it, and a reference resolved in that same deploy keeps the old URL until the next deploy. Co-authored-by: Isaac <no-reply@databricks.com>
Changes
Add the
postgres_data_apisresource (Private Preview). It enables and configures the Data API (PostgREST) of a Lakebase Postgres database:{parent}/data-api; changingparentrecreates it. Removing the resource turns the Data API off; the database and its data are unaffected.status.url; reference it as${resources.postgres_data_apis.<key>.status.url}. It is a stable output field: it is built from the read-write endpoint host, the workspace and the database name, so an in-place update of the Data API cannot change it, and the reference resolves without planning a no-op update of the resources that use it.jwt_cache_max_lifetimeuse the protobuf seconds format, for example300s.update_mask=spec.specis INPUT_ONLY, so GET does not return it.DoReadfills the remote spec fromstatus, which reports every setting in effect, sobundle plandetects out-of-band changes andbundle deployment bindapplies the configured spec on the next deploy. The generatedspec:input_onlyrules are dropped (apitypes.yml: postgres_data_apis: null) and every spec field is a hand-writtenbackend_default, so a field the config omits is left to the server.falseand was never set, so the bool fields are forced on the remote side.update_mask=specmerges, and per-field clearing is not used because clearing some fields currently leaves the Data API unable to serve requests. The removal plans one no-op update; the next plan is clean.postgres_*resources are Beta), so the JSON schema marks itdoNotSuggest. Like its Beta siblings, it is not generated in PyDABs yet, which only covers GA and Public Preview resources.Why
Lakebase users already manage projects, branches and databases with bundles and want to manage the Data API alongside them. The API is at Private Preview.
Notes for reviewers:
postgres_*resource, this one waits on its operation inline in create, update and delete, because the operation ID isn't persisted.experimental.deployment_historyenabled reject this resource type, so theDMS=trueacceptance variants are excluded for this resource (as forfeatures) and the test server's DMS fake does not accept the prefix yet. Both get re-enabled once the server change ships.jwt_role_claim_keyandjwt_cache_max_lifetimeare exposed because the API accepts them. Their runtime behaviour is still evolving at Private Preview; happy to hide them if you prefer.Tests
status.urlresolves on the first deploy, and an in-place update of the Data API plans no update of the job). The test server's Data API fake mirrors the real API's defaults,status.url, merge-on-update semantics, and error codes and messages.trueand an explicitfalse), a duration and an enum: no drift, removal of every optional field converges, delete/destroy idempotent.status.urlresolved on the first deploy, update (row cap applied, job not updated, next plan clean), removed field (value kept), out-of-band change detected and reverted, bind of an existing Data API applied on deploy, destroy (Data API off, data intact).This pull request and its description were written by Isaac.