Repository: metal3-io/baremetal-operator
Pull request #1033 (2021-11-19 08:24:45)

irmc: change the return value of NeedsMAC() to true
When adding iRMC type BMH in OCP environment, if `bootMACAddress` is not specified, the following error will occur:
```
Events:                              
  Type    Reason             Age   From                         Message
  ----    ------             ----  ----                         -------
  Normal  RegistrationError  47s   metal3-baremetal-controller  MAC address  conflicts with existing node openshift-machine-api~master-0
  Normal  RegistrationError  47s   metal3-baremetal-controller  MAC address  conflicts with existing node openshift-machine-api~master-0

```
So modify the return value of `NeedsMAC` so that `bootMACAddress` must be specified when creating a BMH.


Signed-off-by: Zhou Hao <zhouhao@fujitsu.com>

metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **fmuyassarov** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @fmuyassarov` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["fmuyassarov"]} -->
metal3-io-bot: Hi @zhouhao3. Thanks for your PR.

I'm waiting for a [metal3-io](https://github.com/orgs/metal3-io/people) 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](https://github.com/orgs/metal3-io/people) 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](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details>

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.
</details>

andfasano: Hi @zhouhao3,
can you please add some description to the PR (and link an issue if present)? Thanks.


zhouhao3: @andfasano Uupdated, thanks.
hardys: /ok-to-test
/lgtm
metal3-io-bot: @zhouhao3: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit | b55cc4438a8ff5c94ff004a9f5cbfac43e3b47f5 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1033/unit/1465643311686488064) | true | `/test unit`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1033). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:zhouhao3).

<details>

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. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
hardys: lgtm but seems like the test failure is real

```
$ go test .
--- FAIL: TestStaticDriverInfo (0.00s)
    --- FAIL: TestStaticDriverInfo/irmc (0.00s)
        access_test.go:657: MAC needed: true , expected false
FAIL
FAIL	github.com/metal3-io/baremetal-operator/pkg/ironic/bmc	0.005s
FAIL
```


metal3-io-bot: @zhouhao3: PR needs rebase.

<details>

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.
</details>
metal3-io-bot: New changes are detected. LGTM label has been removed.

Patch:
diff --git a/pkg/hardwareutils/bmc/irmc.go b/pkg/hardwareutils/bmc/irmc.go
index c6b12f83..1bfa03f9 100644
--- a/pkg/hardwareutils/bmc/irmc.go
+++ b/pkg/hardwareutils/bmc/irmc.go
@@ -31,7 +31,9 @@ func (a *iRMCAccessDetails) Type() string {
 // NeedsMAC returns true when the host is going to need a separate
 // port created rather than having it discovered.
 func (a *iRMCAccessDetails) NeedsMAC() bool {
-	return false
+	// For the inspection to work, we need a MAC address
+	// https://github.com/metal3-io/baremetal-operator/pull/284#discussion_r317579040
+	return true
 }
 
 func (a *iRMCAccessDetails) Driver() string {


Test patch:
diff --git a/pkg/hardwareutils/bmc/access_test.go b/pkg/hardwareutils/bmc/access_test.go
index 833badb6..9dcd84e2 100644
--- a/pkg/hardwareutils/bmc/access_test.go
+++ b/pkg/hardwareutils/bmc/access_test.go
@@ -476,7 +476,7 @@ func TestStaticDriverInfo(t *testing.T) {
 		{
 			Scenario:   "irmc",
 			input:      "irmc://192.168.122.1",
-			needsMac:   false,
+			needsMac:   true,
 			driver:     "irmc",
 			bios:       "",
 			boot:       "pxe",