Direct-engine reconcile classification fixes#5816
Conversation
… fields Several fields returned (or intentionally not returned) by UC/serving GET responses were not classified, so `bundle plan` reported a perpetual Update after deploy and could hard-fail with "Nothing to update" on redeploy. - catalogs: skip UC-injected `unity.catalog.managed.*` properties as backend defaults (mirrors the existing schemas handling). - registered_models: treat backend-computed `browse_only` as output-only. - model_serving_endpoints: ignore remote changes for `burst_scaling_enabled` and the write-only external-model `*_plaintext` secrets, none of which are echoed on GET. The testserver is extended to reproduce each drift (scoped catalog/model names, and stripping serving secrets on read) and new acceptance tests under each resource's `drift/` dir lock in the no-op plan and redeploy behavior.
radakam
force-pushed
the
fix/direct-reconcile-classification
branch
from
c01de9a to
f4ed131
Compare
Approval status: pending
|
Integration test reportCommit: f4ed131
23 interesting tests: 13 SKIP, 7 KNOWN, 3 RECOVERED
Top 5 slowest tests (at least 2 minutes):
|
| reason: output_only | ||
| - field: updated_by | ||
| reason: output_only | ||
| # Backend-computed; the user never sets it. Not annotated output_only in the spec. |
There was a problem hiding this comment.
We should probably annotate browse_only as output-only (or drop it from the create request) in the universe OpenAPI spec so the regenerated SDK stops surfacing it as a writable field.
| # Accepted on write but not returned by GET. | ||
| - field: config.served_entities[*].burst_scaling_enabled | ||
| reason: input_only | ||
| # Write-only secrets: the backend stores them and returns the reference field, not the plaintext. |
There was a problem hiding this comment.
can we wild-card match these? there will be more as more models are added
There was a problem hiding this comment.
I'm not sure about that, wildcards can't match *_plaintext, and broader wildcards would also ignore legitimate GET fields. It might be better to mark these fields as write-only in the OpenAPI spec so they're generated automatically, like browse_only, what do you think?
| resources: | ||
| catalogs: | ||
| catalog1: | ||
| name: catalog_managed_defaults |
There was a problem hiding this comment.
We have acceptance/bundle/invariant/configs/catalog.yml.tmpl why did not it catch this?
Is it because fuzz testing was against dogfood which adds these fields but our test env does not?
There was a problem hiding this comment.
Exactly -- the testserver wasn't populating these fields (dogfood does), so there was no drift to catch. I also added that to the testserver so it's now reproducible.
4 participants
Why
Fuzzing the direct engine found fields that UC/serving GET responses either compute or never echo back. They weren't classified, so
bundle planshowed a permanentupdateafter deploy and redeploy could fail with400 ... Nothing to update.Changes
bundle/direct/dresources/resources.yml:properties['unity.catalog.managed.*']asbackend_defaults(mirrors existing schemas fix).browse_only→ignore_remote_changes(output-only, backend-computed, not in update surface).config.served_entities[*].burst_scaling_enabled+ all external-model*_plaintextsecrets →ignore_remote_changes(input-only, accepted on write but never returned on GET).These aren't annotated in the OpenAPI spec, so they're declared manually (absent from
resources.generated.yml).libs/testserver/:catalogs.go/registered_models.go: inject the computed values, scoped to dedicated names (catalog_managed_defaults,model_browse_only).serving_endpoints.go: strip*_plaintextsecrets on read (burst_scaling_enabledalready wasn't round-tripped).Tests
New direct-engine acceptance tests asserting no-op plan + no update call on redeploy:
acceptance/bundle/resources/catalogs/drift/managed_propertiesacceptance/bundle/resources/registered_models/drift/browse_onlyacceptance/bundle/resources/model_serving_endpoints/drift/write_only