Change default of RAIDInterface#829
Conversation
metal3-io-bot
added
the
size/XS
|
/test-integration |
| func (p *ironicProvisioner) buildManualCleaningSteps() (cleanSteps []nodes.CleanStep, err error) { | ||
| // Build raid clean steps | ||
| if p.bmcAccess.RAIDInterface() != "" { | ||
| if p.bmcAccess.RAIDInterface() != "" && p.bmcAccess.RAIDInterface() != "no-raid" { |
There was a problem hiding this comment.
TBH I don't think it's correct to ever assume that the default (empty string) means no-raid, since that's only true in the case of one driver (that doesn't return "" - see #319). I think a real fix would be to explicitly set the RAID interface in every BMC driver and only check for no-raid here.
There was a problem hiding this comment.
Sure I can go and change the bmcs with "" to no-raid and update this here =)
There was a problem hiding this comment.
Do we want to look for the string no-raid or do we want a bmcAccess method like RAIDSupported() that returns a bool?
There was a problem hiding this comment.
@dhellmann I would say it's fair to look at no-raid since this would say raid it's not supported, so having no-raid as default instead of "" would be better
Instead of using `""` we will be using `no-raid`.
The support for RAID was considering RAIDInterface defaul value as `""`
to generate the clean steps, this is causing errors in vmedia
deployments with idrac because it uses `no-raid`:
"Node failed to start the first cleaning step. Error: node does not support
this clean step: {'interface': 'raid', 'step': 'delete_configuration'}"
metal3-io-bot
added
size/M
iurygregory
changed the title
|
/test-integration |
|
|
||
| func (a *testAccessDetails) RAIDInterface() string { | ||
| return "" | ||
| return "no-raid" |
There was a problem hiding this comment.
What about using a constant for such value?
There was a problem hiding this comment.
we can probably think about creating possible values for raid as a constant (In a different PR I would say, since this is blocking deployments with virtual media)
|
/approve I agree re adding a constant, but otherwise the change is correct. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtantsur, iurygregory The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
metal3-io-bot
added
the
approved
|
+1 for a constant but since the world is broken let's fix that first. |
6 participants
Instead of using
""we will be usingno-raid.The support for RAID was considering RAIDInterface defaul value as
""to generate the clean steps, this is causing errors in vmedia
deployments with idrac because it uses
no-raid:"Node failed to start the first cleaning step. Error: node does not support
this clean step: {'interface': 'raid', 'step': 'delete_configuration'}"