Skip to content

OSAC-4742: BCM label-based host selection - #738

Draft
mennyaboush wants to merge 1 commit into
osac-project:mainfrom
mennyaboush:feat/OSAC-4742-bcm-label-filtering
Draft

OSAC-4742: BCM label-based host selection#738
mennyaboush wants to merge 1 commit into
osac-project:mainfrom
mennyaboush:feat/OSAC-4742-bcm-label-filtering

Conversation

@mennyaboush

@mennyaboush mennyaboush commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

OSAC-4742: BCM Label Filtering

Story: [DEV] · Epic: OSAC-2857 Label-Based Host Selection · Design: OSAC-1201

Depends on #404 — merge this AFTER #404. This PR is intentionally scoped to just the BCM backend (bcm.go + its unit tests). It is based on main, so the diff is minimal, but it is semantically coupled to #404: only once #404 lands does the controller pass the raw HostSelector (no hostType injection) that this backend expects. On rebase onto post-#404 main, the one-line BCM controller integration fix (createBCMBMI → select by resource_class) is added alongside #404's version of that file.

Summary

Brings the BCM inventory backend to parity with the OpenStack/Metal3 label filtering added in #404 (which skipped BCM). BCMClient.FindFreeHost now matches the BareMetalInstanceType host_label_selector as arbitrary key=value equality (AND across keys) instead of the legacy single-dimension hostType/resource_class filter.

Approach

BCM is the client-side twin of the OpenStack backend, so it mirrors OpenStack's control flow and permissive validation (BCM labels are arbitrary extra_values keys filtered client-side, not Kubernetes labels).

Labels are matched against extra_values directly (not a namespaced osac_labels map). Nothing populates osac_labels anywhere, and unlike Ironic (where resource_class is a native field), BCM's resource_class already lives in extra_values — so matching extra_values directly keeps existing resource_class-only devices selectable with zero migration, while supporting arbitrary multi-label selectors. OSAC-internal keys (osac_instance_id, osac_bmc_address, osac_bmc_credentials_secret) are excluded.

  • managedBy: default-aware ownership guard (excluded from the generic match).
  • provisionState: excluded (CRD-reserved; no BCM analog).
  • Already-assigned hosts (osac_instance_id set) skipped; invalid selectors rejected before any BCM API call.
  • resource_class is no longer required/filtered — reported only.

Scope

Only internal/inventory/bcm.go + internal/inventory/bcm_test.go. The controller and other backends are unchanged (they're in #404).

Testing

go test ./internal/inventory/... passes, incl. new specs: arbitrary-label match, multi-label AND, OSAC-internal keys not matchable, per-device managedBy guard, and a validation table asserting no BCM query on invalid input. go build/go vet/gofmt clean.

Open design point

Matching extra_values directly is a conscious divergence from #404's OpenStack osac_labels namespacing (justified above). If an osac_labels populator is planned, we can switch to a namespaced map for cross-backend consistency — flagging for a quick decision.

@openshift-ci-robot

openshift-ci-robot commented Sep 3, 2026

Copy link
Copy Markdown

@mennyaboush: This pull request references OSAC-4742 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.1.0" version, but no target version was set.

Details

In response to this:

OSAC-4742: BCM Label Filtering

Story type: [DEV] · Epic: OSAC-2857 Label-Based Host Selection · Design: OSAC-1201

Depends on #404 (OSAC-3577/3578/3579). This branch is stacked on #404, so until #404 merges this PR's diff includes #404's commits; it collapses to the 3 BCM files once #404 lands.

Summary

Brings the BCM inventory backend to parity with the OpenStack/Metal3 label filtering added in #404, which deliberately skipped BCM. BCMClient.FindFreeHost now matches the BareMetalInstanceType host_label_selector as arbitrary key=value equality (AND across keys) instead of the legacy single-dimension hostType/resource_class filter.

Approach

BCM is the client-side twin of the OpenStack backend (fetch-all + filter-in-Go), so it mirrors OpenStack's control flow and permissive validation (BCM labels are arbitrary extra_values keys filtered client-side, not Kubernetes labels — Metal3's strict K8s-label validation intentionally does not apply here).

Label source — matched against extra_values directly (not a namespaced osac_labels map). Rationale: nothing populates osac_labels anywhere, and unlike Ironic (where resource_class is a native field), BCM's resource_class already lives in extra_values. Matching extra_values directly keeps existing resource_class-only devices selectable with zero migration, while supporting arbitrary multi-label selectors. OSAC-internal keys (osac_instance_id, osac_bmc_address, osac_bmc_credentials_secret) are excluded from matching.

  • managedBy: default-aware ownership guard (excluded from the generic match).
  • provisionState: excluded (CRD-reserved; no BCM analog).
  • Already-assigned hosts (osac_instance_id set) skipped.
  • Invalid selectors (empty map/key/value, key-with-spaces) rejected before any BCM API call.
  • resource_class is no longer a required field / filter — reported only.

Testing

  • go test ./internal/inventory/... passes, incl. new specs: arbitrary-label match, multi-label AND, OSAC-internal keys not matchable, per-device managedBy guard, and a validation table asserting no BCM query on invalid input.
  • Updated the BCM controller integration specs (createBCMBMI now selects by resource_class, matching the mock device fixtures).
  • go build/go vet/gofmt clean. Note: the controller envtest suite was not executed in the authoring sandbox (envtest binaries couldn't exec there) — needs a CI run.

Open design point for reviewers

Matching extra_values directly is a conscious divergence from #404's OpenStack osac_labels namespacing (justified above). If an osac_labels populator is planned, we can switch to a namespaced map for cross-backend consistency — flagging for a quick decision.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Comment @coderabbitai help to get the list of available commands.

@mennyaboush
mennyaboush force-pushed the feat/OSAC-4742-bcm-label-filtering branch from 4d38d24 to 86d9c5c Compare September 3, 2026 12:54
@openshift-ci

openshift-ci Bot commented Sep 3, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mennyaboush
Once this PR has been reviewed and has the lgtm label, please assign larsks for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mennyaboush
mennyaboush force-pushed the feat/OSAC-4742-bcm-label-filtering branch from 86d9c5c to 51b30da Compare September 3, 2026 13:53
Bring the BCM inventory backend to parity with the OpenStack/Metal3 label
filtering from osac-project#404 (OSAC-3577/3578/3579), which skipped BCM. BCMClient.FindFreeHost
now matches the BareMetalInstanceType host_label_selector as arbitrary key=value
equality (AND across keys) instead of the legacy hostType/resource_class filter.

BCM is the client-side twin of the OpenStack backend, so it mirrors OpenStack's
control flow and permissive validation. Selector keys are matched directly against
the device's extra_values -- BCM's native metadata bag, where resource_class
already lives -- excluding OSAC-internal keys (osac_instance_id/osac_bmc_address/
osac_bmc_credentials_secret). managedBy is a default-aware ownership guard;
provisionState is excluded (CRD-reserved, no BCM analog); assigned hosts skipped;
invalid selectors rejected before any BCM API call. resource_class is no longer a
required field/filter -- reported only.

Matching extra_values directly (rather than a namespaced osac_labels map like
OpenStack) keeps existing resource_class-only devices selectable with no
migration, since nothing populates osac_labels and BCM's resource_class already
lives in extra_values.

Depends on osac-project#404 and must merge after it: the controller only
passes the raw HostSelector (no hostType injection) once osac-project#404 lands. The BCM
controller integration specs (createBCMBMI) are updated together with osac-project#404's
version of that file when this branch is rebased onto post-osac-project#404 main.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
@mennyaboush
mennyaboush force-pushed the feat/OSAC-4742-bcm-label-filtering branch from 51b30da to f7dfb84 Compare September 3, 2026 13:58
@mennyaboush

Copy link
Copy Markdown
Contributor Author

⚠️ CI is expected to be red until #404 merges

This PR is intentionally minimal — it changes only the BCM inventory backend (internal/inventory/bcm.go) and its unit tests. It is semantically dependent on #404 and must merge after it:

The BCM label-selection flow itself is verified green. Running the integration suite in the combined post-#404 context (#404 controller + this bcm.go + the one-line createBCMBMI → resource_class test fix) against envtest:

BCM Integration: SUCCESS! — 10 Passed | 0 Failed   (full controller suite: 145 specs, ok)

Covers: full allocation, BMH readiness delay, full deallocation, missing BMC credentials, assignment contention, BCM unreachable (alloc + dealloc), BCM error response, and device-removed-during-polling.

On merge: once #404 lands, this branch will be rebased onto the post-#404 main, which re-adds the small createBCMBMI → resource_class integration-test fix (matching #404's version of that file). CI is green in that state.

The unit tests in this PR (bcm_test.go) call FindFreeHost directly and pass standalone (go test ./internal/inventory/... → ok).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants