-
Notifications
You must be signed in to change notification settings - Fork 244
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
[batch] introduce Support-Logs-Specs-and-Firewall fee #13542
Conversation
@jigold not actually WIP, but I want control over when it merges. Ready for review. |
@@ -151,6 +153,7 @@ def from_dict(data: dict) -> 'GCPSlimInstanceConfig': | |||
GCPDynamicSizedDiskResource('disk/pd-ssd/1'), | |||
GCPIPFeeResource('service-fee/1'), | |||
GCPServiceFeeResource('ip-fee/1024/1'), | |||
GCPSupportLogsSpecsAndFirewallFees('gcp-support-logs-specs-and-firewall-fees/1'), |
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.
I don't think you need this as it's for backwards compatibility. See comment in #13539.
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.
Can you elaborate on what kind of backwards compatibility? Should we remove this branch entirely?
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.
I think we can. It was for when the instance_config
field was not in the database for old entries, so their value is NULL. We knew what the config should be at the time though. The only reason we'd need this code path is if we wanted to compute resources for old instances. I don't know why we would want to do that.
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.
4b19eba
to
fe59a49
Compare
Fixes #13784 Here's the GCP documentation: https://cloud.google.com/vpc/pricing-announce-external-ips We were previously billing the same IP-Fee for both spot and regular instances. I changed it so we're billing for each instance type accordingly. Following #13542, I hard coded the new resource rates.
Fixes #13526
I verified I got the right number. I didn't import since I think we decided we shouldn't rely on the code base in the migrations.