CSTACKEX-46: Create, Delete iSCSI type Cloudstack volumes, Enter, Can… #31
Conversation
…cel Maintenance mode and changes added in the community PR
sandeeplocharla
requested review from
piyush5netapp,
rajiv-jain-netapp and
suryag1201
|
|
||
| private void waitForDiskToBecomeAvailable(String volumeUuid, KVMStoragePool pool) { | ||
| int numberOfTries = 10; | ||
| int numberOfTries = 30; |
There was a problem hiding this comment.
we have to highlight this change since this is common code and going to impact all the vendors. Can we stay with numberOfTried as 10?
There was a problem hiding this comment.
10 is not at all working for us, I don't think its a option. This code was written long back and it needs to be updated. I've sent a mail to the community that I've made some changes in this file. I haven't made much changes which could probably fix this issue in a better way, as this is a common code for across vendors. We need to anticipate some discussion and suggestions around this file, when we raise this code upstream.
| if (response != null && response.getRecords() != null && !response.getRecords().isEmpty()) { | ||
| // For simplicity, return the first interface's name | ||
| IpInterface ipInterface = response.getRecords().get(0); | ||
| IpInterface ipInterface = null; |
There was a problem hiding this comment.
@piyush5netapp this is what I was looking for. lets keep a todo here to enable load balancing of the LIFs consumption instead of going with the first one always. We need to take this up in subsequent PR for NFS workflows
| throw new CloudRuntimeException("getAccessGroup: Invalid accessGroup object - accessGroup is null"); | ||
| } | ||
| // @Override | ||
| // public AccessGroup getAccessGroup(AccessGroup accessGroup) { |
There was a problem hiding this comment.
any reason to comment this code ?
There was a problem hiding this comment.
This method hasn't been implemented according to the definition in StorageStrategy. Further, as this wasn't being used currently, I've commented the code, so that it could be properly fixed by reusing the logic that we already wrote.
| public class Constants { | ||
|
|
||
| public static final String ONTAP_PLUGIN_NAME = "ONTAP"; | ||
| public static final int NFS3_PORT = 2049; |
There was a problem hiding this comment.
Lets put the source of this information to keep these ports hardcoded
There was a problem hiding this comment.
They are standard ports, which I referenced from other vendor implementation and this confluence https://cwiki.apache.org/confluence/display/CLOUDSTACK/Ports+used+by+CloudStack
Shall I refer this confluence in the code?
There was a problem hiding this comment.
can you add coverage you saw with the below addd unit test code?
There was a problem hiding this comment.
Updated the current code coverage status as screenshots in this PR's description.
I've included the coverage of all the classes, not just the one's that are part of this PR, for reference.
2 participants
CSTACKEX-46: Create, Delete iSCSI type Cloudstack volumes, Enter, Cancel Maintenance mode
Description
This PR has the following changes:
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Passcode: n=AE=.C9
Passcode: $%j3+K4B
Observations:
Though we have increased retries to wait for discovery of luns, the additional volume attachment still times out, so, it needs a manual retry.
When placed into Maintenance mode, Instance is getting Stopped in the first case while it remains in Running state in the second case. We currently don't have any code attributing to this behaviour in our plugin code. Need to understand why cloudstack is behaving this way.
I've also tested force delete of storage pool when CS volumes were present, but, haven't covered it in these recordings.
Find the Code coverage reports of all the classes in plugin:
