You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2020/07/20 20:51:52 UTC
[GitHub] [cloudstack] skattoju4 opened a new pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
skattoju4 opened a new pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219
## Description
Added property to agent.properties that enables or disables the iscsi session clean up feature. #4210
Added a condition to prevent disk partitions from being cleaned up. #4216
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
## Screenshots (if appropriate):
## How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/master/CONTRIBUTING.md) document -->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-661861910
@skattoju4 The commit from PR https://github.com/apache/cloudstack/pull/3819 was added on 4.13, right? This PR should then be also aimed to branch 4.13.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-673407290
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] mike-tutkowski commented on a change in pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
mike-tutkowski commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r458936995
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -38,6 +38,8 @@
private static final String ISCSI_PATH_PREFIX = "/dev/disk/by-path";
private static final String KEYWORD_ISCSI = "iscsi";
private static final String KEYWORD_IQN = "iqn";
+ private static final String KEYWORD_PART = "part";
+ private static final String REGEX_PART = "\\S+part\\d+$";
Review comment:
Perhaps we want to used KEYWORD_PART here instead of "part"?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] skattoju4 commented on a change in pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
skattoju4 commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r459690437
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -38,6 +38,8 @@
private static final String ISCSI_PATH_PREFIX = "/dev/disk/by-path";
private static final String KEYWORD_ISCSI = "iscsi";
private static final String KEYWORD_IQN = "iqn";
+ private static final String KEYWORD_PART = "part";
Review comment:
removed it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r457944776
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -114,7 +114,7 @@ private void updateDiskStatusMapWithInactiveIscsiSessions(Connect conn){
//check the volume map. If an entry exists change the status to True
for (final LibvirtVMDef.DiskDef disk : disks) {
- if (diskStatusMap.containsKey(disk.getDiskPath())) {
+ if (diskStatusMap.containsKey(disk.getDiskPath())&&!disk.getDiskPath().contains("part")) {
Review comment:
Can you please extract `"part"` to a constant?
(Maybe) Adding some details/documentation on why it is important to have the disk path checked with that constant might be a plus as well considering future reference/maintenance on the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd merged pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] mike-tutkowski commented on a change in pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
mike-tutkowski commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r458936995
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -38,6 +38,8 @@
private static final String ISCSI_PATH_PREFIX = "/dev/disk/by-path";
private static final String KEYWORD_ISCSI = "iscsi";
private static final String KEYWORD_IQN = "iqn";
+ private static final String KEYWORD_PART = "part";
+ private static final String REGEX_PART = "\\S+part\\d+$";
Review comment:
Perhaps we want to use KEYWORD_PART here instead of "part"?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-673936067
@skattoju4 it's not just changing the base branch - see the diff - you need to rebase changes against 4.13 (base branch)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] skattoju4 commented on a change in pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
skattoju4 commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r458875100
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -1156,9 +1156,15 @@ public boolean configure(final String name, final Map<String, Object> params) th
storageProcessor.configure(name, params);
storageHandler = new StorageSubsystemCommandHandlerBase(storageProcessor);
- IscsiStorageCleanupMonitor isciCleanupMonitor = new IscsiStorageCleanupMonitor();
- final Thread cleanupMonitor = new Thread(isciCleanupMonitor);
- cleanupMonitor.start();
+ boolean _iscsiCleanUpEnabled = (boolean)params.get("iscsi.session.cleanup.enabled");
+
+ if (_iscsiCleanUpEnabled) {
+ IscsiStorageCleanupMonitor isciCleanupMonitor = new IscsiStorageCleanupMonitor();
+ final Thread cleanupMonitor = new Thread(isciCleanupMonitor);
+ cleanupMonitor.start();
+ } else {
+ s_logger.info("iscsi session clean up is disabled")
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-677591981
@blueorangutan test
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-669465671
@svenvogel @skattoju4 regarding the question of which branch this one should be targeted:
Considering that the bug was introduced in 4.13 and we still support 4.13 as LTS, I think that this one should be merged on 4.13 and then merge-forwarded to 4.14, and master.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-677490693
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-677490906
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-673407691
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd removed a comment on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
rhtyd removed a comment on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-677490693
@blueorangutan package
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rhtyd commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-673408054
@skattoju4 can you cherry-pick or rebase your PR branch against 4.13?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] kiwiflyer commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
kiwiflyer commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-669480383
@GabrielBrascher yes, I agree.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher commented on a change in pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r457788378
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -1156,9 +1156,15 @@ public boolean configure(final String name, final Map<String, Object> params) th
storageProcessor.configure(name, params);
storageHandler = new StorageSubsystemCommandHandlerBase(storageProcessor);
- IscsiStorageCleanupMonitor isciCleanupMonitor = new IscsiStorageCleanupMonitor();
- final Thread cleanupMonitor = new Thread(isciCleanupMonitor);
- cleanupMonitor.start();
+ boolean _iscsiCleanUpEnabled = (boolean)params.get("iscsi.session.cleanup.enabled");
+
+ if (_iscsiCleanUpEnabled) {
Review comment:
To be honest, I am not 100% sure on this one, but I think that `(boolean)params.get("config")` does not handle null parameters (e.g. when commented or removed from configurations).
Can you please take a look and run a few tests considering those cases where the parameter is not on the configuration file?
One approach that I know it would work is `Boolean.parseBoolean`; _parseBoolean_ handles null values (`value = "null->false", pure = true`).
Example:
```
Boolean _iscsiCleanUpEnabled = Boolean.parseBoolean((String)params.get("iscsi.session.cleanup.enabled"));
if (BooleanUtils.isTrue(_iscsiCleanUpEnabled)) {
```
##########
File path: agent/conf/agent.properties
##########
@@ -258,3 +258,6 @@ hypervisor.type=kvm
# timer.
# For all actions refer to the libvirt documentation.
# Recommended values are: none, reset and poweroff.
+#
+iscsi.session.cleanup.enabled=false
Review comment:
1. Considering compatibility on cases where the file remains the old one and does not receive the upgraded `agent.properties` (system admin can choose between keeping an old file or update it during CloudStack agent upgrade), this implementation should handle `null`.
2. As far as I understood, the iSCSI session cleanup works only on managed storage (e.g. Solidfire). If that is the case indeed I think that the parameter details/documentation should add details regarding when it is recommended to use it and when it is risky. Additionally, following other parameters approach, this one (maybe) could also begin commented as it should be set to **false** / **null** on most cases.
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -114,7 +114,7 @@ private void updateDiskStatusMapWithInactiveIscsiSessions(Connect conn){
//check the volume map. If an entry exists change the status to True
for (final LibvirtVMDef.DiskDef disk : disks) {
- if (diskStatusMap.containsKey(disk.getDiskPath())) {
+ if (diskStatusMap.containsKey(disk.getDiskPath())&&!disk.getDiskPath().contains("part")) {
Review comment:
Can you please extract `"part"` to a constant?
Adding some details/documentation on why it is important to have this constant checked might be a plus as well.
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -114,7 +114,7 @@ private void updateDiskStatusMapWithInactiveIscsiSessions(Connect conn){
//check the volume map. If an entry exists change the status to True
for (final LibvirtVMDef.DiskDef disk : disks) {
- if (diskStatusMap.containsKey(disk.getDiskPath())) {
+ if (diskStatusMap.containsKey(disk.getDiskPath())&&!disk.getDiskPath().contains("part")) {
Review comment:
Can you please extract `"part"` to a constant?
(Maybe) Adding some details/documentation on why it is important to have the disk path checked with that constant might be a plus as well considering future reference/maintenance on the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] skattoju4 commented on a change in pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
skattoju4 commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r458874196
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -1156,9 +1156,15 @@ public boolean configure(final String name, final Map<String, Object> params) th
storageProcessor.configure(name, params);
storageHandler = new StorageSubsystemCommandHandlerBase(storageProcessor);
- IscsiStorageCleanupMonitor isciCleanupMonitor = new IscsiStorageCleanupMonitor();
- final Thread cleanupMonitor = new Thread(isciCleanupMonitor);
- cleanupMonitor.start();
+ boolean _iscsiCleanUpEnabled = (boolean)params.get("iscsi.session.cleanup.enabled");
+
+ if (_iscsiCleanUpEnabled) {
Review comment:
Thanks for pointing this out. I used your approach.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] skattoju4 commented on a change in pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
skattoju4 commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r458874595
##########
File path: agent/conf/agent.properties
##########
@@ -258,3 +258,6 @@ hypervisor.type=kvm
# timer.
# For all actions refer to the libvirt documentation.
# Recommended values are: none, reset and poweroff.
+#
+iscsi.session.cleanup.enabled=false
Review comment:
added a short description in the properties file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-677928989
<b>Trillian test result (tid-2520)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34346 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4219-t2520-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 75 look OK, 2 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_02_vpc_privategw_static_routes | `Failure` | 201.89 | test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup | `Failure` | 199.16 | test_privategw_acl.py
test_04_rvpc_privategw_static_routes | `Failure` | 296.18 | test_privategw_acl.py
test_01_redundant_vpc_site2site_vpn | `Failure` | 397.70 | test_vpc_vpn.py
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] skattoju4 commented on a change in pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
skattoju4 commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r458876097
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -114,7 +114,7 @@ private void updateDiskStatusMapWithInactiveIscsiSessions(Connect conn){
//check the volume map. If an entry exists change the status to True
for (final LibvirtVMDef.DiskDef disk : disks) {
- if (diskStatusMap.containsKey(disk.getDiskPath())) {
+ if (diskStatusMap.containsKey(disk.getDiskPath())&&!disk.getDiskPath().contains("part")) {
Review comment:
> @svenvogel @skattoju4 The commit from PR #3819 was added on 4.13, right? This PR should then be also aimed at branch 4.13.
would you know whats the best way to rebase it to 4.13 is ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-677501459
Packaging result: ✔centos7 ✖centos8 ✔debian. JID-1792
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-677491310
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-677593933
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] GabrielBrascher edited a comment on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
GabrielBrascher edited a comment on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-661861910
@svenvogel @skattoju4 The commit from PR https://github.com/apache/cloudstack/pull/3819 was added on 4.13, right? This PR should then be also aimed at branch 4.13.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] svenvogel commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
svenvogel commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-663678131
@skattoju4 yes we will test this and come back as fast as we can. 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] mike-tutkowski commented on a change in pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
mike-tutkowski commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r458936629
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java
##########
@@ -38,6 +38,8 @@
private static final String ISCSI_PATH_PREFIX = "/dev/disk/by-path";
private static final String KEYWORD_ISCSI = "iscsi";
private static final String KEYWORD_IQN = "iqn";
+ private static final String KEYWORD_PART = "part";
Review comment:
Is this constant used anywhere?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] RodrigoDLopez commented on a change in pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
RodrigoDLopez commented on a change in pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#discussion_r458216199
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java
##########
@@ -1156,9 +1156,15 @@ public boolean configure(final String name, final Map<String, Object> params) th
storageProcessor.configure(name, params);
storageHandler = new StorageSubsystemCommandHandlerBase(storageProcessor);
- IscsiStorageCleanupMonitor isciCleanupMonitor = new IscsiStorageCleanupMonitor();
- final Thread cleanupMonitor = new Thread(isciCleanupMonitor);
- cleanupMonitor.start();
+ boolean _iscsiCleanUpEnabled = (boolean)params.get("iscsi.session.cleanup.enabled");
+
+ if (_iscsiCleanUpEnabled) {
+ IscsiStorageCleanupMonitor isciCleanupMonitor = new IscsiStorageCleanupMonitor();
+ final Thread cleanupMonitor = new Thread(isciCleanupMonitor);
+ cleanupMonitor.start();
+ } else {
+ s_logger.info("iscsi session clean up is disabled")
Review comment:
I believe you have forgotten the ';' on here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] svenvogel commented on pull request #4219: iscsi session cleanup now configurable, filters iscsi partitions
Posted by GitBox <gi...@apache.org>.
svenvogel commented on pull request #4219:
URL: https://github.com/apache/cloudstack/pull/4219#issuecomment-662138493
@GabrielBrascher yes sure its in 4.13.1 and 4.14 and can be rebased. how is it with a forward merge after merge?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org