Skip to content

server: throw exception if fail to cleanup IP resources when release a public IP #9059

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

Closed
weizhouapache wants to merge 2 commits into from
Closed

server: throw exception if fail to cleanup IP resources when release a public IP #9059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f0b26b
server: throw exception if fail to cleanup IP resources when release …
weizhouapache May 8, 2024
8d0aab5
Update 9059: rename markAsUnavailable to markAsReleasing
weizhouapache Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion engine/schema/src/main/java/com/cloud/network/dao/IPAddressDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@

public interface IPAddressDao extends GenericDao<IPAddressVO, Long> {

IPAddressVO markAsUnavailable(long ipAddressId);
IPAddressVO markAsReleasing(long ipAddressId);

void unassignIpAddress(long ipAddressId);

Expand Down
2 changes: 1 addition & 1 deletion engine/schema/src/main/java/com/cloud/network/dao/IPAddressDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ public int countIPs(long dcId, Long accountId, String vlanId, String vlanGateway

@Override
@DB
public IPAddressVO markAsUnavailable(long ipAddressId) {
public IPAddressVO markAsReleasing(long ipAddressId) {
SearchCriteria<IPAddressVO> sc = AllFieldsSearch.create();
sc.setParameters("id", ipAddressId);

Expand Down
9 changes: 5 additions & 4 deletions server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,8 +732,9 @@ public boolean disassociatePublicIpAddress(long addrId, long userId, Account cal
PublicIpQuarantine publicIpQuarantine = null;
// Cleanup all ip address resources - PF/LB/Static nat rules
if (!cleanupIpResources(addrId, userId, caller)) {

@hsato03 hsato03 Jun 27, 2024

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.

Are the IP resources that are cleaned up by the cleanupIpResources method being rolled back when the exception is thrown?

@sureshanaparti sureshanaparti Jun 28, 2024

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.

@weizhouapache can you check this ^^^

success = false;
s_logger.warn("Failed to release resources for ip address id=" + addrId);
String msg = String.format("Failed to release resources for ip address id=%s", addrId);
s_logger.error(msg);
throw new CloudRuntimeException(msg);

@sureshanaparti sureshanaparti May 8, 2024 •
edited
Loading

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.

does this block any other subsequent calls, & changes behavior?

@weizhouapache weizhouapache May 8, 2024

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.

it does, but I think the process should be stopped here if the error happens.

@DaanHoogland DaanHoogland Jun 21, 2024

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 think this is very much unreadible code:

  • in a method named disassociatePublicIpAddress(..)
  • after a call to cleanupIpResources(..)
  • folows a call to markIpAsUnavailable(..)
    at the very least the last one must be renamed. I ropose markIpForReleasing(..). Looking in the code, that is what it actually does.

I agree that some action other than marking for releasing should follow a failure to clean up. Let's investigate the callers and see what can be done.

@sureshanaparti sureshanaparti Jun 24, 2024

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.

Hi @weizhouapache should these be addressed here, or later in separate PR?

@weizhouapache weizhouapache Jun 24, 2024

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 @DaanHoogland
renamed the last method from markAsUnavailable to markAsReleasing

}

IPAddressVO ip = markIpAsUnavailable(addrId);
Expand Down Expand Up @@ -1567,7 +1568,7 @@ public IPAddressVO associateIPToGuestNetwork(long ipId, long networkId, boolean
if (ip != null) {
try {
s_logger.warn("Failed to associate ip address, so releasing ip from the database " + ip);
_ipAddressDao.markAsUnavailable(ip.getId());
_ipAddressDao.markAsReleasing(ip.getId());
if (!applyIpAssociations(network, true)) {
// if fail to apply ip associations again, unassign ip address without updating resource
// count and generating usage event as there is no need to keep it in the db
Expand Down Expand Up @@ -1981,7 +1982,7 @@ public IPAddressVO doInTransaction(TransactionStatus status) {
}
}

return _ipAddressDao.markAsUnavailable(addrId);
return _ipAddressDao.markAsReleasing(addrId);
}
});
}
Expand Down