🌱 Exclude hosts with virtual media from PROVISIONING_LIMIT #1173
Conversation
The limit was designed to avoid glitches when too many hosts DHCP or boot from PXE. Virtual media requires neither, and with a pre-built ISO is actually quite efficient.
metal3-io-bot
added
the
size/M
|
/test-centos-integration-main |
|
/lgtm |
| @@ -1938,7 +1948,7 @@ func (p *ironicProvisioner) loadBusyHosts() (hosts map[string]struct{}, err erro | |||
|
|
|||
| hosts = make(map[string]struct{}) | |||
| pager := nodes.List(p.client, nodes.ListOpts{ | |||
| Fields: []string{"uuid,name,provision_state,driver_internal_info,target_provision_state"}, | |||
| Fields: []string{"uuid,name,provision_state,boot_interface"}, | |||
There was a problem hiding this comment.
I don't remember the reason why these fields were requested initially, but if they're not required anymore then it seems fine to do some cleanup
There was a problem hiding this comment.
I think we wanted to use agent_url from driver_internal_info, but then dropped this idea.
There was a problem hiding this comment.
Ok. Just wondering if it could be worth moving this refactoring into a separate commit for the sake of clarity with a proper comment (another PR seems overkilling in this case)
| // media deployments may work without DHCP and can share the same image. | ||
| if bmcAccess.SupportsISOPreprovisioningImage() { | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
I understand that this could be an handy centralized place to perform this check, and it may sound as a reasonable location froma technical point of view. Anyhow I'd evaluate also the possibility to "push it up" in the stack - where the ensureCapacity is invoked, essentially in the updateHostStateFrom and checkDelayedHost. Not to save any comp cycle, but just to move such logic, which is strictly host dependent, out from the provisioner - so that the opt out decision could be made earlier
There was a problem hiding this comment.
I'm not sure I'm following, this logic is also provisioner-dependent, it relies on knowing how Ironic works. We'd also need to move the filtering of the busy hosts up the stack somehow, it's going to be a significant change.
There was a problem hiding this comment.
I was thinking on a more general mechanism to avoid applying the delay check mechanism at all based on some predicate (in this case it'd be based on bmcAccess.SupportsISOPreprovisioningImage(), but in future it could be something else, maybe an host label) (btw, using a label wouldn't need to recompile necessary the code in case a new scenario will pop up).
I won't block on this, but I'd consider a more generic mechanism in future if other scenarios will arise
|
|
||
| // If the current host uses virtual media, do not limit it. Virtual | ||
| // media deployments may work without DHCP and can share the same image. | ||
| if bmcAccess.SupportsISOPreprovisioningImage() { |
There was a problem hiding this comment.
General question: the delay mechanism gets applied also during deprovisioning, did you maybe also evaluate the impact of disabling it for vmedia hosts also during this phase?
There was a problem hiding this comment.
It matters even less there: cleaning works completely locally.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
metal3-io-bot
added
the
approved
furkatgofurov7
changed the title
The limit was designed to avoid glitches when too many hosts DHCP or
boot from PXE. Virtual media requires neither, and with a pre-built ISO
is actually quite efficient.