Skip to content

CLOUDSTACK-9595: Fix regression introduced in #1762 - #2370

Merged
yadvr merged 1 commit into
apache:masterfrom
shapeblue:vpcvpn_regression
Dec 20, 2017
Merged

CLOUDSTACK-9595: Fix regression introduced in #1762#2370
yadvr merged 1 commit into
apache:masterfrom
shapeblue:vpcvpn_regression

Conversation

@yadvr

@yadvryadvr commented Dec 20, 2017

Copy link
Copy Markdown
Member

The assignDedicateIpAddress previously had marked the newly fetched
IP as allocated but now it does not do that. This fails for VPCs
where SNATs IP are retained as allocating and not allocated after
creation.

@yvsubhash@rafaelweingartner and please review.
@blueorangutan package

@yadvr
CLOUDSTACK-9595: Fix regression introduced inapache#1762
dcef015
The `assignDedicateIpAddress` previously had marked the newly fetched
IP as allocated but now it does not do that. This fails for VPCs
where SNATs IP are retained as allocating and not allocated after
creation.
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
@blueorangutan

blueorangutan commented Dec 20, 2017

Copy link
Copy Markdown

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

rafaelweingartner
rafaelweingartner approved these changes Dec 20, 2017
View reviewed changes

@rafaelweingartnerrafaelweingartner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @rhtyd

Comment threadserver/src/com/cloud/network/IpAddressManagerImpl.java

return fetchNewPublicIp(dcId, null, null, owner, VlanType.VirtualNetwork, guestNtwkId, isSourceNat, false, null, false, vpcId, displayIp);

ip = fetchNewPublicIp(dcId, null, null, owner, VlanType.VirtualNetwork, guestNtwkId, isSourceNat, true, null, false, vpcId, displayIp);

@rafaelweingartnerrafaelweingartnerDec 20, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can return directly the call to fetchNewPublicIp.

I have not checked. What is that boolean flag you changed there?

@yadvryadvrDec 20, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@rafaelweingartner actually there is a ip variable that is used in finally which is why I assign and then return see the code below this line.

@rafaelweingartnerrafaelweingartnerDec 20, 2017•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok, now I saw it.
Thanks

@blueorangutan

blueorangutan commented Dec 20, 2017

Copy link
Copy Markdown

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

@yadvryadvr added this to the 4.11 milestone Dec 20, 2017
@yadvr

yadvr commented Dec 20, 2017

Copy link
Copy Markdown
MemberAuthor

@blueorangutan test

@blueorangutan

blueorangutan commented Dec 20, 2017

Copy link
Copy Markdown

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

@yvsubhash

yvsubhash commented Dec 20, 2017

Copy link
Copy Markdown

@rhtyd Code changes are fine LGTM for code

@yadvr

yadvr commented Dec 20, 2017

Copy link
Copy Markdown
MemberAuthor

Thanks for confirming @yvsubhash, I'll merge the fix once test results arrive.

@blueorangutan

blueorangutan commented Dec 20, 2017

Copy link
Copy Markdown

Trillian test result (tid-1839)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 28609 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2370-t1839-kvm-centos7.zip
Smoke tests completed. 63 look OK, 4 have error(s)
Only failed tests results shown below:

TestResultTime (s)Test File
test_01_vpc_privategw_aclFailure41.24test_privategw_acl.py
test_02_vpc_privategw_static_routesFailure137.19test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanupFailure116.94test_privategw_acl.py
test_04_rvpc_privategw_static_routesFailure222.45test_privategw_acl.py
test_02_create_template_with_checksum_sha1Error5.16test_templates.py
test_03_create_template_with_checksum_sha256Error5.15test_templates.py
test_04_create_template_with_checksum_md5Error5.15test_templates.py
test_01_volume_usageFailure856.60test_usage.py
test_01_vpc_remote_access_vpnFailure65.69test_vpc_vpn.py

@yadvr

yadvr commented Dec 20, 2017

Copy link
Copy Markdown
MemberAuthor

Tests LGTM, the new vpc vpn failure is due to create vpn user failing. I'll merge this based on tests and two code reviews.

@yadvr
yadvr merged commit 0e3ddb2 into apache:masterDec 20, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rafaelweingartnerrafaelweingartnerrafaelweingartner approved these changes
@resmoresmoAwaiting requested review from resmo
@widowidoAwaiting requested review from wido
@marcaurelemarcaureleAwaiting requested review from marcaurele
@DaanHooglandDaanHooglandAwaiting requested review from DaanHoogland
@jayapalujayapaluAwaiting requested review from jayapalu
@GabrielBrascherGabrielBrascherAwaiting requested review from GabrielBrascher
@nvazqueznvazquezAwaiting requested review from nvazquez

Assignees

No one assigned

Labels

Severity:BLOCKERtype:bug

Projects

None yet

Milestone

4.11.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@yadvr@blueorangutan@yvsubhash@rafaelweingartner