feat: add support for iscsidirect protocol#63
feat: add support for iscsidirect protocol#63plieven wants to merge 6 commits intokolesa-team:mainfrom
Conversation
Signed-off-by: Peter Lieven <pl@dlhnet.de>
we need this later to get the LUN for a connected volume. So this introduces the GET method for connections. Consequently use this also to connect a volume if it is not yet connected and disconnect it only if it is currently connected. Signed-off-by: Peter Lieven <pl@dlhnet.de>
this adds support protocol type iscsidirect which allows to connect to a pure store without the need for open-iscsi or multipathd. Signed-off-by: Peter Lieven <pl@dlhnet.de>
Signed-off-by: Peter Lieven <pl@dlhnet.de>
plieven
force-pushed
the
feat/iscsidirect
branch
from
90dd0c2 to
702c023
Compare
amulet1
left a comment
amulet1
left a comment
There was a problem hiding this comment.
Overall this is a good addition.
PureStoragePlugin.pm
Outdated
| $name = 'create volume connection'; | ||
| $ignore = 'Connection already exists.'; | ||
| my $ignore = ''; | ||
| if ( !defined($mode) ) { |
There was a problem hiding this comment.
Could we instead of using undefined $mode for PATCH change it as following:
DELETE if $mode == -1, "PATCHif$mode == 0, 'POST' if $mode == 1`?
I think it would make it "cleaner". But also please see my other comments, as I think the connection check is not really needed.
There was a problem hiding this comment.
We have DELETE, GET and POST. We can use -1, 0 and +1 for that.
There was a problem hiding this comment.
Yes, I meant to write GET, not PATCH.
PureStoragePlugin.pm
Outdated
|
|
||
| my $protocol = $scfg->{ protocol } // $default_protocol; | ||
| if ( $protocol eq "iscsidirect" ) { | ||
| die "Error :: path: snapshot is not implemented ($snapname)\n" if defined($snapname); |
There was a problem hiding this comment.
This die should be done for all protocols.
Also, for consistency the whole thing should probably be in filesystem_path(), not in path().
There was a problem hiding this comment.
filesystem_path is no longer called if path is implemented. But you are right, it should be there for all protocols and can be remove from filesystem_path.
There was a problem hiding this comment.
Plugin.pm calls filesystem_path() although it looks like this is for filesystem-based storage. Still, since we have it, the path() logic probably should be moved there.
PureStoragePlugin.pm
Outdated
| if ( $protocol eq "iscsidirect" ) { | ||
| die "Error :: path: snapshot is not implemented ($snapname)\n" if defined($snapname); | ||
|
|
||
| my $lun = $class->purestorage_volume_connection( $scfg, $volname ); |
There was a problem hiding this comment.
Why not just always try to connect volume here? Checking if it exists first just adds an extra API call in case if volume is not already connected.
The original purestorage_volume_connection() was ignoring Connection already exists. error for that reason.
There was a problem hiding this comment.
Here I need the version with GET because only this call returns the LUN id. If the connection already exists the LUN is not returned.
PureStoragePlugin.pm
Outdated
|
|
||
| my $serial = $response->{ items }->[0]->{ serial } or die "Error :: Failed to retrieve volume serial"; | ||
|
|
||
| my $protocol = $scfg->{ protocol } // $default_protocol; |
There was a problem hiding this comment.
Let's create and use a supporting function to retrieve protocol value. Something like
sub get_protocol() {
my ($scfg) = @_;
return $scfg->{ protocol } // $default_protocol;
}
PureStoragePlugin.pm
Outdated
| if (!$lun) { | ||
| $class->purestorage_volume_connection( $scfg, $volname, 1 ); | ||
| } | ||
| my $protocol = $scfg->{ protocol } // $default_protocol; |
There was a problem hiding this comment.
Again, I see no real need to check for existing connection if in the end we just want to make sure the volume is connected.
There was a problem hiding this comment.
Here it is okay to call POST and DELETE unconditally and ignore the errors if you want to spare the api call.
|
@amulet1 I would suggest to send follow up patches to address your comments. If you are happy with the results I send a new PR with clean patches. |
Signed-off-by: Peter Lieven <pl@dlhnet.de>
Signed-off-by: Peter Lieven <pl@dlhnet.de>
Signed-off-by: Peter Lieven <pl@dlhnet.de>
|
@timansky any love on this ? |
4 participants
this adds support protocol type iscsidirect which allows to connect to a pure store without the need for open-iscsi or multipathd.