Improve LinstorSR.py to handle thick SR creation
#85
Conversation
drivers/LinstorSR.py
Outdated
| for attempt in range(3): | ||
| online_hosts = util.get_online_hosts(self.session) | ||
| if len(online_hosts) >= len(host_adresses): | ||
| util.SMlog("DEBUG: All hosts online") | ||
| break # Ok and proceed to create | ||
| else: | ||
| util.SMlog("DEBUG: Online host: {} ; Adresses: {}".format(online_hosts, host_adresses)) | ||
| time.sleep(15) # Sleep and retry |
There was a problem hiding this comment.
I think it's better to modify the logic of get_online_hosts instead (without using host metrics, just enabled flag I guess) and not try to add a sleep call during creation.
There was a problem hiding this comment.
Can we look for reason on why host metrics were used here? Or we can introduce get_enabled_hosts to be at par with semantics and use it while leaving get_online_hosts as is. Will go with your call.
There was a problem hiding this comment.
The function get_online_hosts predate the git history, it won't be easy to know why
There was a problem hiding this comment.
@rushikeshjadhav I agree with you, a new function is probably a good idea. This would avoid breaking the use of the current function in cleanup.py.
There was a problem hiding this comment.
By looking at code, online_hosts seems necessary to fetch hostname and its ip address. There is verification logic about Multiple hosts with same hostname.
Further it calls _prepare_sr_on_all_hosts with enabled=True which subsequently calls _prepare_sr that works if host is enabled. So checking for enabled may be redundant.
This might have to be verified with someone from xapi that if fetching ip address matters in cases of enabled vs online. We can switch to enabled if ip address and hostnames are available at this stage or have to revert to online and wait for it.
There was a problem hiding this comment.
I think enabled = True should be a sufficient condition for hostname and address to be available. Host_metrics.live is true depending on connectivity with the coordinator host checked during heartbeat, so if this is important instead of just the fact of availability of the data in the database, it should be used instead.
There was a problem hiding this comment.
@last-genius Thanks. I have updated the code accordingly.
rushikeshjadhav
force-pushed
the
3.2.3-8.3-online-hosts
branch
2 times, most recently
from
697676c to
2b681af
Compare
|
I changed the base branch to 3.2.12-8.3, can you rebase on it? Thanks! :) |
drivers/util.py
Outdated
| """ | ||
| Returns a list of host refs that are enabled in the pool. | ||
| """ | ||
| enabled_hosts = [host[0] for host in session.xenapi.host.get_all_records_where('field "enabled" = "true"').items()] |
There was a problem hiding this comment.
| enabled_hosts = [host[0] for host in session.xenapi.host.get_all_records_where('field "enabled" = "true"').items()] | |
| enabled_hosts = list(session.xenapi.host.get_all_records_where('field "enabled" = "true"').keys()) |
Wouldn't that do the trick just fine?
There was a problem hiding this comment.
if only refs are needed, session.xenapi.host.get_all_where('field "enabled" = "true"') is available, no need to get the records
There was a problem hiding this comment.
if only refs are needed,
session.xenapi.host.get_all_where('field "enabled" = "true"')is available, no need to get the records
I'm getting XenAPI.Failure: ['MESSAGE_METHOD_UNKNOWN', 'host.get_all_where'], is it due to my pending updates?
There was a problem hiding this comment.
Hmm, are you running 8.2? Apparently this was only added in March 2024, so should probably keep the get_all_records_where for now, could just leave a note to upgrade it in the future (and for the neighboring functions too)
There was a problem hiding this comment.
Its 8.3.0 with xapi-core-24.19.2-1.9.xcpng8.3.x86_64
…In some drivers e.g. Linstor, we need to ensure that hosts are enabeld before performing operations, hence this function is needed. Signed-off-by: Rushikesh Jadhav <[email protected]>
…as the metrics could take time. Thus, changed the mechanism to `get_enabled_hosts`. Signed-off-by: Rushikesh Jadhav <[email protected]>
rushikeshjadhav
force-pushed
the
3.2.3-8.3-online-hosts
branch
from
2b681af to
2395ddb
Compare
|
@rushikeshjadhav Have you tested this code in real condition already? |
Yes, it's being referred and tested in xcp-ng/xcp-ng-tests#282. It's in Draft and will convert once done. |
|
I won't have time to review this one. |
In some cases
thickSR creation may fail due toget_online_hostsas the metrics could take time. Thus, changed the mechanism toget_enabled_hosts.