Skip to content

server: Cleanup dhcp and dns entries only on expunging VM#3608

Merged
yadvr merged 1 commit into
apache:4.13from
shapeblue:vr-cleanup-vm-expunge
Sep 26, 2019
Merged

server: Cleanup dhcp and dns entries only on expunging VM#3608
yadvr merged 1 commit into
apache:4.13from
shapeblue:vr-cleanup-vm-expunge

Conversation

@yadvr

@yadvr yadvr commented Sep 25, 2019

Copy link
Copy Markdown
Member

This fixes a behaviour to not cleanup DHCP and DNS rules for NICs of a
VM in the VR when it is stopped, but instead when VM is expunged because
stopped VMs in CloudStack still retain the IPs and records.

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)

This fixes a behaviour to not cleanup DHCP and DNS rules for NICs of a
VM in the VR when it is stopped, but instead when VM is expunged because
stopped VMs in CloudStack still retain the IPs and records.

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
@yadvr

yadvr commented Sep 25, 2019

Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

@yadvr

yadvr commented Sep 25, 2019

Copy link
Copy Markdown
Member Author

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

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

@anuragaw anuragaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM from code.

@shwstppr shwstppr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM on changes!
Although dhcp cleanup on VM stop seems more logical to me.

@yadvr yadvr requested review from nvazquez and removed request for nvazquez September 26, 2019 11:33

@nvazquez nvazquez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, this makes cleanup introduced in this PR: #3351 to be performed only on VM expunge

@wido wido left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM based on code

@yadvr yadvr merged commit b853ebd into apache:4.13 Sep 26, 2019
@yadvr

yadvr commented Oct 2, 2019

Copy link
Copy Markdown
Member Author

FYI - this partially reverts and refactors changes introduced in #3351 to cleanup VM dhcp/dns rules in the VR only when a VM is expunged. A VM hostname and IP address is used as constraint and checks even of a VM is stopped, i.e. you cannot deploy a VM with the same hostname and IP in a network with an existing VM with those attributes. This was tested against a local monkeybox KVM env and regression tested agains Trillian/kvm.

ustcweizhou added a commit to ustcweizhou/cloudstack that referenced this pull request Oct 8, 2019
According comment in PR apache#3608, dhcp and dns entries are cleaned up only when a VM is expunged.
Revert part of commit 8fb388e.
nvazquez added a commit to shapeblue/cloudstack that referenced this pull request Oct 10, 2019
yadvr pushed a commit that referenced this pull request Oct 17, 2019
* server: Do NOT cleanup dhcp and dns when stop a vm

According comment in PR #3608, dhcp and dns entries are cleaned up only when a VM is expunged.
Revert part of commit 8fb388e.

* server: cleanup dns/dhcp entries in removeNic instead of finalizeExpunge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants