Assume host is unavailable until inspection is done.

[s3rj1k]

Signed-off-by: seivanov seivanov@mirantis.com

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: s3rj1k
To complete the pull request process, please assign maelk
You can assign the PR to them by writing /assign @maelk in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[metal3-io-bot]

Hi @s3rj1k. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 kubernetes/test-infra repository.

[s3rj1k]

/assign @maelk

[dhellmann]

I'm not sure this is the right place to be making the decision. We probably shouldn't have that Available() method at all, since it should be up to the user of the API to decide which hosts it thinks are usable. The host has a status field now that it didn't have before, so we could change CAPM3 to look at that, like we do in CAPBM.

[s3rj1k]

@dhellmann Currently there is also a similar check in https://github.com/metal3-io/baremetal-operator/blob/master/apis/metal3.io/v1alpha1/baremetalhost_types.go#L744

Removing Available() can possible break integration with third-party users of BM-operator.

Can we do this check in BM-operator?

CAPM3: https://github.com/metal3-io/cluster-api-provider-metal3/blob/master/baremetal/metal3machine_manager.go#L785

[s3rj1k]

How about calling NeedsHardwareInspection() method from within Avaliable() method?

[dhellmann]

@dhellmann Currently there is also a similar check in https://github.com/metal3-io/baremetal-operator/blob/master/apis/metal3.io/v1alpha1/baremetalhost_types.go#L744

Removing Available() can possible break integration with third-party users of BM-operator.

Can we do this check in BM-operator?

CAPM3: https://github.com/metal3-io/cluster-api-provider-metal3/blob/master/baremetal/metal3machine_manager.go#L785

What I'm proposing is that we properly separate the concerns. The Available() method is from a time when I had not thought through the implications of having the host type control when provisioning could be requested. That's not really its job, though.

We should deprecate Available() and change metal3machine_manager.go to perform all of the checks it wants. Then, if we decide CAPM3 should not provision a host until after inspection we can apply that rule there without forcing it on other consumers.

It's not clear, though, why we consider the availability of a host related to whether inspection has been performed. The baremetal-operator will just wait to provision the host if the image settings are applied before inspection.

[dhellmann]

See metal3-io/cluster-api-provider-metal3#150 and #741 instead.

[s3rj1k]

The idea to have one single method to check in CAPM3 (or possible forks) versus multiple of aggregated methods.

It seems that providing a single method to check availability is more convenient.

[dhellmann]

The idea to have one single method to check in CAPM3 (or possible forks) versus multiple of aggregated methods.

It seems that providing a single method to check availability is more convenient.

Well, you're adding a rule that I don't think we want in CAPBM, so it's not actually convenient. :-)

[s3rj1k]

@dhellmann Please do not close this PR before a merge of
metal3-io/cluster-api-provider-metal3#150
#741

[zaneb]

The provisioning state is a far superior way of determining whether a host can be provisioned than the hardware details.
If we're going to decide for people what available means, we should expose it to the k8s API as a condition in the CRD, not as a golang function. We probably should never have implemented methods on the BareMetalHost struct.
I actually don't have a problem with just deleting those methods. It won't break anybody immediately, only when they re-vendor. I don't recall us promising anywhere that we would keep this stuff stable so that you can always re-vendor without changes, so it's fair game.

[s3rj1k]

@zaneb Still feels bad that there is no promise of stable API

[dhellmann]

@zaneb Still feels bad that there is no promise of stable API

The API is the CRD, and that's not changing, right?

[s3rj1k]

@zaneb Still feels bad that there is no promise of stable API

The API is the CRD, and that's not changing, right?

API on a level of k8s indeed does not change, but on a golang level (when some one imports code) it does change.

[zaneb]

@zaneb Still feels bad that there is no promise of stable API

In the golang ecosystem not even actual libraries offer stable APIs, let alone internal implementations that were never designed to be used as libraries.

[s3rj1k]

@zaneb Still feels bad that there is no promise of stable API

In the golang ecosystem not even actual libraries offer stable APIs, let alone internal implementations that were never designed to be used as libraries.

Well this is a matter of declaring this officially, do any of metal3 projects have this declared somewhere?
Maybe I missed something when reading docs?

Also they do say to vendor, this is ok when they do frequent tag releases.

[s3rj1k]

I think we should create a separate issue about tag releases and vendoring.

What do you all think?

@dhellmann @zaneb @maelk

[dhellmann]

I have no problem with tagging more releases. But yes, let's discuss that elsewhere.

[s3rj1k]

#740 (comment)