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 2021/01/04 08:43:05 UTC
[GitHub] [cloudstack] div8cn opened a new pull request #4565: Fix RBD primary storage host port null error
div8cn opened a new pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565
### Description
Ubuntu 20.04 +KVM +CEPH 15.2
Add RBD main storage through UI, it will fail when there is no host port parameter;
Because when we created the pool, we did not add the port target in the xml
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- 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: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ********************************************************************************* -->
### Types of changes
- [ ] 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)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### 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] rhtyd commented on pull request #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#issuecomment-754656103
@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] rhtyd commented on pull request #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#issuecomment-754563022
@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] wido commented on a change in pull request #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
wido commented on a change in pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#discussion_r551206332
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java
##########
@@ -84,7 +84,11 @@ public boolean parseDomainXML(String domXML) {
String authUserName = getAttrValue("auth", "username", disk);
String poolUuid = getAttrValue("secret", "uuid", disk);
String host = getAttrValue("host", "name", disk);
- int port = Integer.parseInt(getAttrValue("host", "port", disk));
+ int port = 0;
+ String _xmlPort = getAttrValue("host", "port", disk);
+ if ( ! _xmlPort.isEmpty()) {
Review comment:
I would do that just for the sake of security
----------------------------------------------------------------
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 #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#issuecomment-754461078
@div8cn with you comment https://github.com/apache/cloudstack/pull/4568#issuecomment-754321932 do we need this PR?
----------------------------------------------------------------
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] div8cn commented on pull request #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
div8cn commented on pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#issuecomment-754559694
@rhtyd I think this PR is necessary,
The best practice is to let libvirt decide to use port 3300 or 6789 to communicate with CEPH.
Therefore, it is best not to enter any port when adding mon;
@wido What do you think?
----------------------------------------------------------------
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] wido commented on pull request #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
wido commented on pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#issuecomment-754560992
I agree @div8cn , this PR is needed.
In the early days of Ceph you needed to specify the port. Now we should have libvirt/qemu/librados/librbd handle this. We shouldn't interfere.
People running Ceph monitors on alternative ports are rare, very rare. I have never encountered them recently.
So yes, just a hostname should be sufficient without a port number.
----------------------------------------------------------------
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 #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#issuecomment-754642319
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2525
----------------------------------------------------------------
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 #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#issuecomment-754954890
<b>Trillian test result (tid-3372)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30348 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4565-t3372-kvm-centos7.zip
Smoke tests completed. 86 look OK, 0 have error(s)
Only failed tests results shown below:
Test | Result | Time (s) | Test 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] rhtyd closed pull request #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
rhtyd closed pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565
----------------------------------------------------------------
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 #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#issuecomment-754563031
@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 #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#issuecomment-754656702
@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 commented on pull request #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#issuecomment-755441656
@rhtyd it would be great to have this one on board, but I don't see reason in blocking a release because of it.
Any comments, @wido @DaanHoogland @div8cn @weizhouapache?
----------------------------------------------------------------
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 #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
GabrielBrascher commented on a change in pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#discussion_r552692404
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java
##########
@@ -84,7 +84,11 @@ public boolean parseDomainXML(String domXML) {
String authUserName = getAttrValue("auth", "username", disk);
String poolUuid = getAttrValue("secret", "uuid", disk);
String host = getAttrValue("host", "name", disk);
- int port = Integer.parseInt(getAttrValue("host", "port", disk));
+ int port = 0;
+ String _xmlPort = getAttrValue("host", "port", disk);
+ if ( ! _xmlPort.isEmpty()) {
Review comment:
1. @div8cn can you please change the variable name from `_xmlPort` to `xmlPort`?
According to [Java conventions](https://docs.oracle.com/javase/tutorial/java/nutsandbolts/variables.html),
> while it's technically legal to begin your variable's name with "_", this practice is discouraged
2. Regarding the String validation. Is it possible to have a `tagNode` of length 0?
`getAttrValue()` has this piece of code:
```
NodeList tagNode = eElement.getElementsByTagName(tag);
if (tagNode.getLength() == 0) {
return null;
}
```
I would recommend, just for safety, to use something as `org.apache.commons.lang.StringUtils`; it provides String validations such as `StringUtils.isNotBlank(Str)`.
`isNotBlank` returns true only if not null, not empty, and not blank.
```
String xmlPort = getAttrValue("host", "port", disk);
if (StringUtils.isNotBlank(xmlPort)) {
....
}
```
----------------------------------------------------------------
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 #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#issuecomment-754562606
@wido @DaanHoogland @GabrielBrascher @weizhouapache should this be treated as a blocker then, or good to have?
@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 a change in pull request #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
rhtyd commented on a change in pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#discussion_r551183874
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java
##########
@@ -84,7 +84,11 @@ public boolean parseDomainXML(String domXML) {
String authUserName = getAttrValue("auth", "username", disk);
String poolUuid = getAttrValue("secret", "uuid", disk);
String host = getAttrValue("host", "name", disk);
- int port = Integer.parseInt(getAttrValue("host", "port", disk));
+ int port = 0;
+ String _xmlPort = getAttrValue("host", "port", disk);
+ if ( ! _xmlPort.isEmpty()) {
Review comment:
Should we do both null and empty check?
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolXMLParser.java
##########
@@ -55,7 +55,11 @@ public LibvirtStoragePoolDef parseStoragePoolXML(String poolXML) {
String format = getAttrValue("format", "type", source);
if (type.equalsIgnoreCase("rbd")) {
- int port = Integer.parseInt(getAttrValue("host", "port", source));
+ int port = 0;
+ String _xmlPort = getAttrValue("host", "port", source);
+ if ( ! _xmlPort.isEmpty()) {
Review comment:
Should we do both null and empty check?
----------------------------------------------------------------
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 #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
rhtyd merged pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565
----------------------------------------------------------------
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] DaanHoogland commented on a change in pull request #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#discussion_r551792810
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java
##########
@@ -84,7 +84,11 @@ public boolean parseDomainXML(String domXML) {
String authUserName = getAttrValue("auth", "username", disk);
String poolUuid = getAttrValue("secret", "uuid", disk);
String host = getAttrValue("host", "name", disk);
- int port = Integer.parseInt(getAttrValue("host", "port", disk));
+ int port = 0;
+ String _xmlPort = getAttrValue("host", "port", disk);
+ if ( ! _xmlPort.isEmpty()) {
Review comment:
I did an investigation of the dom code used in `getAttrValue()` and it returns an empty string if the element `"host"` does not have an attribute named `"port"`, so _xmlPort will not be null.
----------------------------------------------------------------
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] DaanHoogland commented on a change in pull request #4565: Fix RBD primary storage host port null error
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on a change in pull request #4565:
URL: https://github.com/apache/cloudstack/pull/4565#discussion_r551327251
##########
File path: plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtDomainXMLParser.java
##########
@@ -84,7 +84,11 @@ public boolean parseDomainXML(String domXML) {
String authUserName = getAttrValue("auth", "username", disk);
String poolUuid = getAttrValue("secret", "uuid", disk);
String host = getAttrValue("host", "name", disk);
- int port = Integer.parseInt(getAttrValue("host", "port", disk));
+ int port = 0;
+ String _xmlPort = getAttrValue("host", "port", disk);
+ if ( ! _xmlPort.isEmpty()) {
Review comment:
there is a lot of npe danger in `getAttrValue()`, none of teh parameters are checked. (meaning yes please)
----------------------------------------------------------------
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