Skip to content

Fix issue when multiple cidrs with different sizes are assigned on a VR #2792

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 1 commit into from
Aug 7, 2018
Merged

Fix issue when multiple cidrs with different sizes are assigned on a VR #2792

yadvr merged 1 commit into from
Aug 7, 2018

Conversation

@RPDiep
Copy link
Contributor

@RPDiep RPDiep commented Aug 6, 2018

Without this patch, the VR uses the netmask of the primary network for all assigned cidrs.
This patch correctly applies the corresponding netmask.

Description

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • 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)

GitHub Issue/PRs

No issue has yet been reported. We came across this today.

Screenshots (if appropriate):

How Has This Been Tested?

I've live patched the virtual router and rebooted it. Before, /etc/dnsmasq.d/cloud.conf had wrong netmasks for several networks. After, the networks were set correctly.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
    Testing
  • I have added tests to cover my changes.
  • All relevant new and existing integration tests have passed.
  • A full integration testsuite with all test that can run on my environment has passed.

@wido wido requested review from GabrielBrascher, wido and yadvr August 6, 2018 12:24
@wido wido added this to the 4.12.0.0 milestone Aug 6, 2018
@RPDiep
Copy link
Contributor Author

RPDiep commented Aug 6, 2018

Before patch:

root@r-29920-VM:~# grep 'eth0-[0-9],1,' /etc/dnsmasq.d/cloud.conf
dhcp-option=tag:interface-eth0-0,1,255.255.255.128
dhcp-option=tag:interface-eth0-1,1,255.255.255.128
dhcp-option=tag:interface-eth0-2,1,255.255.255.128
dhcp-option=tag:interface-eth0-3,1,255.255.255.128
dhcp-option=tag:interface-eth0-4,1,255.255.255.128

After patch:

root@r-29920-VM:~# grep 'eth0-[0-9],1,' /etc/dnsmasq.d/cloud.conf
dhcp-option=tag:interface-eth0-0,1,255.255.254.0
dhcp-option=tag:interface-eth0-1,1,255.255.255.128
dhcp-option=tag:interface-eth0-2,1,255.255.255.128
dhcp-option=tag:interface-eth0-3,1,255.255.255.0
dhcp-option=tag:interface-eth0-4,1,255.255.255.0

@wido wido changed the base branch from master to 4.11 August 6, 2018 12:54
@wido wido changed the base branch from 4.11 to master August 6, 2018 12:54
@wido
Copy link
Contributor

wido commented Aug 6, 2018

@RPDiep Thanks! As we want this to go into 4.11, could you maybe rebase against 4.11 and change it in the PR? We'll forward merge it from the 4.11 branch into master so that it reaches 4.12 then as well.

@wido wido modified the milestones: 4.12.0.0, 4.11.2.0 Aug 6, 2018
Fix issue when multiple cidrs with different sizes are assigned on a VR
96af47f 
Without this patch, the VR uses the netmask of the primary network for all assigned cidrs.
This patch correctly applies the corresponding netmask.
@RPDiep RPDiep force-pushed the vr_dhcp_multiple_cidr_fix branch from 2b91c8c to 96af47f Compare August 6, 2018 13:05
@RPDiep RPDiep changed the base branch from master to 4.11 August 6, 2018 13:05
@RPDiep
Copy link
Contributor Author

RPDiep commented Aug 6, 2018

Should be good now @wido

@yadvr
Copy link
Member

yadvr commented Aug 6, 2018

@blueorangutan package

wido
wido reviewed Aug 6, 2018
View reviewed changes
Copy link
Contributor

@wido wido left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@blueorangutan
Copy link

blueorangutan commented Aug 6, 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 Aug 6, 2018

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

@yadvr
Copy link
Member

yadvr commented Aug 6, 2018

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Aug 6, 2018

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

@blueorangutan
Copy link

blueorangutan commented Aug 6, 2018

Trillian test result (tid-2912)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 23062 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2792-t2912-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 64 look OK, 3 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup Error 0.00 test_deploy_virtio_scsi_vm.py
test_03_vpc_privategw_restart_vpc_cleanup Failure 1136.12 test_privategw_acl.py
test_02_unsecure_vm_migration Error 65.39 test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration Error 39.22 test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration Error 172.93 test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration Error 133.70 test_vm_life_cycle.py
test_08_migrate_vm Error 17.68 test_vm_life_cycle.py

borisstoyanov
borisstoyanov approved these changes Aug 7, 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.

LGTM, the failures does not seem related

@yadvr yadvr merged commit 33a6ea0 into apache:4.11 Aug 7, 2018
yadvr
yadvr reviewed Aug 7, 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.

LGTM

wido pushed a commit to CLDIN/cloudstack that referenced this pull request Aug 15, 2018
@wido
router: Use network based netmask for dnsmasq (apache#2792)
c7879ad 
Without this patch, the VR uses the netmask of the primary network for all assigned cidrs.
This patch correctly applies the corresponding netmask.

(cherry picked from commit 33a6ea0)
wido pushed a commit to CLDIN/cloudstack that referenced this pull request Aug 29, 2018
@wido
router: Use network based netmask for dnsmasq (apache#2792)
2ac29c8 
Without this patch, the VR uses the netmask of the primary network for all assigned cidrs.
This patch correctly applies the corresponding netmask.

(cherry picked from commit 33a6ea0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr left review comments

@wido wido wido approved these changes

@rafaelweingartner rafaelweingartner rafaelweingartner approved these changes

@GabrielBrascher GabrielBrascher GabrielBrascher approved these changes

@borisstoyanov borisstoyanov borisstoyanov approved these changes

Assignees

No one assigned

Labels

component:virtual-router type:bug

Projects

None yet

Milestone

4.11.2.0

Development

Successfully merging this pull request may close these issues.

7 participants

@RPDiep @wido @yadvr @blueorangutan @rafaelweingartner @GabrielBrascher @borisstoyanov