Skip to content
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

Jump to bottom

Add initial support of GCP disks and snaphosts cost #853

Merged
merged 9 commits into from
Jun 25, 2023
Merged

Add initial support of GCP disks and snaphosts cost #853

merged 9 commits into from
Jun 25, 2023

Conversation

nemca
Copy link
Contributor

@nemca nemca commented Jun 14, 2023 •
edited
Loading

  • initial support of GCP disks cost
  • new time helper EndingOfMonth
  • linting fix version subcommand
  • refactoring instance cost calculation

@mlabouardy mlabouardy added the gcp label Jun 14, 2023
@mlabouardy mlabouardy added this to the v3.0.19 milestone Jun 14, 2023
ShubhamPalriwala
ShubhamPalriwala reviewed Jun 14, 2023
View reviewed changes
Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala left a comment

Choose a reason for hiding this comment

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

Hey @nemca, amazing work on this! And the fact that you have also added tests for the same, hats off! 🚀

I have just left some small refactoring and nits, let me know what you think of those!

I tested the pricing for a couple of my disks on GCP and it seemed accurate to me. However, I'll comment more on this once we have the Catalog API implemented (if we go ahead with it).

All in all, adding cost calculation is my favourite kind of PR reviews haha!

providers/gcp/compute/disks.go Outdated
Comment on lines 104 to 110
type calculateDiskCostData struct {
diskType string
size int64
project string
zone string
pricing *gcpcomputepricing.Pricing
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we shift this to a types_gcp_disks.go under utils/ directory for uniformity? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, i'll be do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move to utils/gcpcomputepricing/types_disk.go

providers/gcp/compute/disks.go Outdated
Comment on lines 136 to 141
case strings.Contains(strings.ToLower(*dt.Name), strings.ToLower(gcpcomputepricing.Standard)):
opts.DiskType = gcpcomputepricing.Standard
case strings.Contains(strings.ToLower(*dt.Name), strings.ToLower(gcpcomputepricing.SSD)):
opts.DiskType = gcpcomputepricing.SSD
case strings.Contains(strings.ToLower(*dt.Name), strings.ToLower(gcpcomputepricing.Balanced)):
opts.DiskType = gcpcomputepricing.Balanced
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit: we can prolly wrap the strings.Contains(strings.ToLower(str)) in a function and just call it directly leading to better code readability

This comment was marked as outdated.

Sign in to view

utils/gcpcomputepricing/machine_types.go Outdated
Comment on lines 33 to 34
DiskType string
DiskSize uint64
Copy link
Contributor

Choose a reason for hiding this comment

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

verify if this addition to the Opts struct can be used to get more accurate costs in other cases as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't understand your question very well.
But I refactored the code and removed the redundant DiskType field from this structure.

@nemca
Copy link
Contributor Author

nemca commented Jun 14, 2023

However, I'll comment more on this once we have the Catalog API implemented (if we go ahead with it).

I plan to start migrating to the Catalog API right now.
For me now the main thing is to understand that I'm on the right track. And then it is more convenient for me to refactor in small steps.

@nemca
Copy link
Contributor Author

nemca commented Jun 14, 2023

I tested the pricing for a couple of my disks on GCP and it seemed accurate to me.

Pricing for PD Standard work well ;-)

nemca
nemca commented Jun 15, 2023
View reviewed changes
providers/gcp/compute/disks.go Outdated
Comment on lines 136 to 141
case strings.Contains(strings.ToLower(*dt.Name), strings.ToLower(gcpcomputepricing.Standard)):
opts.DiskType = gcpcomputepricing.Standard
case strings.Contains(strings.ToLower(*dt.Name), strings.ToLower(gcpcomputepricing.SSD)):
opts.DiskType = gcpcomputepricing.SSD
case strings.Contains(strings.ToLower(*dt.Name), strings.ToLower(gcpcomputepricing.Balanced)):
opts.DiskType = gcpcomputepricing.Balanced

This comment was marked as outdated.

Sign in to view

@nemca nemca requested a review from ShubhamPalriwala June 15, 2023 16:12
@ShubhamPalriwala
Copy link
Contributor

Hey @nemca, great work! thanks for incorporating the feedback! I see that you also added support for GCP Snapshots as well! 👏🏼

I like the code structuring now :)) I still saw a couple of ToDos so let me know once they are wrapped up and I'll try to checkout and see your feature locally here haha! But great work! No major feedback from me rn, might come up when I test it 🤞🏼

@nemca
Copy link
Contributor Author

nemca commented Jun 15, 2023

Hey @nemca, great work! thanks for incorporating the feedback! I see that you also added support for GCP Snapshots as well! 👏🏼

I like the code structuring now :)) I still saw a couple of ToDos so let me know once they are wrapped up and I'll try to checkout and see your feature locally here haha! But great work! No major feedback from me rn, might come up when I test it 🤞🏼

I'm found a bug with calculating instances cost. Stay tuned...
And also i do migrate machine code to new structure.

@nemca nemca changed the title Add initial support of GCP disks cost Add initial support of GCP disks and snaphosts cost Jun 15, 2023
@nemca nemca requested a review from mlabouardy as a code owner June 16, 2023 12:47
@mlabouardy mlabouardy modified the milestones: v3.0.19, v3.0.20 Jun 19, 2023
mlabouardy
mlabouardy approved these changes Jun 19, 2023
View reviewed changes
Copy link
Collaborator

@mlabouardy mlabouardy left a comment

Choose a reason for hiding this comment

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

LGTM!

ShubhamPalriwala
ShubhamPalriwala reviewed Jun 21, 2023
View reviewed changes
Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala left a comment

Choose a reason for hiding this comment

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

Please see if you can handle the suggested small changes, would be helpful

providers/gcp/compute/snapshots.go Outdated
logrus.WithFields(logrus.Fields{
"provider": "GCP",
"account": client.Name,
"service": "Compute Engine",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: renaming this to Snapshots?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nemca could you make this small changes please? might help in the future in debugging?

Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch, I agree :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: renaming this to Snapshots?

Maybe Compute Engine Snapshots?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or Compute Disk Snapshot, like the value of the Service field above in the resource?

utils/gcpcomputepricing/snapshot.go

func typeSnapshotGetter(p *Pricing, opts Opts) (Subtype, error) {
var capacity Subtype
// TODO: switch by snapshot type
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still pending? or have you handled this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, still pending. I'm can't found where I can fetch snapshot types from API

@nemca nemca requested a review from ShubhamPalriwala June 21, 2023 12:22
ShubhamPalriwala
ShubhamPalriwala approved these changes Jun 24, 2023
View reviewed changes
Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala left a comment

Choose a reason for hiding this comment

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

Hey @nemca, just reviewed and tested the PR and it looks amazing!
image

Adding cost calculation to GCP Disks is a great feature you have just added since it is used by most of the Instances and commonly seen across most of the cloud projects. The PR looks great to me and thank you so much for being patient with all the small and nit queries I had. Great work! Really excited to see this go live now! 🚀
Over to youuu @mlabouardy

@mlabouardy mlabouardy merged commit 29e4b6d into tailwarden:develop Jun 25, 2023
@PumpkinSeed PumpkinSeed mentioned this pull request Jul 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlabouardy mlabouardy mlabouardy approved these changes

@ShubhamPalriwala ShubhamPalriwala ShubhamPalriwala approved these changes

Assignees
No one assigned
Labels
gcp
Projects
None yet
Milestone
v3.0.20
Development

Successfully merging this pull request may close these issues.
3 participants
@nemca @ShubhamPalriwala @mlabouardy