Fix resource types missing from rad resource-type list with per-type manifest files#11933
Fix resource types missing from rad resource-type list with per-type manifest files#11933kachawla wants to merge 7 commits into
Conversation
…manifest files When multiple manifest files share the same namespace, the initializer now merges types into the existing Location and ResourceProviderSummary instead of overwriting them. Also preserves existing location address when a later manifest does not specify one. Adds integration tests verifying namespace merge behavior with real manifests and a functional test deploying Radius.Compute/containers and Radius.Compute/routes end-to-end. Signed-off-by: Karishma Chawla <kachawla@microsoft.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Pull request overview
Fixes a regression in UCP manifest initialization where per-type manifest files that share a namespace (e.g., Radius.Compute) would overwrite previously-registered Location and ResourceProviderSummary entries, causing earlier types to disappear from rad resource-type list.
Changes:
- Update
registerResourceProviderDirectto read existing Location/Summary records and merge in newly discovered resource types (and preserve location address when omitted). - Expand UCP integration coverage to validate namespace-level merging and ensure all
defaults.yamltypes are registered from real built-in manifests. - Add a portable functional test that deploys two default
Radius.Computetypes end-to-end using the default recipe pack.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/ucp/initializer/service.go | Implements read-merge-write for Location and ResourceProviderSummary during manifest registration. |
| pkg/ucp/integrationtests/resourceproviders/resourceproviders_test.go | Adds/updates integration tests to verify merged registration across multiple files per namespace and validate defaults registration. |
| pkg/ucp/integrationtests/resourceproviders/testdata/manifests-no-location/persistentVolumes.yaml | Adds a second no-location manifest sharing Radius.Compute namespace for merge regression coverage. |
| test/functional-portable/dynamicrp/noncloud/resources/default_containers_test.go | Adds an e2e functional test deploying default containers and routes types from the same namespace. |
| test/functional-portable/dynamicrp/noncloud/resources/testdata/default-containers-test.bicep | Bicep template used by the new e2e functional test. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11933 +/- ##
=======================================
Coverage 51.69% 51.70%
=======================================
Files 724 724
Lines 45508 45522 +14
=======================================
+ Hits 23525 23535 +10
- Misses 19763 19765 +2
- Partials 2220 2222 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Instead of read-merge-write on Location and Summary per file, merge all manifest files by namespace in memory first, then register once per namespace. This makes the on-disk manifest directory the source of truth and avoids accumulating stale types from previous database state. registerResourceProviderDirect is reverted to the simple overwrite behavior since it now receives a single merged provider per namespace with all types already combined. Signed-off-by: Karishma Chawla <kachawla@microsoft.com>
The Radius.Compute/containers recipe resolves the application via Applications.Core, which fails when the app is Radius.Core/applications referencing an Applications.Core/environments. Fix by creating a self-contained Radius.Core/environments with its own K8s namespace. Signed-off-by: Karishma Chawla <kachawla@microsoft.com>
Signed-off-by: Karishma Chawla <kachawla@microsoft.com>
|
|
||
| // Merge types from this file into the existing provider for this | ||
| // namespace. If a type appears in multiple files, the later file wins. | ||
| for typeName, resourceType := range rp.Types { |
There was a problem hiding this comment.
is this a decision we took instead of flagging multiple definitions? if so, should we add a log to help with debugging.
There was a problem hiding this comment.
Good question. My original thought was that it shouldn't be blocking as we have PR reviews that should catch it. But on second thought, it should cause the startup to fail because it can go unnoticed otherwise. Updating to return an error.
Thanks for reviewing!
Signed-off-by: Karishma Chawla <kachawla@microsoft.com>
Signed-off-by: Karishma Chawla <kachawla@microsoft.com>
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
Background
PR #11911 introduced automated synchronization of default resource type manifests from
resource-types-contrib. As part of that change, the manifest file layout indeploy/manifest/built-in-providers/changed from one file per namespace (e.g.,radius_compute.yamlcontainingcontainers,routes, andpersistentVolumes) to one file per type (e.g.,containers.yaml,routes.yaml,persistentVolumes.yaml). This matches the per-type layout inresource-types-contriband keeps diffs scoped to the type that changed.However, the initializer's
registerResourceProviderDirectfunction was not designed for multiple files sharing the same namespace. It saves the Location and ResourceProviderSummary per namespace on each file, and each save was a full overwrite rather than a merge.Problem
When multiple manifest files share the same namespace (e.g.,
containers.yamlandpersistentVolumes.yamlboth underRadius.Compute), the initializer was overwriting the Location and ResourceProviderSummary on each file, causing earlier types to disappear fromrad resource-type list. Only the last file's types (alphabetically) were visible.The individual ResourceType records were saved correctly (one per type), but the Location and Summary - which UCP's routing layer and
rad resource-type listread from - only contained the last file's types.Fix
registerResourceProviderDirectnow reads the existing Location and Summary from the database before saving, merging in new types alongside existing ones. If no existing record is found (first file for this namespace), it behaves as before. OnlyErrNotFoundis treated as "no existing record"; other errors (database outage, decode failure) cause the initializer to fail fast.The fix also preserves the existing location address when a later manifest for the same namespace does not specify one.
Changes
pkg/ucp/initializer/service.go- Read-merge-write for Location and ResourceProviderSummary, with ErrNotFound-only error handling and address preservationpkg/ucp/integrationtests/resourceproviders/resourceproviders_test.go- UpdatedTest_ResourceProvider_RegisterManifests_NoLocationto verify both types from two files sharing a namespace are registered and present in the location's resourceTypes map. AddedTest_ResourceProvider_DefaultsRegisteredthat readsdefaults.yaml, loads real manifests, and verifies all types are registered with correct location entries.testdata/manifests-no-location/persistentVolumes.yaml- Second test manifest sharing theRadius.Computenamespacetest/functional-portable/dynamicrp/noncloud/resources/default_containers_test.go- End-to-end functional test deployingRadius.Compute/containersandRadius.Compute/routes(two types from the same namespace) using the default recipestest/functional-portable/dynamicrp/noncloud/resources/testdata/default-containers-test.bicep- Bicep template for the e2e testTesting
Test_ResourceProvider_RegisterManifests_NoLocation): Two test manifests sharingRadius.Computenamespace, verifies both types appear in the location's resourceTypes map.Test_ResourceProvider_DefaultsRegistered): Readsdefaults.yaml, loads manifests frombuilt-in-providers/dev/, verifies all types are registered with correct location entries.Test_DefaultContainers_Deploy): End-to-end deployment ofRadius.Compute/containersandRadius.Compute/routesusing default recipes.