-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Conversation
DaanHoogland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm, needs testing of usage results though
|
@rajujith can you have a look? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #10721 +/- ##
============================================
+ Coverage 15.17% 15.90% +0.72%
- Complexity 11332 11336 +4
============================================
Files 5415 5046 -369
Lines 474893 445220 -29673
Branches 57920 52711 -5209
============================================
- Hits 72046 70794 -1252
+ Misses 394792 366514 -28278
+ Partials 8055 7912 -143
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| 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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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.
|
right now I do not know what the @sudo87 |
|
did some testing (add/remove new nic, change default nic, etc), found some issues. I agree it is better to keep as it is now. |

Description
This PR fixes #10697 where usage_id is always 1. With this change, network_offering_id will get added in usage_id.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?