feat: add two-phase expected resource auto-discovery to validator#164
feat: add two-phase expected resource auto-discovery to validator#164mchmarny merged 5 commits intoNVIDIA:mainfrom
Conversation
7ceb158 to
7cd6a2f
Compare
mchmarny
left a comment
There was a problem hiding this comment.
The extraction of manifest rendering into pkg/manifest is a good refactor — sharing rendering logic between bundler and validator avoids drift. The resource discovery design (two-phase: helm template + manifest file rendering, with merge semantics) is well-structured, and the test coverage is thorough (784 lines of tests for 391 lines of implementation).
Note: The PR template is not filled out — no summary, type of change, risk assessment, or component checkboxes are checked. For a +1627/-237 change touching 9 files across bundler, validator, manifest, and e2e, this context would help reviewers.
Main concerns:
-
YAML library behavioral change (
gopkg.in/yaml.v3→sigs.k8s.io/yaml) — This changestoYamloutput for the bundler too (not just validator), sincehelm.gonow calls the sharedmanifest.Render(). The libraries have different marshaling paths (direct YAML vs JSON→YAML intermediate) which can produce different output. -
Network dependency introduced —
helm template --repodownloads charts from remote repositories. Validators that were previously offline-capable now require network + helm CLI when recipes have chart coordinates. The helm CLI check is a hard error (line 76), while individual chart failures are warnings — this asymmetry could be confusing. -
E2E Test 4 always passes — The merge test passes regardless of whether the expected-resources check fails or succeeds, defeating the purpose of including
nonexistent-deploy.
Minor concerns:
- Timeout constant should be in
pkg/defaultsper project conventions renderManifestFilesmissingcontext.Contextparameter (inconsistent with project rules and withrenderHelmTemplate)- Recipe mutation before serialization should be documented as intentional
…dd context to renderManifestFiles, fix e2e assertion
Summary
Add two-phase expected resource auto-discovery to the validator: Phase 1 renders Helm charts via CLI subprocess (
helm template), Phase 2 renders component manifest files via sharedpkg/manifest.Render(). Extract shared manifest rendering topkg/manifestto eliminate duplication between bundler and validator. ManualexpectedResourcesmerge with and override auto-discovered ones.Motivation / Context
Validators need to know which Kubernetes resources (Deployments, DaemonSets, StatefulSets) a component should produce so they can check deployment health. Previously this required manually listing
expectedResourcesin every recipe component.Now the validator auto-discovers workload resources from two sources:
helm template) — discovers main workload resources from chartspkg/manifest.Render()— discovers supplementary resources from component manifest files (same logic as the bundler)Missing helm CLI is a hard error when Helm components with chart coordinates exist. Helm is included in the validator image (
Dockerfile.validator).The bundler and validator previously duplicated Go-template rendering logic. This is now extracted to a shared
pkg/manifestpackage.Fixes: N/A
Related: N/A
Type of Change
Component(s) Affected
cmd/eidos,pkg/cli)cmd/eidosd,pkg/api,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)pkg/manifest(new shared package),pkg/defaultsImplementation Notes
New:
pkg/manifest— shared manifest renderingRender(),HelmFuncMap(),RenderInputfrom the bundler's unexported helpersgopkg.in/yaml.v3fortoYamlto maintain behavioral compatibility with the bundler's existing output (avoids JSON-intermediate marshaling differences fromsigs.k8s.io/yaml)pkg/bundler/deployer/helmandpkg/validatornow import from one placeNew:
pkg/validator/resource_discovery.go— two-phase discoveryresolveExpectedResources()runs before validation in the CLI processhelm template) via CLI subprocess to discover main workload resources. Requires network access for chart downloads (HTTP repos via--repo, OCI registries viaoci://prefix)pkg/manifest.Render()to discover supplementary resourcesmergeExpectedResources()combines auto-discovered + manual, with manual taking precedencerenderManifestFiles()acceptscontext.Contextand checksctx.Done()for cancellationComponentRenderTimeoutlives inpkg/defaultsper project conventionsModified:
pkg/bundler/deployer/helm/helm.gopkg/manifestModified:
pkg/validator/phases.goresolveExpectedResourcesat the start of validation; intentionally mutatesrecipeResultbefore serialization so the check pod sees the full expected resources listWriteString(Sprintf)→Fprintf)Modified:
pkg/defaults/timeouts.goComponentRenderTimeout(60s) for helm template and manifest file renderingModified:
pkg/component/helpers.goModified:
tests/e2e/run.shnonexistent-deployis not foundTesting
Risk Assessment
Rollout notes: Discovery is additive — components without chart sources or manifest files simply skip discovery. Manual
expectedResourcesstill work exactly as before.Checklist
make testwith-race)make lint)git commit -S) — GPG signing info