search

metal3-io / baremetal-operator

Bare metal host provisioning integration for Kubernetes
Apache License 2.0
590 stars 254 forks source link

Expected hardware configuration validation #351

Closed shubhamk14 closed 4 years ago

shubhamk14 commented 4 years ago

As a infrastructure provider we would like to validate expected hardware configuration for a give bare-metal host. The default minimum expected hardware configuration is as follows:

HardwareExpectedConfiguration: default: properties: cpu: count: 48 disk: sizeBytesGB: 100 numberOfDisks: 3 nics: OOB: Mgmt: Data: ram: sizeBytesGB: 64 firmware: version: 3.30.30.30 systemVendor: name: Dell required:

The above expected hardware configuration should be part of existing bare-metal yaml file. And this should be validated against introspection data. If inspected hardware configuration doesn't match above configuration, error should be raised.

zaneb commented 4 years ago

I don't understand why this needs to be part of the operator.

dhellmann commented 4 years ago

We've previously discussed making this part of the hardware profile matching logic. See https://github.com/metal3-io/metal3-docs/pull/4 for that history. The original design did call for the BareMetal operator to do the validation. After learning more about the k8s ecosystem, I'm a bit less happy with that approach.

I could see a separate operator coming around after inspection is completed by the baremetal-operator and comparing the results to expected values specified somewhere like a profile and using an annotation to mark the host as "invalid" if there is no match.

I could also see the cluster-api provider logic doing the comparison to determine if a host matches the criteria specified in a Machine or MachineSet and simply ignoring hosts that don't match. It could then report on the Machine when no hosts are available. The benefit of that approach is we could report errors the same way if no hosts match the label selector already specified in the Machine.

I like those approaches because they give us options for dealing with hosts that don't look like what we expect, while keeping the baremetal operator focused on performing the provisioning without introducing any hard rules that make it less flexible.

Would either of the approaches I propose solve the use case you have?

digambar15 commented 4 years ago

Thanks @dhellmann. Please go through the below inline comments. -

We've previously discussed making this part of the hardware profile matching logic. See metal3-io/metal3-docs#4 for that history. The original design did call for the BareMetal operator to do the validation. After learning more about the k8s ecosystem, I'm a bit less happy with that approach. ---------> We have gone through existing hardware profile implementation and saw that current profiles are hardcode which is not scalable or customizable approach.

I could see a separate operator coming around after inspection is completed by the baremetal-operator and comparing the results to expected values specified somewhere like a profile and using an annotation to mark the host as "invalid" if there is no match. --------> With the default profile hardcoded as it exists doesn't meet requirement of hardware vendor to match the service provider requirements of specific pools like BM compute host workloads, BM Network intensive hosts, BM Storage intensive hosts, BM HPC hosts. Thus we need a template for each one of those pools to configure and validate the hardware requirement. We believe this approach is our recommendation and we would like to persue this option.

I could also see the cluster-api provider logic doing the comparison to determine if a host matches the criteria specified in a Machine or MachineSet and simply ignoring hosts that don't match. It could then report on the Machine when no hosts are available. The benefit of that approach is we could report errors the same way if no hosts match the label selector already specified in the Machine. ---------> Workflow automation requires that, at the cluster-api level only requires search on the baremetal host pools to identify matching hosts. There is no need for detailed validation here.

I like those approaches because they give us options for dealing with hosts that don't look like what we expect, while keeping the baremetal operator focused on performing the provisioning without introducing any hard rules that make it less flexible. -------> We need both the approaches to cover hardware and software aspects and they are not optional. Both are mendatory for desired hardware configuration.

zaneb commented 4 years ago

Thus we need a template for each one of those pools to configure and validate the hardware requirement. We believe this approach is our recommendation and we would like to persue this option.

Just to clarify, you're recommending we pursue Doug's first suggestion of having a separate CRD for a pool, then having an operator that goes around checking which Hosts match the hardware specs for the pool and label them, and then a MachineSet would have a set of labels to look for when selecting a Host for a Machine? That makes a lot of sense to me.

dhellmann commented 4 years ago

Thus we need a template for each one of those pools to configure and validate the hardware requirement. We believe this approach is our recommendation and we would like to persue this option.

Just to clarify, you're recommending we pursue Doug's first suggestion of having a separate CRD for a pool, then having an operator that goes around checking which Hosts match the hardware specs for the pool and label them, and then a MachineSet would have a set of labels to look for when selecting a Host for a Machine? That makes a lot of sense to me.

I said annotation, but a label does make more sense because it would work with the existing label selector.

We would need a new optional field in the BareMetalHost for the expected profile. The current field in the spec section forcibly overrides the profile. I'm not sure what we would want the policy enforcement controller to do if the user specifies a profile that doesn't match the hardware.

zaneb commented 4 years ago

TBH I'm not clear on the value of having a hardware profile in the Host CRD at all in this scenario. Practical profiles are not going to be mutually exclusive, so it feels weird to be specifying exactly one in the first place. As far as validating that the hardware is what you expect: if you expect the Host to get some label/annotation (indicating that it matches a profile) and it doesn't have it, isn't that all the notice you need? What else do you gain by putting that knowledge into the CRD as well? What is the baremetal operator going to do about it?

dhellmann commented 4 years ago

TBH I'm not clear on the value of having a hardware profile in the Host CRD at all in this scenario. Practical profiles are not going to be mutually exclusive, so it feels weird to be specifying exactly one in the first place. As far as validating that the hardware is what you expect: if you expect the Host to get some label/annotation (indicating that it matches a profile) and it doesn't have it, isn't that all the notice you need? What else do you gain by putting that knowledge into the CRD as well? What is the baremetal operator going to do about it?

So maybe we remove the profile field we have in the spec section?

The operator wouldn't use a field specifying what profile is expected, so maybe that's an annotation instead of a field? I'm not sure where the line is between adding a field and using an annotation in this case.

arkadykanevsky commented 4 years ago

I think we are discussing two separate but related topics. One is node hw profile that includes all components of the node, like NICs, disks, Raid controllers, etc. That helps to decide if the node under consideration is the right candidate for the inclusion into a cluster.

Another is to configure the node for its role. For example, the same physical node can be used as base cluster node with remote storage, or in HCI mode. One will need to setup specific version of BIOS and FW to match the rest of the cluster, setup RAID0 or passthru, or RAID6; specific BIOS settings for virtualization or not, NICs for remote booting, etc. The same chosen node can be HW configured differently. MY understanding that this proposal addresses second point.

zaneb commented 4 years ago

That isn't my understanding. ~A lot of the stuff you mention in the second point already exists or is in progress (e.g. #319 is RAID support). The proposal is about a way to check that the detected hardware has the characteristics you expected it to have.

digambar15 commented 4 years ago

As discussed with @dhellmann on slack channel, I will submit a spec where we will bring more clarity on this proposal. @zaneb your understanding is right. There are two proposals we are driving from our side so @arkadykanevsky has mentioned about it.

digambar15 commented 4 years ago

Created a Spec for this issue - https://github.com/metal3-io/metal3-docs/pull/60 Please go through it

stbenjam commented 4 years ago

/label kind/feature

pramchan commented 4 years ago

Refer: https://github.com/metal3-io/metal3-docs/pull/60 Changes requested 1 review requesting changes Learn more.
1 change requested
@dhellmann Some checks haven’t completed yet 1 pending check @metal3-io-bot @metal3-io-bot generated this status. tide Pending — Not mergeable. Needs approved, lgtm labels.
What does lgtm lables mean? My understanding is if you are asking labels that Airship community will use it will depend on what pool of "expected host profile" we seek. eg. Hosts for "ControllerNodePool-00"... "~-99", "WorkloadNodePool-00"..."~-99", "StorageNodePool-00"..."~-99". The Airship or any other community using metal3-io can then assign aliases to label's as they need and define corresponding "ExpectedHarwareProfile-00"..."~99". Thus we don't have to wait to find what is legitimate labels. We leave the mapping to northbound callers and move on to approve this PR#60 and close this IR#351. Let me know if this makes sense?

dhellmann commented 4 years ago

Refer: metal3-io/metal3-docs#60 Changes requested 1 review requesting changes Learn more. 1 change requested @dhellmann Some checks haven’t completed yet 1 pending check @metal3-io-bot @metal3-io-bot generated this status. tide Pending — Not mergeable. Needs approved, lgtm labels. What does lgtm lables mean?

"lgtm" means "looks good to me" and is the way a PR is approved to be merged by the bot. It's a GitHub label, rather than a kubernetes resource label.

My understanding is if you are asking labels that Airship community will use it will depend on what pool of "expected host profile" we seek. eg. Hosts for "ControllerNodePool-00"... "~-99", "WorkloadNodePool-00"..."~-99", "StorageNodePool-00"..."~-99". The Airship or any other community using metal3-io can then assign aliases to label's as they need and define corresponding "ExpectedHarwareProfile-00"..."~99". Thus we don't have to wait to find what is legitimate labels. We leave the mapping to northbound callers and move on to approve this PR#60 and close this IR#351. Let me know if this makes sense?

I think you are saying that the names of labels applied to hosts should come from the user. I agree with that. One way to achieve it would be to label or annotate a host with the names of the HardwareClassification resources with rules that match that host.

metal3-io-bot commented 4 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle stale

dhellmann commented 4 years ago

I think it's safe to close this, now that we have approved https://github.com/metal3-io/metal3-docs/pull/60 and the work is happening in https://github.com/metal3-io/hardware-classification-controller

/close

metal3-io-bot commented 4 years ago

@dhellmann: Closing this issue.

In response to [this](https://github.com/metal3-io/baremetal-operator/issues/351#issuecomment-624293078): >I think it's safe to close this, now that we have approved https://github.com/metal3-io/metal3-docs/pull/60 and the work is happening in https://github.com/metal3-io/hardware-classification-controller > >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.