Conversation
Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
fcd29a8 to
c294f4e
Compare
|
FYI: @AlonaKaplan |
tzumainn
left a comment
There was a problem hiding this comment.
Hi! This is impressively thorough. I do have some initial questions!
-
If I'm reading this correctly, this proposal has O-SAC explicitly call BMC for power control. So if we use Ironic, we'd be using Ironic purely as an inventory source, and ignoring Ironic's bare metal management capabilities. Is that correct? If so, that architecture does make me a little nervous; it basically implies that O-SAC would essentially implement its own bare metal management platform.
-
Is there a reason the fulfillment service needs to have things like BMC information? If that information is present in the inventory, couldn't the reconciliation process grab them directly from the inventory?
-
The model ties Hosts directly to Clusters; however I had thought that the plan was to associate Hosts with HostPools, and then update the Cluster workflow to first create an associated HostPool (allowing for a more generic solution in the cases where we want O-SAC to use bare metal for non-OpenShift purposes).
No, the proposal isn't about O-SAC using directly the BMC, rather about having enough information to be able to tell other systems to do it. In the example of the integration with NVIDIA BCM what we did is get the details from NVIDIA BCM and then use them to create Metal3 BareMetalHosts to actually control the power on/off. Metal3 in turn uses Irnonic, so that at the end of the day it is actually Ironic that does the power management.
The reason to have the BMC information in the fulfillment service API/database is that that is what allows us to have decoupled inventory and provisioning systems. In the NVIDIA BCM scenario, for example, BCM does have that information, but we want to use Metal3 for provisioning. If we hadn't the information in the fulfillment service then those two systems would need to interact directly: either NVIDIA BCM learns how to use Metal3 for provisioning, or else Metal3 learns how to get the details from NVIDIA BCM. If there are N inventory sources supported and M provisioning systems this results in NxM integrations. If we have the information in the fulfillment service then only N+M integrations are needed.
That host-cluster relationship is what we used in the NVIDIA BCM scenario, for the sake of simplicity. I agree it would be better to have the host pool concept in the middle. |
Am I correct in thinking that this proposal also suggests that we should standardize on BareMetalHosts to perform any needed bare metal operations? So in the MOC environment, the Ironic used by MOC ESI would be relegated strictly to an inventory source, and O-SAC would create BareMetalHosts and perform operations upon the BareMetalHosts in order to perform functions such as power control (and underneath the hood, a separate standalone Ironic would be used). Is that correct? If so, I believe this goes against the documented architecture, where both the bare metal inventory and low level bare metal management layer are both replaceable. I have two other specific concerns:
It's my understanding that we want to separate out low level bare metal management - power control and network isolation - from higher level functions such as provisioning. From the MOC perspective, provisioning is often simply a matter of attaching a piece of hardware to a specific VLAN and powering it on.
If that's the case, would it make more sense for this proposal to suggest that we implement this at the host pool level? Implementing it at the cluster level and then going backwards seems like a potential exercise in frustration. |
Position BareMetalHost resources (from Metal3) as one possible provisioning mechanism rather than an integral part of the enhancement. The pluggable inventory source architecture should not mandate a specific technology. Changes: - Add explicit non-goal stating that specific provisioning mechanisms are out of scope - Make Hub Reconciler resource watcher description generic with HyperShift and Metal3 as an example - Refactor Workflow Integration to show a generic flow, with a separate subsection demonstrating the HyperShift/Metal3 implementation Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
|
@tzumainn I tried to clarify the intent around |
Thanks for the clarification! It helps to understand that you meant that as a possible example of a provisioning workflow. I think I'm still not a fan of having the fulfillment service collect BMC information, essentially creating an internal bare metal inventory. The issues I see are:
Would it make sense to have an intermediate (and optional) bare metal service that inventories can plug into? If that service exposes the same functions that ESI does, it should be simple to have the cluster fulfillment workflow switch between the two. |
| - Establish a synchronization pattern for inventory source adapters. | ||
| - Support hub configuration for HyperShift-based cluster provisioning. | ||
| - Enable host assignment tracking for clusters. | ||
| - Identify DNS provisioning as a required capability for cluster creation. |
There was a problem hiding this comment.
It's not obvious to me how this is related.
|
|
||
| - Define API extensions to capture hardware details needed for bare-metal provisioning. | ||
| - Establish a synchronization pattern for inventory source adapters. | ||
| - Support hub configuration for HyperShift-based cluster provisioning. |
There was a problem hiding this comment.
It's not obvious to me how this is related.
| |-------|------|-------------| | ||
| | `url` | `string` | URL of the BMC interface (e.g., `redfish-virtualmedia://10.0.0.1/redfish/v1/Systems/1`) | | ||
| | `user` | `string` | Username for BMC authentication | | ||
| | `password` | `string` | Password for BMC authentication | |
There was a problem hiding this comment.
I think we need a way to safely store secrets: #16 .
|
|
||
| ### Hub Reconciler | ||
|
|
||
| A new hub reconciler is introduced to manage the Kubernetes resources required for HyperShift-based |
There was a problem hiding this comment.
Commenting as I read... at this point I don't understand why hypershift is a factor. AFAIK the goal with bare metal is to get the hosts booted with a discovery ISO so they appear in ACM's host inventory (aka as Agent resources), and from there they can be assigned to clusters whether hypershift or not.
There was a problem hiding this comment.
It's not clear to me that we can use Agent resources for anything other than cluster installation (unless there have been changes to ACM of which I am not aware).
There was a problem hiding this comment.
This is an example of the information that is needed regarding hubs, should they be considered part of the inventory.
|
|
||
| 1. **Namespace**: A dedicated namespace for the fulfillment service resources. | ||
|
|
||
| 2. **Pull Secret**: A Kubernetes secret containing Docker registry credentials for pulling |
|
|
||
| 3. **Host Selection**: Same as above. | ||
|
|
||
| 4. **BareMetalHost Creation**: Reconciler creates `BareMetalHost` resources in the hub cluster |
There was a problem hiding this comment.
Don't we want this to happen prior to cluster creation, so that available hosts are already running the discovery ISO and appear as Agents ready to be assigned to a new or existing cluster?
There was a problem hiding this comment.
A motivating factor for importing hosts as agents before cluster creation is that the process of importing a node as an Agent can take a noticeable chunk of time. By importing nodes asynchronously, we can make the cluster deployment process much more responsive.
There was a problem hiding this comment.
Nothing here details when this happens. In the example of BCM integration this happens in the host reconciler, so as soon as a new host is created.
There was a problem hiding this comment.
The steps in your workflow are numbered, so I interpreted that as an ordering.
| Other provisioning mechanisms would implement steps 4-5 differently while maintaining the same | ||
| overall inventory synchronization and host selection patterns. | ||
|
|
||
| ### DNS Provisioning Requirements |
There was a problem hiding this comment.
I read this section, and it still seems like DNS is a separate problem to be solved on its own. I'm not clear on why it's in this proposal.
There was a problem hiding this comment.
It is in this proposal because is part of the example integration with BCM. I can remove it.
| - BMC credentials are stored in the fulfillment service database, requiring appropriate security | ||
| measures. | ||
|
|
||
| ## Alternatives |
There was a problem hiding this comment.
Could we reframe the problem as being local to each hub, with the main goal being to take inventory out of some external system and use it to create Agent resources (by booting the right discovery ISO, etc)? And any info we need to correlate with OSAC concepts could be labels, such as resource type, etc? That would make this generally useful even outside of OSAC, and would limit the need to store credentials and such directly in OSAC.
That would also fit nicely with the ACM pattern where the interface for making any kind of host available for provisioning comes down to "find a way to boot it with this ISO, which might or might not involve BMHs."
|
|
||
| ### Hub Reconciler | ||
|
|
||
| A new hub reconciler is introduced to manage the Kubernetes resources required for HyperShift-based |
There was a problem hiding this comment.
It's not clear to me that we can use Agent resources for anything other than cluster installation (unless there have been changes to ACM of which I am not aware).
|
|
||
| #### Example: HyperShift with Metal3 | ||
|
|
||
| As a concrete example, when using HyperShift with Metal3 for bare-metal provisioning, the workflow |
There was a problem hiding this comment.
For the sake of clarity, note that Metal3 is not involved in our bare metal deployments at this time.
There was a problem hiding this comment.
I know. This describes the example of the integration with BCM.
|
|
||
| 3. **Host Selection**: Same as above. | ||
|
|
||
| 4. **BareMetalHost Creation**: Reconciler creates `BareMetalHost` resources in the hub cluster |
There was a problem hiding this comment.
A motivating factor for importing hosts as agents before cluster creation is that the process of importing a node as an Agent can take a noticeable chunk of time. By importing nodes asynchronously, we can make the cluster deployment process much more responsive.
|
|
||
| Rather than embedding support for each inventory source directly into the fulfillment service, a | ||
| pluggable architecture allows: | ||
|
|
There was a problem hiding this comment.
I agree that we need something like this at some layer in the stack. I'm a little unclear what the workflow looks like between this proposed service and existing OSAC components. At the moment, the fulfillment service doesn't need to know details about how lower level components implement their specific functionality (e.g., it doesn't care whether we're using Ironic or something else entirely to manage bare metal resources).
|
|
||
| | Field | Type | Description | | ||
| |-------|------|-------------| | ||
| | `url` | `string` | URL of the BMC interface (e.g., `redfish-virtualmedia://10.0.0.1/redfish/v1/Systems/1`) | |
There was a problem hiding this comment.
I'm uncomfortable with the fulfillment service needing to handle credentials like this -- and not just the credentials, really, but needing to know this level of detail about the hardware resources. The fulfillment service shouldn't need to know whether a system is managed with redfish or ipmi or something entirely different. These should be details that are encapsulated in the inventory system; all the fulfillment service should need is the unique id of the host.
| | Field | Type | Description | | ||
| |-------|------|-------------| | ||
| | `bmc` | `BMC` | BMC (Baseboard Management Controller) connection details | | ||
| | `rack` | `string` | Physical rack location | |
There was a problem hiding this comment.
"Physical rack location" is insufficient to describe the location of a server in a data center, which may be organized into different slots, pods or power domains or other constructs (e.g., a server may be in "slot 2, position 4, rack 2, pod 1"). Our existing model translates the physical location of a server into a set of labels, which could then be used in some sort of match expression to select services matching specific criteria. I think that's a good model because it doesn't require us to make assumptions about how hardware is organized.
For example, an agent on the development cluster has a set of labels that look like this:
"topology.nerc.mghpcc.org/cabinet": "08",
"topology.nerc.mghpcc.org/pod": "a",
"topology.nerc.mghpcc.org/row": "4",
"topology.nerc.mghpcc.org/slot": "1a",
"topology.nerc.mghpcc.org/u": "31"
There was a problem hiding this comment.
That would mean that the component that select the hosts to be part of cluster is tied to the inventory source. Saying that because we might need to provision hosts for a cluster that are in different racks. fault domain, firecell, ...
There was a problem hiding this comment.
Just FYI:
There are a limited number of established labels for topology: https://kubernetes.io/docs/reference/node/node-labels/
There have been proposals to add more similar labels: kubernetes/kubernetes#130892
| | `url` | `string` | URL of the BMC interface (e.g., `redfish-virtualmedia://10.0.0.1/redfish/v1/Systems/1`) | | ||
| | `user` | `string` | Username for BMC authentication | | ||
| | `password` | `string` | Password for BMC authentication | | ||
| | `insecure` | `bool` | Whether to skip TLS certificate verification | |
There was a problem hiding this comment.
There also needs to be a mechanism for providing custom CA certificates.
| |-------|------|-------------| | ||
| | `pull_secret` | `string` | Docker configuration JSON for pulling container images | | ||
| | `ssh_public_key` | `string` | SSH public key to install on provisioned hosts | | ||
| | `ip` | `string` | IP address of the hub cluster (used for DNS configuration) | |
There was a problem hiding this comment.
I don't think we should ever need to record the ip address of the hub cluster. Can you give an example of when this would be necessary? I'm also unsure that this is relevant to this proposal, which is about inventory systems, not cluster provisioning.
There was a problem hiding this comment.
It was needed, for example, when we used a host port (instead of a load balancer) for the API server of the managed cluster. In that setup it is necessary to create a DNS record pointing to the IP of the management cluster.
| | Field | Type | Description | | ||
| |-------|------|-------------| | ||
| | `hosts` | `repeated string` | List of host identifiers assigned to this node set | | ||
|
|
There was a problem hiding this comment.
repeated is the key word using in .proto files to define lists of things.
|
|
||
| - Use event-based synchronization for real-time updates. | ||
| - Run periodic synchronization at longer intervals (e.g., hourly) to catch any missed events and | ||
| ensure eventual consistency. |
There was a problem hiding this comment.
When would the synchronizer miss an event? I don't the inventory system is going to be highly transactional.
There was a problem hiding this comment.
The synchronizer may be down, or the event may be lost for unknown reasons.
In our own system, for example, there is no guarantee that an event will be delivered.
|
|
||
| | Field | Type | Description | | ||
| |-------|------|-------------| | ||
| | `cluster` | `string` | Identifier of the cluster this host is assigned to | |
There was a problem hiding this comment.
A host might be assigned to something else than a cluster (can be a Host/HostPool today, maybe we would build other services on top of BM in the future?), we might need something more generic here?
|
|
||
| **Status fields:** | ||
|
|
||
| | Field | Type | Description | |
There was a problem hiding this comment.
should we expose if the machine is degraded (e.g.: fan is broken) or if a maintenance is planned or if is planned to be decommissioned?
|
|
||
| 2. **Cluster Creation**: Tenant requests a cluster with node sets specifying host class and count. | ||
|
|
||
| 3. **Host Selection**: Cluster reconciler selects unassigned hosts matching the requested host |
There was a problem hiding this comment.
Related to my comment above, I wonder if it wouldn't be of the inventory source to select hosts given some requirements (number of hosts, host class, distribution of the hosts in the datacenter for resiliency, and maybe other criteria that I'm missing)
| The specific implementation of DNS provisioning (which DNS server to use, authentication | ||
| mechanism, etc.) is deployment-specific and may vary between environments. The fulfillment | ||
| service should provide a pluggable or configurable mechanism for DNS updates, similar to the | ||
| inventory source pattern. |
There was a problem hiding this comment.
+1 we may want to provide a user-facing API to manage their DNS, like Route53 in AWS (end have the service provider to plug their underlying DNS provider)
| measures. | ||
|
|
||
| ## Alternatives | ||
|
|
There was a problem hiding this comment.
I think we should have a pluggable API on top of the inventory source that answers:
- capacity, return the host class, and the number of hosts available in each classes
- get, return hosts given a query (number of hosts, host classes, where is the datacenter, ...)
- other?
I think answering these questions might be different depending on the inventory sources.
| - **Loose coupling**: The core service has no knowledge of specific inventory sources. | ||
| - **Independent scaling**: Synchronizers can be scaled and configured independently. | ||
| - **Failure isolation**: Inventory source issues don't affect core service operations. | ||
| - **Flexible deployment**: Only the needed synchronizers are deployed for each environment. |
There was a problem hiding this comment.
Is there a need to update back the inventory source when a free host is picked up to be used?
Enhance the proposal based on review feedback to address several concerns and improve clarity: The motivation section now explains why inventory data should be stored in the fulfillment service database: it decouples provisioning components from inventory sources and reduces integration complexity from n×m to n+m. A new "Inventory Source API Abstraction" alternative is documented with an honest comparison. While on-demand querying avoids data duplication, it has disadvantages: poor query performance when inventory sources lack query capabilities, availability coupling with external systems, and complexity in building a flexible abstraction layer. A minimal synchronization example using Ironic with Ansible demonstrates that synchronizers and detailed inventory data are optional. Deployments where provisioning tools already communicate with the inventory source only need host identifiers and classes in the fulfillment service. The BMC configuration now includes a `trusted_cas` field for TLS certificate verification, complementing the existing `insecure` flag. DNS provisioning is moved from goals to non-goals and the DNS section is reframed as an example of how the pluggable architecture pattern could apply to other provisioning concerns. References to the secrets management enhancement proposal (PR osac-project#16) are added where credential storage is discussed. The non-goals section clarifies that a generic extension mechanism for custom properties will be implemented independently of this proposal. Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
|
Dear reviewers, as agreed in the meeting today I incorporated your feedback in the last commit: 844f5b7 . |
tzumainn
left a comment
There was a problem hiding this comment.
Thanks for the update! I left some additional comments.
One more point I want to bring up: I think this proposal defines a Host as a representation of a piece of hardware; however a previous proposal defined it as a tenant's ephemeral access to a piece of hardware. Does this proposal replace the latter with the former, or would the two live side-by-side?
| know how to communicate with each provisioning system. By centralizing inventory data in the | ||
| fulfillment service, only `n + m` integrations are needed: each inventory source synchronizes with | ||
| the fulfillment service, and each provisioning component reads from it. | ||
|
|
There was a problem hiding this comment.
I think I disagree with these points. They're only true if the inventory sources are providing the same information - but (for example) Ironic won't be able to provide BMC credentials; that means a provisioning system will have to treat Ironic differently from other inventory sources. It gets worse if (for example) a provisioning system requires information A, B, and C; and inventory sources X, Y, and Z all provide different combinations of that information to the fulfillment service and require the rest to be queried directly.
In my mind, that's why the current architecture makes sense: we say that provisioning requires functions A and B and C, so we figure out a singular interface and implement that interface for each bare metal inventory/management system. Then each provisioning workflow only needs to be written once, and we simply have to write a new interface for each new bare metal management system we want to integrate with - work that I believe would have to be done anyway.
| because: | ||
| - **Query performance**: Some inventory sources (such as BCM) lack query capabilities. Translating | ||
| fulfillment service queries into inventory source API calls would require fetching entire | ||
| collections and filtering in memory, which becomes expensive for large inventories. |
There was a problem hiding this comment.
This feels like a BCM-specific issue; to me, it makes more sense to build a BCM-specific layer that allows for these capabilities.
| - **Availability coupling**: The fulfillment service's ability to serve inventory data depends on | ||
| the availability of the underlying inventory source. If BCM is down, hosts cannot be listed. With | ||
| local storage, the fulfillment service remains operational even when the inventory source is | ||
| temporarily unreachable. |
There was a problem hiding this comment.
I kinda think if the inventory source is down, then the fulfillment service should have problems. If the original inventory source is the source of truth regarding the state of the hardware, then it makes sense to have OSAC pause if it can't query or update that inventory.
| temporarily unreachable. | ||
| - **Complexity in the abstraction layer**: Each inventory source has a different data model and | ||
| capabilities. Building a sufficiently flexible abstraction that works well with all sources while | ||
| maintaining good performance is challenging. |
There was a problem hiding this comment.
If we limit the abstraction layer to specific operations - power control, network attachment, etc - I don't think it's that complex.
Update the proposal based on further review feedback: The `rack` field is replaced with a `topology` map that accepts arbitrary location attributes (region, zone, cabinet, slot, etc.), allowing flexible physical location descriptions that provisioning components can translate into appropriate constructs. An `available` field is added to hosts to indicate whether a host can be allocated, handling degraded, maintenance, or decommissioned machines. The `cluster` field is removed from host status; host-to-cluster assignment will be tracked through a `HostPool` resource for more flexible allocation. The Ironic example is expanded to explain how inventory systems that don't expose BMC passwords work: administrators must choose provisioning components that work directly with the inventory system. A note about field growth is added to the BCM synchronizer section, clarifying that only the subset of inventory data required for provisioning is copied. A host selection section explains that provisioning components implement selection logic since some inventory systems (like BCM) lack this capability. The Metal3 example is reframed as hypothetical since it's not currently deployed, and the workflow is corrected to show that BareMetalHost resources are created during discovery, before cluster creation. Terminology is updated to use "container" instead of "Docker" throughout. Reverse synchronization is added to non-goals. Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
|
Dear reviewers, I tried to incorporate your feedback into the latest commit: 420f858 . Please check again. |
|
This proposal has been rejected. |
- Resolve AdminNetworksPage topology view wording contradiction (issue osac-project#3) - Specify IPv4/IPv6 CIDRs explicitly in FR-6 (issue osac-project#4) - Pick side drawer pattern for subnet detail display in FR-12 (issue osac-project#5) - Add Priority field to SecurityGroup rule specification in FR-18 (issue osac-project#6) - Standardize PublicIP action terminology to 'Release' in FR-28 (issue osac-project#7) - Document multi-NIC same-VN constraint rationale in FR-34 (issue osac-project#8) - Align wizard empty-state flow with inline overlay pattern in FR-38 (issue osac-project#9) - Define Retry action API contract in FR-42 (issue osac-project#10) - Clarify Subnet endpoints are create/delete only in FR-45 (issue osac-project#11) - Remove redundant NFR-9 (issue osac-project#12) - Update Open Question 8.2 wording to match Non-Goals Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
No description provided.