SAP: Use is_instance_storage_share instead of ssh #513
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fwiesel
force-pushed
the
use_is_instance_storage_share
branch
2 times, most recently
from
af93921 to
0fca032
Compare
fwiesel
changed the title
fwiesel
force-pushed
the
use_is_instance_storage_share
branch
3 times, most recently
from
d9ea76d to
3bcf566
Compare
|
Yuck, doesn't work as intended... one is an IP, the other is the compute name. |
fwiesel
force-pushed
the
use_is_instance_storage_share
branch
2 times, most recently
from
ed00f21 to
4936757
Compare
fwiesel
changed the title
fwiesel
force-pushed
the
use_is_instance_storage_share
branch
2 times, most recently
from
d5d53f6 to
6048802
Compare
fwiesel
force-pushed
the
use_is_instance_storage_share
branch
from
6048802 to
ff641ee
Compare
leust
reviewed
Dec 9, 2024
| @@ -691,7 +691,8 @@ def detach_interface(self, context, instance, vif): | |||
| def migrate_disk_and_power_off(self, context, instance, dest, | |||
| flavor, network_info, | |||
| block_device_info=None, | |||
| timeout=0, retry_interval=0): | |||
| timeout=0, retry_interval=0, | |||
| dest_compute=None): | |||
There was a problem hiding this comment.
Since you are changing this, might have been beneficial to bring in the whole migration object here.
Currently the VMware driver does an extra db call to fetch the migration by instance_uuid, which can be removed if we had the migration object already here.
What do you think?
There was a problem hiding this comment.
Good point. I assumed upstream chose explicitly not to expose the migration object, but if we need it in vmware-driver anyway...
I'll have a quick look how much I'd need to change for that.
There was a problem hiding this comment.
Opened #517 for that... but I am not sure, if the additional changes are paying off.
The libvirt-driver implements is_instance_storage_share twice: Once on driver-internally as `_is_path_shared_with` needing an ssh trust, and via the driver interface `is_instance_storage_share`, relying on RPC. By moving exposing `is_instance_storage_share` in ComputeVirtAPI, the driver can use this instead of the internal method removing the need for an SSH key in case of having a shared storage. But since it requires a change of the driver internal API, it is questionable if we can upstream the change as it is. Change-Id: Icddad1ab000ea07dea7af3aa5f7c18974f4eb8fb
fwiesel
force-pushed
the
use_is_instance_storage_share
branch
from
ff641ee to
ce60c32
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The libvirt-driver implements is_instance_storage_share twice:
Once on driver-internally as
_is_path_shared_withneeding an sshtrust, and via the driver interface
is_instance_storage_share,relying on RPC.
By moving exposing
is_instance_storage_sharein ComputeVirtAPI,the driver can use this instead of the internal method removing
the need for an SSH key in case of having a shared storage.
But since it requires a change of the internal driver API, it is questionable if
we can upstream the change as it is.
Change-Id: Icddad1ab000ea07dea7af3aa5f7c18974f4eb8fb