-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change iSCSI storage connection validation to properly avoid duplications #551
Change iSCSI storage connection validation to properly avoid duplications #551
Conversation
AND (connection = v_connection) | ||
AND ( | ||
port = v_port | ||
OR ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If port
and v_port
are NULL
, wouldn't the first part port = v_port
catch it?
Is this OR
required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I actually copied it from the previously used query, but I can't quite understand what is the purpose of this check. In our case these values should not be null anyway.
connection.getPort(), | ||
connection.getIqn() | ||
); | ||
if (connections != null && !connections.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO can be simplified:
if (CollectionUtils.isEmpty(connections)) {
return null;
}
return connections.get(0);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can be simplified even further - the result should never be null
@@ -80,7 +80,7 @@ protected String isConnWithSameDetailsExists(StorageServerConnections connection | |||
String connectionField = connection.getConnection(); | |||
connections = storageServerConnectionDao.getAllForStorage(connectionField); | |||
} else { | |||
StorageServerConnections sameConnection = iscsiStorageHelper.findConnectionWithSameDetails(connection); | |||
StorageServerConnections sameConnection = iscsiStorageHelper.findConnectionByAddressPortAndIqn(connection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason not to change findConnectionWithSameDetails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used elsewhere, there's even a comment that warns you from changing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought: maybe better if findConnectionByAddressPortAndIqn()
to simply return the list of all connections as received from storageServerConnectionDao.getStorageConnectionsByConnectionPortAndIqn
(that can be or not be an empty list)?
That will simplify the findConnectionByAddressPortAndIqn()
method (that just needs to be renamed to plural "Connections"), will simplify this method (just put the result into connections
local variable). No need for all those List
--> single element --> back to List
redundant code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Done
connection.getPort(), | ||
connection.getIqn() | ||
); | ||
if (connections != null && !connections.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can be simplified even further - the result should never be null
41f92d2
to
e903e2b
Compare
e903e2b
to
c4154fa
Compare
/ost |
...in/java/org/ovirt/engine/core/bll/storage/connection/StorageServerConnectionCommandBase.java
Show resolved
Hide resolved
@@ -207,6 +207,14 @@ public StorageServerConnections findConnectionWithSameDetails(StorageServerConne | |||
return null; | |||
} | |||
|
|||
public List<StorageServerConnections> findConnectionByAddressPortAndIqn(StorageServerConnections connection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename to findConnectionsByAddressPortAndIqn
?
Add "s" for plural...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
...ager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageServerConnectionDaoImpl.java
Show resolved
Hide resolved
Currently when adding/updating iSCSI storage server connection, there is a validation that checks if duplicate connection exists by iqn, address, port, portal, username and password. This allows creating two storage connections with the same iqn, address and port, but with different portals. There cannot be two portals with the same iqn-address-port, thus creating two such connections does not make sense and will lead to errors connecting the host to storage. This change fixes the validation to fail add/update request if connection with the same iqn-address-port combination exists Bug-Url: https://bugzilla.redhat.com/2079903
c4154fa
to
cfbf20d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
Currently when adding/updating iSCSI storage server connection, there
is a validation that checks if a duplicate connection exists by iqn,
address, port, portal, username and password. This allows creating
two storage connections with the same iqn, address and port, but with
different portals.
There cannot be two portals with the same iqn-address-port, thus
creating two such connections does not make sense and will lead to
errors connecting the host to storage.
This change fixes the validation to fail add/update request if
connection with the same iqn-address-port combination exists
Bug-Url: https://bugzilla.redhat.com/2079903