Skip to content

Save network offering_id in usage_id #10721

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
sudo87 wants to merge 1 commit into from
Closed

Save network offering_id in usage_id #10721

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 3 additions & 5 deletions usage/src/main/java/com/cloud/usage/parser/NetworkOfferingUsageParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@
// Only create a usage record if we have a runningTime of bigger than zero.
if (useTime > 0L) {
NOInfo info = noMap.get(noIdKey);
createUsageRecord(UsageTypes.NETWORK_OFFERING, useTime, startDate, endDate, account, info.getVmId(), info.getNOId(), info.getZoneId(), info.isDefault());
createUsageRecord(UsageTypes.NETWORK_OFFERING, useTime, startDate, endDate, account, info.getVmId(), info.getNOId(), info.getZoneId());

Check warning on line 119 in usage/src/main/java/com/cloud/usage/parser/NetworkOfferingUsageParser.java

View check run for this annotation

Codecov / codecov/patch

usage/src/main/java/com/cloud/usage/parser/NetworkOfferingUsageParser.java#L119 

Added line #L119 was not covered by tests
}
}

Expand All @@ -135,8 +135,7 @@
usageDataMap.put(key, noUsageInfo);
}

private static void createUsageRecord(int type, long runningTime, Date startDate, Date endDate, AccountVO account, long vmId, long noId, long zoneId,
boolean isDefault) {
private static void createUsageRecord(int type, long runningTime, Date startDate, Date endDate, AccountVO account, long vmId, long noId, long zoneId) {

Check warning on line 138 in usage/src/main/java/com/cloud/usage/parser/NetworkOfferingUsageParser.java

View check run for this annotation

Codecov / codecov/patch

usage/src/main/java/com/cloud/usage/parser/NetworkOfferingUsageParser.java#L138 

Added line #L138 was not covered by tests
// Our smallest increment is hourly for now
if (s_logger.isDebugEnabled()) {
s_logger.debug("Total running time " + runningTime + "ms");
Expand All @@ -155,9 +154,8 @@
// Create the usage record
String usageDesc = "Network offering:" + noId + " for Vm : " + vmId + " usage time";

long defaultNic = (isDefault) ? 1 : 0;
UsageVO usageRecord =
new UsageVO(zoneId, account.getId(), account.getDomainId(), usageDesc, usageDisplay + " Hrs", type, new Double(usage), vmId, null, noId, null, defaultNic,
new UsageVO(zoneId, account.getId(), account.getDomainId(), usageDesc, usageDisplay + " Hrs", type, new Double(usage), vmId, null, noId, null, noId,

Check warning on line 158 in usage/src/main/java/com/cloud/usage/parser/NetworkOfferingUsageParser.java

View check run for this annotation

Codecov / codecov/patch

usage/src/main/java/com/cloud/usage/parser/NetworkOfferingUsageParser.java#L158 

Added line #L158 was not covered by tests
Copy link
Member

@winterhazel winterhazel Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This change does not make sense for me. Why are we including the network offering's ID in usage_id when the ID is already in offering_id? I think we should leave it as it is.

I know that the naming does not make much sense, but this field represents whether the NIC associated with the helper entry is a VM's default one. With this change, we do not have that information anymore, possibly breaking integrations.

Copy link
Contributor

@DaanHoogland DaanHoogland Apr 16, 2025

Choose a reason for hiding this comment

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

@winterhazel the usage_id is meant to be the id of the resource being used, isn’t it? Why would this be different for usage_type 13 than it is for other usage_types?

Copy link
Member

@winterhazel winterhazel Apr 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@DaanHoogland not having the resource's ID here is undoubtedly inconsistent with other usage types. Regardless, I expect that there has been at least a reason/use case that prompted the introduction of this value (defaultNic) into the usage record, although definitely not in such a good way. Removing this from the record would break that, and possibly other integrations that were developed over this odd organization.

Therefore, considering that the network offering is already available in the usage record, that these are (presumably) working currently, and that the change can break them, I do not think that it would be beneficial to change it for the sake of consistency.

Maybe deprecating this usage type and working on an improved version (e.g. a NIC type) would be a better direction? See #10697 (comment). This usage type's behavior is weird considering its name, and I frequently see people not understanding how it works, performing rating over it, and ending up with unexpected values.

Copy link
Contributor

@DaanHoogland DaanHoogland Apr 17, 2025

Choose a reason for hiding this comment

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

clear,
let’ s indeed think about deprecating it than.
@sudo87 , do you understand @winterhazel ‘ s argument? (and sorry about your PR ;)

Copy link
Collaborator Author

@sudo87 sudo87 Apr 17, 2025

Choose a reason for hiding this comment

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

Yeah Daan, thanks @winterhazel. We can close this pr and probably resolve the linked bug as well.

null, startDate, endDate);
s_usageDao.persist(usageRecord);
}
Expand Down