Skip to content

CLOUDSTACK-10239 default provider if needed #2430

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.

Sign up for GitHub

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

Jump to bottom
Merged
yadvr merged 2 commits into from
Jan 26, 2018
Merged

CLOUDSTACK-10239 default provider if needed #2430

yadvr merged 2 commits into from
Jan 26, 2018

Conversation

@DaanHoogland
Copy link
Contributor

@DaanHoogland DaanHoogland commented Jan 24, 2018

No description provided.

@yadvr
Copy link
Member

yadvr commented Jan 24, 2018

@DaanHoogland does this PR solve the issue (blocker-ish) shared on users@ towards 4.11 RC1?

yadvr
yadvr reviewed Jan 24, 2018
View reviewed changes
Copy link
Member

@yadvr yadvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some remarks, if the changes to the API mandatory parameters is acceptable, then LGTM.

plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java
private String type;

@Parameter(name = ApiConstants.LDAP_DOMAIN, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
@Parameter(name = ApiConstants.LDAP_DOMAIN, type = CommandType.STRING, required = false, description = "name of the group or OU in LDAP")
Copy link
Member

@yadvr yadvr Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this a regression that was added due to LDAP related enhancements, or could this cause issue if the field is made non-mandatory?

Copy link
Contributor Author

@DaanHoogland DaanHoogland Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the validation of the field is done deeper in the stack. A check is done to see if either ldapdomain or name is filled.

plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LinkDomainToLdapCmd.java

@Deprecated
@Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, description = "name of the group or OU in LDAP")
@Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = false, description = "name of the group or OU in LDAP")
Copy link
Member

@yadvr yadvr Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment ^^

Copy link
Contributor Author

@DaanHoogland DaanHoogland Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapContextFactory.java
final String factory = _ldapConfiguration.getFactory();
final String url = providerUrl == null ? _ldapConfiguration.getProviderUrl(domainId) : providerUrl;
String url = providerUrl == null ? _ldapConfiguration.getProviderUrl(domainId) : providerUrl;
if (StringUtils.isEmpty(url) && domainId != null) {
Copy link
Member

@yadvr yadvr Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do a Strings.isNullOrEmpty() instead?

Copy link
Contributor Author

@DaanHoogland DaanHoogland Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can but getProviderUrl(domainId) returns "" at least

@DaanHoogland
Copy link
Contributor Author

DaanHoogland commented Jan 24, 2018 •
edited
Loading

@rhtyd it does,
@marcaurele I think this is in your toko, not? just did some googling, guess it is not.
I am not sure of my solution, it fixes backwards compatibility but I think the clean solution it to specify for which domain an ldap server is meant (as it can be specified for several domain anyway)

@DaanHoogland DaanHoogland modified the milestones: 4.12, 4.11.1 Jan 25, 2018
@DaanHoogland
Copy link
Contributor Author

DaanHoogland commented Jan 25, 2018

as it is a backwards compatibility issue, i am marking this as a blocker. happy to discuss though.

@yadvr yadvr modified the milestones: 4.11.1, 4.11.0 Jan 25, 2018
@yadvr
Copy link
Member

yadvr commented Jan 25, 2018

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Jan 25, 2018

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

blueorangutan commented Jan 25, 2018

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1681

@yadvr
Copy link
Member

yadvr commented Jan 25, 2018

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Jan 25, 2018

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

blueorangutan commented Jan 25, 2018

Trillian test result (tid-2193)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36815 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2430-t2193-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_public_ip_range.py
Intermitten failure detected: /marvin/tests/smoke/test_templates.py
Intermitten failure detected: /marvin/tests/smoke/test_usage.py
Intermitten failure detected: /marvin/tests/smoke/test_volumes.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 62 look OK, 5 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_02_vpc_privategw_static_routes Failure 153.87 test_privategw_acl.py
test_04_extract_template Failure 128.44 test_templates.py
ContextSuite context=TestISOUsage>:setup Error 0.00 test_usage.py
test_06_download_detached_volume Failure 137.83 test_volumes.py
test_hostha_enable_ha_when_host_in_maintenance Error 2.90 test_hostha_kvm.py

borisstoyanov
borisstoyanov approved these changes Jan 25, 2018
View reviewed changes
Copy link
Contributor

@borisstoyanov borisstoyanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen these same failures in the recent PRs and rerun them with a marvin box and they passed, tests LGTM.

@yadvr
Copy link
Member

yadvr commented Jan 26, 2018

Test LGTM, most of the the failures were caused by a fake public IP range added by test_public_ip_range.py that failed download urls for iso/volume/template.

@yadvr yadvr merged commit 6aadbc5 into apache:4.11 Jan 26, 2018
yadvr added a commit to shapeblue/cloudstack that referenced this pull request Jan 26, 2018
@yadvr
Merge branch '4.11'
d6ce39b 
- CLOUDSTACK-10239: Fallback to default provider if needed (apache#2430)
- CLOUDSTACK-10255: Fix agent logrotate (apache#2429)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes

@borisstoyanov borisstoyanov borisstoyanov approved these changes

@marcaurele marcaurele Awaiting requested review from marcaurele

@karuturi karuturi Awaiting requested review from karuturi

Assignees

No one assigned

Labels

Severity:BLOCKER

Projects

None yet

Milestone

4.11.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@DaanHoogland @yadvr @blueorangutan @borisstoyanov