feat(rest-api): iPXE template + templated-OS sync workflows & activities - #3232
Merged
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThis PR adds site-agent discovery for OperatingSystem and iPXE Template inventory, wires new Temporal workflows and activities to publish that inventory, and adds workflow-side reconciliation, schema changes, and tests to persist the inventory into the REST database. ChangesOS/iPXE Template Inventory Discovery and Reconciliation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant SiteAgent as Site-Agent Cron/Publisher
participant Discovery as Discovery Activity
participant Core as Core gRPC
participant Temporal as Temporal Queue
participant UpdateWF as Update Workflow
participant DBAct as Reconciliation Activity
participant DB as REST Database
SiteAgent->>Discovery: trigger DiscoverXInventory
Discovery->>Core: fetch resources
Core-->>Discovery: data or error
Discovery->>Temporal: publish inventory payload
Temporal->>UpdateWF: deliver payload
UpdateWF->>DBAct: UpdateXInDB(siteID, inventory)
DBAct->>DB: reconcile records and associations
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-07 22:12:17 UTC | Commit: 8a612f3 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
rest-api/site-agent/pkg/components/managers/operatingsystem/publisher.go (1)
34-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated
uuid.MustParseand near-identical config blocks.The three inventory-manager blocks (OS Image, OperatingSystem, iPXE Template) each re-parse
ManagerAccess.Conf.EB.Temporal.ClusterIDviauuid.MustParseand repeat the sameManageInventoryConfigscaffolding. Parsing the site ID once and reusing it would reduce duplication and centralize the (panicking) parse failure mode to a single call site.func (api *API) RegisterPublisher() error { ManagerAccess.Data.EB.Log.Info().Msg("OperatingSystem: Registering inventory workflow and activity") + siteID := uuid.MustParse(ManagerAccess.Conf.EB.Temporal.ClusterID) // Register DiscoverOsImageInventory workflow ... osImageInventoryManager := swa.NewManageOsImageInventory(swa.ManageInventoryConfig{ - SiteID: uuid.MustParse(ManagerAccess.Conf.EB.Temporal.ClusterID), + SiteID: siteID,Given each manager also has a distinct config shape (e.g.
SitePageSize/CloudPageSizeonly for OS Image), a full generic helper may not be worth the effort here — this is a lower-priority polish item.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/site-agent/pkg/components/managers/operatingsystem/publisher.go` around lines 34 - 61, Parse ManagerAccess.Conf.EB.Temporal.ClusterID once in this publisher setup and reuse the resulting site ID across the inventory manager registrations instead of calling uuid.MustParse in each block. In the OperatingSystem publisher flow, hoist the parsed UUID to a shared local variable and pass it into each ManageInventoryConfig so the OS Image, OperatingSystem, and iPXE Template setup blocks stay consistent while avoiding repeated parse/panic points. Keep the distinct per-manager config fields as-is, but remove the duplicated ClusterID parsing from each NewManageOperatingSystemInventory / NewManageIpxeTemplateInventory call site.rest-api/site-agent/pkg/components/managers/operatingsystem/cron.go (1)
27-35: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFail-fast chain silently skips remaining cron registrations.
RegisterCronreturns immediately if the "OS Image" registration fails, without attempting "OperatingSystem" or "iPXE Template" registration. A single transient Temporal error on the first cron therefore disables the two newly introduced inventory-sync crons for the process lifetime (until restart), with no distinct signal that only part of the registration failed.Consider attempting all three registrations and aggregating errors (e.g.
errors.Join), so an issue with one inventory type doesn't mask the others' successful registration.♻️ Proposed fix to avoid short-circuiting
func (api *API) RegisterCron() error { - if err := api.registerInventoryCron("OS Image", "inventory-os-image-", sww.DiscoverOsImageInventory); err != nil { - return err - } - if err := api.registerInventoryCron("OperatingSystem", "inventory-operating-system-", sww.DiscoverOperatingSystemInventory); err != nil { - return err - } - return api.registerInventoryCron("iPXE Template", "inventory-ipxe-template-", sww.DiscoverIpxeTemplateInventory) + errOsImage := api.registerInventoryCron("OS Image", "inventory-os-image-", sww.DiscoverOsImageInventory) + errOperatingSystem := api.registerInventoryCron("OperatingSystem", "inventory-operating-system-", sww.DiscoverOperatingSystemInventory) + errIpxeTemplate := api.registerInventoryCron("iPXE Template", "inventory-ipxe-template-", sww.DiscoverIpxeTemplateInventory) + return errors.Join(errOsImage, errOperatingSystem, errIpxeTemplate) }As per path instructions for
rest-api/site-agent/**: "Review site-agent changes for local reconciliation safety... and robustness during connectivity loss."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/site-agent/pkg/components/managers/operatingsystem/cron.go` around lines 27 - 35, RegisterCron currently returns on the first failed registerInventoryCron call, which can skip later cron registrations and hide partial success. Update RegisterCron to attempt all three registrations (OS Image, OperatingSystem, iPXE Template) regardless of individual failures, collecting any errors from registerInventoryCron and returning a combined result (for example via errors.Join). Keep the existing symbols RegisterCron and registerInventoryCron so the fix is localized and the caller can see whether one or more cron setups failed.Source: Path instructions
rest-api/workflow/pkg/activity/operatingsystem/operatingsystem_test.go (1)
456-464: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a same-provider, different-site deletion regression.
The current scenarios avoid cross-site interference by using distinct providers, but production providers can have multiple sites. Add a case where Site B owns a Local iPXE OS and reconciling an empty inventory for Site A does not delete Site B’s OS.
As per path instructions, review Go code for correctness and test coverage.
Also applies to: 574-610
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/workflow/pkg/activity/operatingsystem/operatingsystem_test.go` around lines 456 - 464, Add a regression test in TestManageOsImage_UpdateOperatingSystemsInDB covering two sites under the same Infrastructure Provider: create a provider-owned Local iPXE Operating System for Site B, reconcile an empty inventory for Site A, and verify Site B’s record is not deleted. Update the relevant scenario setup in TestManageOsImage_UpdateOperatingSystemsInDB and any helper assertions so deletion-by-absence is scoped correctly by site, not just provider, using the existing Operating System reconciliation test helpers and local OS record symbols from the diff.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rest-api/site-agent/pkg/components/managers/operatingsystem/publisher.go`:
- Line 62: `RegisterPublisher` is ignoring failures from `api.RegisterCron()`,
so publisher registration can appear successful even when inventory cron setup
fails. Update `RegisterPublisher` to capture and return the error from
`RegisterCron()` instead of discarding it, using the `RegisterPublisher` and
`RegisterCron` flow to locate the fix.
In `@rest-api/site-workflow/pkg/activity/ipxetemplate.go`:
- Around line 49-62: The Temporal publish calls in ipxetemplate.go are
incorrectly reusing the activity ctx, which can cause the outbound workflow
execution to fail if the activity context is near timeout or canceled. Update
both ExecuteWorkflow calls in the iPXE template activity to use
context.Background() instead of ctx, matching the pattern used in
DiscoverOperatingSystemInventory and keeping the publish independent from the
activity lifecycle. Ensure both the failure path and the success path use the
new background context when calling
mii.config.TemporalPublishClient.ExecuteWorkflow.
In `@rest-api/workflow/pkg/activity/operatingsystem/operatingsystem.go`:
- Around line 741-759: Scope the deletion reconciliation to the current
reporting site by using the OS-site association for siteID as the candidate set
instead of every local iPXE OS for the provider. Update the logic around
osDAO.GetAll and the loop in the reconciliation block so only OperatingSystem
records associated with the active site are considered for soft-delete, while
still keeping the existing reportedOSIDs check and osDAO.Delete behavior.
- Around line 177-179: The DB update branches in operating system status
handling are logging the wrong variable: when
`mos.updateOperatingSystemSiteAssociationStatusInDB` (and the other scoped
update calls) returns `serr != nil`, the logger should use that scoped error
instead of the outer `err`. Update the `slogger.Error()` calls in these failure
blocks to report the local error variable so the actual DB failure is preserved
for `updateOperatingSystemSiteAssociationStatusInDB` and the related status
update paths.
- Around line 523-566: The templated iPXE OS path in operatingSystem sync is
persisting IpxeTemplateId without guaranteeing a valid site-available template
reference. In the create flow around the templated-iPXE branch and the later
osDAO.Create call, only proceed with IpxeTemplateId when it has a non-empty
value and the itsaDAO.GetByIpxeTemplateIDAndSiteID lookup succeeds; otherwise
skip or clear the template field. Apply the same validation in the update logic
referenced by the same reportedOS/IpxeTemplateId handling so it cannot overwrite
an OS with an empty or unavailable template association.
- Around line 554-604: The OS creation flow in operatingsystem reconciliation
performs dependent writes through osDAO.Create, osDAO.Update, and ossaDAO.Create
without a shared transaction, so a later failure can leave partial state. Wrap
these writes in cdb.WithTx in the reconciliation path, using the same
transaction handle for the OperatingSystemCreateInput, the inactive update
branch, and the OperatingSystemSiteAssociationCreateInput so they commit or roll
back together. Keep the error handling on the existing osDAO, osDAO.Update, and
ossaDAO calls, but ensure they all execute inside the transaction scope.
In `@rest-api/workflow/pkg/workflow/operatingsystem/update.go`:
- Around line 114-115: Remove the result decode from the ExecuteActivity call in
the operating system workflow: UpdateOperatingSystemsInDB only returns an error,
so in operating-system update.go the call to workflow.ExecuteActivity(...).Get
should use a nil result target instead of osIDs. After that, remove the
follow-up osIDs handling loop in the same workflow unless
OsManager.UpdateOperatingSystemsInDB is intentionally changed to return IDs, so
the contract between the workflow and activity stays consistent.
---
Nitpick comments:
In `@rest-api/site-agent/pkg/components/managers/operatingsystem/cron.go`:
- Around line 27-35: RegisterCron currently returns on the first failed
registerInventoryCron call, which can skip later cron registrations and hide
partial success. Update RegisterCron to attempt all three registrations (OS
Image, OperatingSystem, iPXE Template) regardless of individual failures,
collecting any errors from registerInventoryCron and returning a combined result
(for example via errors.Join). Keep the existing symbols RegisterCron and
registerInventoryCron so the fix is localized and the caller can see whether one
or more cron setups failed.
In `@rest-api/site-agent/pkg/components/managers/operatingsystem/publisher.go`:
- Around line 34-61: Parse ManagerAccess.Conf.EB.Temporal.ClusterID once in this
publisher setup and reuse the resulting site ID across the inventory manager
registrations instead of calling uuid.MustParse in each block. In the
OperatingSystem publisher flow, hoist the parsed UUID to a shared local variable
and pass it into each ManageInventoryConfig so the OS Image, OperatingSystem,
and iPXE Template setup blocks stay consistent while avoiding repeated
parse/panic points. Keep the distinct per-manager config fields as-is, but
remove the duplicated ClusterID parsing from each
NewManageOperatingSystemInventory / NewManageIpxeTemplateInventory call site.
In `@rest-api/workflow/pkg/activity/operatingsystem/operatingsystem_test.go`:
- Around line 456-464: Add a regression test in
TestManageOsImage_UpdateOperatingSystemsInDB covering two sites under the same
Infrastructure Provider: create a provider-owned Local iPXE Operating System for
Site B, reconcile an empty inventory for Site A, and verify Site B’s record is
not deleted. Update the relevant scenario setup in
TestManageOsImage_UpdateOperatingSystemsInDB and any helper assertions so
deletion-by-absence is scoped correctly by site, not just provider, using the
existing Operating System reconciliation test helpers and local OS record
symbols from the diff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8b4c4b90-7272-42fb-98b1-4a4a462643a7
📒 Files selected for processing (15)
rest-api/site-agent/pkg/components/managers/operatingsystem/cron.gorest-api/site-agent/pkg/components/managers/operatingsystem/publisher.gorest-api/site-workflow/pkg/activity/ipxetemplate.gorest-api/site-workflow/pkg/activity/operatingsystem.gorest-api/site-workflow/pkg/workflow/ipxetemplate.gorest-api/site-workflow/pkg/workflow/operatingsystem.gorest-api/workflow/cmd/workflow/main.gorest-api/workflow/pkg/activity/ipxetemplate/ipxetemplate.gorest-api/workflow/pkg/activity/ipxetemplate/ipxetemplate_test.gorest-api/workflow/pkg/activity/operatingsystem/operatingsystem.gorest-api/workflow/pkg/activity/operatingsystem/operatingsystem_test.gorest-api/workflow/pkg/util/testing.gorest-api/workflow/pkg/workflow/ipxetemplate/update.gorest-api/workflow/pkg/workflow/ipxetemplate/update_test.gorest-api/workflow/pkg/workflow/operatingsystem/update.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rest-api/site-agent/pkg/components/managers/operatingsystem/publisher.go (1)
34-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the repeated registration boilerplate.
The
DiscoverOperatingSystemInventoryandDiscoverIpxeTemplateInventoryblocks (Lines 34-60) mirror the pre-existingDiscoverOsImageInventoryblock almost verbatim: buildManageInventoryConfig, register workflow, register activity, log twice. With three near-identical copies now in place, a small helper (e.g.registerInventoryWorkflowAndActivity(label, workflow, activity, cfg)) would remove the duplication and make it harder to drift (e.g. forgetting a log line or a config field) as more inventory types are added.♻️ Sketch of a possible helper
+func (api *API) registerDiscoveryWorkflowAndActivity( + label string, + workflowFn interface{}, + registerActivity func(), +) { + ManagerAccess.Data.EB.Managers.Workflow.Temporal.Worker.RegisterWorkflow(workflowFn) + ManagerAccess.Data.EB.Log.Info().Msgf("OperatingSystem: Successfully registered %s workflow", label) + registerActivity() + ManagerAccess.Data.EB.Log.Info().Msgf("OperatingSystem: Successfully registered %s activity", label) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/site-agent/pkg/components/managers/operatingsystem/publisher.go` around lines 34 - 61, The DiscoverOperatingSystemInventory and DiscoverIpxeTemplateInventory registration blocks duplicate the same workflow/activity setup pattern already used by DiscoverOsImageInventory. Extract that repeated boilerplate into a small helper (for example in publisher.go near the existing registration code) that takes the label, workflow, activity, and ManageInventoryConfig, then performs RegisterWorkflow, RegisterActivity, and the two log messages consistently. Update the existing DiscoverOperatingSystemInventory and DiscoverIpxeTemplateInventory registrations to call the helper so the setup stays aligned and easier to extend.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rest-api/site-agent/pkg/components/managers/operatingsystem/publisher.go`:
- Around line 34-61: The DiscoverOperatingSystemInventory and
DiscoverIpxeTemplateInventory registration blocks duplicate the same
workflow/activity setup pattern already used by DiscoverOsImageInventory.
Extract that repeated boilerplate into a small helper (for example in
publisher.go near the existing registration code) that takes the label,
workflow, activity, and ManageInventoryConfig, then performs RegisterWorkflow,
RegisterActivity, and the two log messages consistently. Update the existing
DiscoverOperatingSystemInventory and DiscoverIpxeTemplateInventory registrations
to call the helper so the setup stays aligned and easier to extend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: efd8c814-2a88-4b84-b1f3-af5aeaf6d37e
📒 Files selected for processing (5)
rest-api/site-agent/pkg/components/managers/operatingsystem/publisher.gorest-api/site-workflow/pkg/activity/ipxetemplate.gorest-api/workflow/pkg/activity/operatingsystem/operatingsystem.gorest-api/workflow/pkg/activity/operatingsystem/operatingsystem_test.gorest-api/workflow/pkg/workflow/operatingsystem/update.go
🚧 Files skipped from review as they are similar to previous changes (2)
- rest-api/site-workflow/pkg/activity/ipxetemplate.go
- rest-api/workflow/pkg/activity/operatingsystem/operatingsystem.go
thossain-nv
reviewed
Jul 8, 2026
thossain-nv
left a comment
Contributor
There was a problem hiding this comment.
Still going through the OperatingSystem inventory activity, but wanted to publish the initial comments.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rest-api/db/pkg/db/model/ipxetemplate.go (1)
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider a typed
Visibilityenum instead of a barestring.
Visibilityhas exactly two known values (Public/Internal) and round-trips with thecwssaws.IpxeTemplateScopeproto enum (see theipxeVisibilityToStringconversion in the workflow activity). This is precisely the case the repo's own DB-model guideline targets: a finite-valued domain concept that round-trips with proto should be a named string type owning its ownToProto/FromProtomethods, not a bare string paired with a free conversion function.Given this PR is a pure rename, this is a deferred improvement rather than a blocker — but worth tracking before the type proliferates further (e.g. into
IpxeTemplateFilterInputor additional callers).♻️ Suggested direction (not required for this PR)
- Visibility string `bun:"visibility,notnull"` + Visibility IpxeTemplateVisibility `bun:"visibility,notnull"`type IpxeTemplateVisibility string const ( IpxeTemplateVisibilityInternal IpxeTemplateVisibility = "Internal" IpxeTemplateVisibilityPublic IpxeTemplateVisibility = "Public" ) func (v IpxeTemplateVisibility) ToProto() cwssaws.IpxeTemplateScope { /* ... */ } func (v *IpxeTemplateVisibility) FromProto(p cwssaws.IpxeTemplateScope) { /* ... */ }As per coding guidelines, "Named composite types that represent domain concepts with conversion needs should own their proto behavior via receiver methods, not free functions" (
rest-api/db/pkg/db/model/**/*.go).Also applies to: 56-56, 63-71, 76-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/db/pkg/db/model/ipxetemplate.go` around lines 30 - 33, Replace the bare string visibility constants in IpxeTemplate with a named string type for the domain concept, and move the proto mapping onto receiver methods. Update the IpxeTemplateVisibilityInternal/Public constants to use the new type, then add ToProto and FromProto methods on IpxeTemplateVisibility instead of relying on the free ipxeVisibilityToString-style conversion in the workflow activity. Keep all existing call sites and any future inputs like IpxeTemplateFilterInput using the typed visibility value consistently.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rest-api/db/pkg/db/model/ipxetemplate.go`:
- Around line 30-33: Replace the bare string visibility constants in
IpxeTemplate with a named string type for the domain concept, and move the proto
mapping onto receiver methods. Update the IpxeTemplateVisibilityInternal/Public
constants to use the new type, then add ToProto and FromProto methods on
IpxeTemplateVisibility instead of relying on the free
ipxeVisibilityToString-style conversion in the workflow activity. Keep all
existing call sites and any future inputs like IpxeTemplateFilterInput using the
typed visibility value consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6edee695-2430-4ca8-9998-713fd7d4269a
📒 Files selected for processing (7)
rest-api/db/pkg/db/model/ipxetemplate.gorest-api/db/pkg/db/model/ipxetemplate_test.gorest-api/db/pkg/db/model/ipxetemplatesiteassociation_test.gorest-api/db/pkg/migrations/20260623150000_ipxe_os_and_templates.gorest-api/db/pkg/migrations/20260708130000_ipxe_template_scope_to_visibility.gorest-api/workflow/pkg/activity/ipxetemplate/ipxetemplate.gorest-api/workflow/pkg/activity/ipxetemplate/ipxetemplate_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- rest-api/workflow/pkg/activity/ipxetemplate/ipxetemplate_test.go
- rest-api/workflow/pkg/activity/ipxetemplate/ipxetemplate.go
pbreton
force-pushed
the
feat/ipxe-templated-os-2
branch
from
July 9, 2026 22:50
97e37d6 to
be49050
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3232.docs.buildwithfern.com/infra-controller |
pbreton
force-pushed
the
feat/ipxe-templated-os-2
branch
from
July 10, 2026 18:17
537df31 to
bb655db
Compare
thossain-nv
approved these changes
Jul 10, 2026
thossain-nv
left a comment
Contributor
There was a problem hiding this comment.
This looks good to go, left a few questions.
|
|
||
| slogger := logger.With().Str("ControllerOperatingSystemID", reportedOSID.String()).Logger() | ||
|
|
||
| coreUpdated, _ := time.Parse(time.RFC3339, reportedOS.Updated) |
Contributor
There was a problem hiding this comment.
Should we guard against invalid values here?
Contributor
Author
There was a problem hiding this comment.
Will log+skip but not propagate update.
| slogger.Error().Err(uerr).Msg("Failed to update Operating System, DB error") | ||
| continue | ||
| } | ||
| // Backfill: if the record previously had a tenant_id (old ownership model), clear it. |
Contributor
There was a problem hiding this comment.
Curious under what circumstance this would be necessary? An OS would only have Tenant ID if it was created through REST.
Contributor
Author
There was a problem hiding this comment.
Removed as potentially dangerous: if a new local OS is created with inconsistent data we will now warn and do nothing.
pbreton
force-pushed
the
feat/ipxe-templated-os-2
branch
from
July 14, 2026 16:22
d3071e2 to
3a2ec15
Compare
PR 2/3 (sync layer, stacked on 1/3). Wires core<->site synchronization for iPXE templates and templated Operating Systems: - workflow activities/workflows for ipxetemplate + operatingsystem update - UpdateOperatingSystemsInDB reconciliation (+ tests) incl. templated iPXE - ipxetemplate activity/workflow (+ tests) and workflow util test helper - site-workflow activities/workflows for template + OS - site-agent operatingsystem manager cron/publisher updates - register new workflows/activities in workflow/cmd/workflow/main.go
- publisher: return the error from RegisterCron() so inventory cron setup failures surface instead of being swallowed. - site-workflow ipxetemplate: publish inventory with context.Background() (not the activity ctx) so the outbound publish is independent of the activity lifecycle, matching DiscoverOperatingSystemInventory. - reconcile: log the scoped error (serr) instead of the stale outer err in the OS Image status/association update failure paths. - reconcile: require a non-empty, site-available iPXE template before persisting IpxeTemplateId on both create and update paths; skip otherwise so an OS cannot be created/overwritten with an unavailable template. - reconcile: wrap the OS create, inactive-correction, and site-association writes in a single cdb.WithTx so they commit or roll back together. - reconcile: scope deletion-by-absence to the reporting Site's associations so one Site's inventory cannot soft-delete a provider's OSes at another Site. - workflow: UpdateOperatingSystemsInDB returns only an error; decode into a nil result target and drop the dead osIDs status-update loop. Adds reconcile tests for template-availability protection and Site-scoped deletion.
Rename the iPXE template's `scope` concept to `visibility` on the REST side to avoid confusion with the operating_system `ipxe_os_scope` (Local/Global/Limited), which is a distinct concept. The Core-facing proto enum (IpxeTemplateScope) is left unchanged. - db model: constants IpxeTemplateScopeInternal/Public -> IpxeTemplateVisibility*, struct field IpxeTemplate.Scope (bun:"scope") -> Visibility (bun:"visibility"), and the Create/Update inputs. - migration: add a guarded, idempotent rename migration that converts an existing `scope` column/index to `visibility`, and is a no-op on fresh DBs. Because the original ipxe_template migration creates the table from the bun model, its raw index statement is updated to index `visibility` so fresh DBs build correctly; bun tracks migrations by name, so already-applied environments skip it and are reconciled by the new rename migration (both paths converge). - sync activity: reportedScope -> reportedVisibility, ipxeScopeToString -> ipxeVisibilityToString, plus the DB field writes and log key. - tests updated accordingly (proto-side Scope references preserved).
…llectors Refactor the site-side OS-definition and iPXE-template inventory collectors to the shared paged inventory pipeline (manageInventoryImpl / CollectAndPublishInventory), matching the OsImage collector. Each page now carries the full reported ID set in InventoryPage.ItemIds. Make the cloud OS-definition reconciler (UpdateOperatingSystemsInDB) paging-aware: create/update processes the per-page items while deletion runs only on the final page against the complete ItemIds set, so an earlier page cannot prematurely soft-delete an OS that appears on a later page. The iPXE-template reconciler was already paging-aware. Also rename registerInventoryCron's workflowFn parameter to workflowFunc, and fix a latent compile break in operatingsystem_test.go where multi-line IpxeTemplateCreateInput literals still used the removed Scope field (now Visibility). Adds paged-inventory reconcile tests for both OS definitions and iPXE templates.
Upstream moved the site-agent workflow proto types from workflow-schema/schema/site-agent/workflows/v1 (cwssaws) to proto/core/gen/v1 (corev1) and removed the old package. Re-home the templated-OS / iPXE-template sync activities, workflows, and tests onto corev1. Pure package/qualifier migration; no behavior change.
…shers The OperatingSystem and iPXE template inventory managers were constructed without SitePageSize/CloudPageSize, leaving them at 0. The shared paged inventory pipeline divides by the page size (buildPagedInventoryInput) and chunks by it (SliceToChunks), so a 0 page size makes the discover activity panic (integer divide by zero) on every run and nothing is ever published to the cloud. Set both to the values used by the OsImage publisher (InventoryCarbidePageSize site / InventoryCloudPageSize cloud) so OS and iPXE template inventory propagate from nico-core to REST.
…nt_id A Local-scoped OS is provider-owned by definition and must never carry a tenant_id. The reconcile previously "backfilled" such rows by clearing the tenant and assigning the provider. But no correct path can produce a Local (or nil-scope) OS with a tenant_id -- the API/sync create paths never set it and the ipxe_os_scope backfill migration maps tenant-owned iPXE to Global -- so reaching that state signals an upstream bug. Silently clearing the tenant hid that error and irreversibly reassigned ownership. Replace the clear (and its needsTenantClear trigger) with a data-integrity guard that logs an error and skips the record, leaving ownership untouched so the anomaly can be investigated. Add a reconcile test asserting an anomalous Local iPXE OS with a tenant_id is neither overwritten nor re-homed.
Follow the established FromProto convention (OperatingSystemIpxeParameter / OperatingSystemIpxeArtifact) for the OperatingSystem DB model: add OperatingSystemCreateInput.FromProto to map the proto-derived definition fields (type, status, scalar flags, iPXE script/template reference, parameters, artifacts, hash). The inbound reconcile now calls FromProto and only sets the sync-context fields (ID, Org, InfrastructureProviderID, IpxeOsScope), replacing the large inline struct literal. No behavior change.
…ncile Explicitly parse reportedOS.Updated and, on a missing or malformed value, log a warning and skip the timestamp-driven definition update for that OS (other reconciliation reasons still apply) instead of silently suppressing updates via a discarded parse error. Add a test asserting an OS reported with an invalid Updated is not renamed.
WithTx returns begin/commit errors that no inner DAO handler logs, so a failed transaction was skipped silently at the `if txErr != nil` guard. Log txErr before continuing so those failures are visible.
Signed-off-by: Patrice Breton <pbreton@nvidia.com>
…name Core renamed the iPXE template proto enum IpxeTemplateScope to IpxeTemplateVisibility and the IpxeTemplate.scope field to visibility (NVIDIA#3509). Update the inbound sync consumers accordingly: the site-workflow collector filter, the cloud reconcile activity, ipxeVisibilityToString, and the test fixtures. REST-side DB/API field naming is unchanged.
pbreton
force-pushed
the
feat/ipxe-templated-os-2
branch
from
July 14, 2026 23:59
3a2ec15 to
bbc1e93
Compare
thossain-nv
approved these changes
Jul 15, 2026
3 tasks
pbreton
added a commit
that referenced
this pull request
Jul 23, 2026
This PR continues implementation of Templated iPXE Operating System in NICo REST API. Adds the REST API layer for the Templated iPXE OS variant plus the read-only iPXE template endpoints. This builds on the DB models, migrations, proto conversions and inbound sync landed in the previous PRs — no new DB or workflow changes here; this is purely the external API surface, OpenAPI spec and regenerated Go SDK. Follow-up PR will implement Provider-specific behavior and capabilities. ## Related issues Builds on #3232 ## Type of Change - [X] **Add** - New feature or capability ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [X] Unit tests added/updated Signed-off-by: Patrice Breton <pbreton@nvidia.com>
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
2 participants
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 the synchronization layer for the Templated iPXE Operating System feature: the cloud-side inbound reconcile plus the site-agent / site-workflow inventory collectors that keep Operating System definitions and iPXE templates consistent between on-site NICo Core and NICo REST.
Additive workflow/collector wiring only; no schema or API changes.
Site-agent collectors discover Operating Systems and iPXE templates on-site and publish them as inventory; cloud inventory workflows reconcile that inventory into the DB.
Highlights:
UpdateOperatingSystemsInDBplus OS and iPXE-template inventory workflows/activities, registered inworkflow/cmd/workflow/main.go. Creates/updates provider-owned OSes reported by a site, skips Templated iPXE OSes whose template isn't available at the site, and soft-deletes Local iPXE OSes absent from site inventory.Related issues
Type of Change
Breaking Changes
Testing
Additional Notes