Configure RAID Interface for iRMC driver by longkb · Pull Request #319 · metal3-io/baremetal-operator

longkb · 2019-10-10T14:53:51Z

Currently, RAID interface is not set when we create Ironic node(#320). This PR aims to make RAID interface to be set by vendor driver.

Signed-off-by: Kim Bao Long longkb@vn.fujitsu.com
Co-Authored-By: Dao Cong Tien tiendc@vn.fujitsu.com

longkb · 2019-10-17T03:09:27Z

Hi @dhellmann , @dtantsur
This PR aims to add bootInterface into accessDetail, that needs for RAID configuratuon. Please help me to review it if you have a time :)

longkb · 2019-10-24T09:38:26Z

/cc dtantsur
Could you help me to review this PR? :)

zaneb · 2019-10-25T15:23:50Z

 }
+
+func (a *iDracAccessDetails) RAIDInterface() string {
+	return "no-raid"


Is there a reason not to set it to "agent"?

RAID configuration is going to be added in a driver-independent way, so I'd have thought we'd want to ensure that all drivers can use it somehow.

Is there a reason not to set it to "agent"?

I would like to set it as no-raid as default interface

RAID configuration is going to be added in a driver-independent way, so I'd have thought we'd want to ensure that all drivers can use it somehow.

If vendor driver want to support RAID configuration, they can change no-raid to their driver

Note that software RAID is vendor-independent.

Note that software RAID is vendor-independent.

Yep. But sofware RAID is out of scope in this version of RAID configuration

The default RAID interface for the iDrac driver in Ironic is idrac-wsman (unlike the iRMC driver, where the default is no-raid). So this change actually disables RAID support in iDrac where before it worked fine, while enabling it in iRMC where before it was disabled.

I think that for all of the non-iRMC drivers you should return an empty string so that it continues to use the driver default instead of overriding it to no-raid in every case.

Thanks @zaneb
You're right. I have just update my PR to set RAID interface of non-iRMC drivers to empty string as you said
Please keep going on :)

arkadykanevsky · 2019-12-03T16:53:05Z


 	// Boot interface to set
 	BootInterface() string
+


This is fine for a single RAID. But in many cases you have 2 RAIDs.
One for 2 or small # of disks for Boot/Operating system.
Another one, including NORAID for remaining disks.

This applies to the whole patch.
It will be too hard to retrofit later.

Do you have a specific proposal for a different API? As far as I can tell, the gopher cloud client for Ironic only accepts a single string. Are you proposing that it should take multiple values? Or are you proposing that the user needs to somehow specify the value?

Doug,
I am thinking that a variable length array will be sufficient.
First, parameter the size of array. Default 1.
The second is array of RAIDinterfaces. Each raid interface will be a tuple. First, part of tuple will be RAID type, second is list (or array) of disks in that RAID.

I recall that Ironic has a way to specify more than one RAID per node. Would be good to check how they specify it. Maybe Digmabar can comment on it.

This is an internal API, for statically defined types that let us pass different values to ironic based on the BMC type the user gives us. I think the value being returned here is telling ironic which RAID driver to use within itself (note the values returned by the methods below), rather than specifying individual controllers on a host. Are you saying we may need to tell ironic multiple values in that case?

Yes, this is just telling the Ironic driver which RAIDInterface to use to configure RAID on this particular machine. So e.g. for the iDRAC driver the options are idrac-wsman/idrac, or no-raid to disable RAID support altogether (or agent, to configure it in software). Perhaps one day there'll be an idrac-redfish too. But this has nothing to do with what RAID arrays are to be created, that's in #292. All arrays are configured, or not configured, through the same interface.

Yes, I agree with Arkady. IRonic support multiple Raid configuration for RAID. We should allow use to create multiple RAID which would be helpful for configuring root disk (say RAID1) and remaining disks (RAID 0 or non-raid) depending on the RAID controller. This will certainly good feature for us as Ironic is already supporting it.
@zaneb Yes. You understanding is correct. Above implementation is irrespective of which driver you are using whether it's idrac-wsman/idrac/idrac-redfish.

That's exactly what's proposed in the current PR: https://github.com/metal3-io/baremetal-operator/pull/292/files#diff-2336c3885f739a6a5c66bc71e72497b3R133-R140

zaneb · 2019-12-09T14:43:11Z

 		p.publisher("Registered", "Registered new host")

+		raidInterface := p.bmcAccess.RAIDInterface()
+		if raidInterface != "" {


The type of the RAIDInterface member in nodes.CreateOpts is string, so it will be initialised to an empty string on lines 239-244 anyway. There's no harm in just passing

RAIDInterface: p.bmcAccess.RAIDInterface(),

after line 241. You don't need to do anything special to check if the BMC plugin is returning an empty string.

Thanks @zaneb . You're right.
But it seems that these Interfaces already added by [1]
So I would like to configure the RAID interface to irmc in this PR :)

[1] c8fdf04#diff-c3f8fb66d64901a306b6faf6f7019c9f

zaneb · 2019-12-10T02:16:40Z

/lgtm

zaneb · 2019-12-10T02:20:53Z

/approve

metal3-io-bot · 2019-12-10T02:20:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: longkb, zaneb

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [zaneb]

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

Currently, RAID interface of iRMC is set as empty when we create Ironic node. This PR aims to configure this interface to "irmc" to support RAID configuration in iRMC driver. Signed-off-by: Kim Bao Long <longkb@vn.fujitsu.com> Co-Authored-By: Dao Cong Tien <tiendc@vn.fujitsu.com>

METAL-376: Re-enable RAID interfaces

metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 10, 2019

metal3-io-bot assigned dtantsur Oct 22, 2019

metal3-io-bot requested a review from dtantsur October 24, 2019 09:38

zaneb reviewed Oct 25, 2019

View reviewed changes

zaneb mentioned this pull request Dec 3, 2019

Expected hardware configuration validation #351

Closed

arkadykanevsky reviewed Dec 3, 2019

View reviewed changes

zaneb mentioned this pull request Dec 6, 2019

Add idrac redfish virtual media bmc support #359

Merged

longkb force-pushed the enable_set_raid_interface branch from 4f62bfe to df1488e Compare December 9, 2019 03:21

metal3-io-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 9, 2019

zaneb reviewed Dec 9, 2019

View reviewed changes

longkb force-pushed the enable_set_raid_interface branch from df1488e to bbf975b Compare December 10, 2019 01:46

metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 10, 2019

longkb force-pushed the enable_set_raid_interface branch from bbf975b to 53381c8 Compare December 10, 2019 02:01

metal3-io-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 10, 2019

longkb changed the title ~~Allows RAID Interface to be set by vendor driver~~ Configure RAID Interface for iRMC driver Dec 10, 2019

metal3-io-bot assigned zaneb Dec 10, 2019

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 10, 2019

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 10, 2019

metal3-io-bot merged commit 2c34f25 into metal3-io:master Dec 10, 2019

zaneb mentioned this pull request Dec 10, 2019

Allows RAID Interface to be set by vendor driver #320

Closed

zaneb mentioned this pull request Mar 23, 2021

Change default of RAIDInterface #829

Merged

honza pushed a commit to honza/baremetal-operator that referenced this pull request Jan 5, 2024

Merge pull request metal3-io#319 from MahnoorAsghar/redfish-raid-ocp

99b535c

METAL-376: Re-enable RAID interfaces

Conversation

longkb commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

longkb commented Oct 17, 2019

Uh oh!

longkb commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaneb commented Dec 10, 2019

Uh oh!

zaneb commented Dec 10, 2019

Uh oh!

metal3-io-bot commented Dec 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

longkb commented Oct 10, 2019 •

edited

Loading

longkb commented Oct 24, 2019 •

edited

Loading