Skip to content

server bug fix: remove network details when network is removed#5675

Merged
weizhouapache merged 2 commits intoapache:4.16from
shapeblue:4.16-remove-network-details
Nov 25, 2021
Merged

server bug fix: remove network details when network is removed#5675
weizhouapache merged 2 commits intoapache:4.16from
shapeblue:4.16-remove-network-details

Conversation

@weizhouapache
Copy link
Copy Markdown
Member

Description

This PR removes network details when network is removed.

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)

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?

@weizhouapache weizhouapache added this to the 4.16.1.0 milestone Nov 10, 2021
@weizhouapache weizhouapache reopened this Nov 12, 2021
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

clgtm

if (networkAccount != null) {
_networkAccountDao.remove(networkAccount.getId());
}
// Remove network details
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.

I don't think this comment is needed, given the name of the method invoked

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Nov 18, 2021

@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: ✖️ el7 ✖️ el8 ✖️ debian ✔️ suse15. SL-JID 1724

@weizhouapache
Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@weizhouapache 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: ✔️ el7 ✖️ el8 ✔️ debian ✔️ suse15. SL-JID 1735

Copy link
Copy Markdown
Contributor

@GutoVeronezi GutoVeronezi left a comment

Choose a reason for hiding this comment

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

Code LTGM, I did not test it though.

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Nov 23, 2021

@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: ✖️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1770

@sureshanaparti
Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@sureshanaparti 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: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1777

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Nov 24, 2021

@blueorangutan test centos7 vmware-67u3

@blueorangutan
Copy link
Copy Markdown

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

_networkAccountDao.remove(networkAccount.getId());
}

networkDetailsDao.removeDetails(networkFinal.getId());
Copy link
Copy Markdown
Contributor

@sureshanaparti sureshanaparti Nov 24, 2021

Choose a reason for hiding this comment

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

The foreign key should serve the purpose here. But, when a network is destroyed, the removed date is set, and the record is not removed from the table (to keep/track the history at later stage when needed). The same is the case with most of the resources, where the foreign key is set with 'ON DELETE CASCADE' but unused. May be, a cleanup (enabled/disabled) for all such removed entires from the DB, have to be introduced at regular intervals (that can be set using global config).

CONSTRAINT `fk_network_details__network_id` FOREIGN KEY `fk_network_details__network_id`(`network_id`) REFERENCES `networks`(`id`) ON DELETE CASCADE

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@sureshanaparti
yes, you are absolutely right. ON DELETE CASCADE does not work as expected because cloudstack updates removed field instead deleting the record from DB.

@weizhouapache
Copy link
Copy Markdown
Member Author

travis failures are not related to this pr

Tests ha enable/disable feature at cluster and zone level ... === TestName: test_ha_configure_enabledisable_across_clusterzones | Status : EXCEPTION ===
ERROR

and

Tests that an user is unable to remove, modify tags created by admin but should access ... === TestName: test_31_user_cant_remove_update_admin_tags | Status : EXCEPTION ===
ERROR

@blueorangutan
Copy link
Copy Markdown

Trillian test result (tid-2580)
Environment: vmware-67u3 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35311 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5675-t2580-vmware-67u3.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test Result Time (s) Test File

@weizhouapache weizhouapache merged commit 965316b into apache:4.16 Nov 25, 2021
@weizhouapache
Copy link
Copy Markdown
Member Author

Merged based on 3 approvals and trillian test result.

@weizhouapache weizhouapache deleted the 4.16-remove-network-details branch December 9, 2022 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

6 participants