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.

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

Add node subcommand #95

Merged
merged 8 commits into from
Jun 14, 2021
Merged

Add node subcommand #95

merged 8 commits into from
Jun 14, 2021

Conversation

kaelanspatel
Copy link
Contributor

Ref #78.

Adds node subcommand which generates a cost table filtered by individual nodes. Also implements QueryAssets and writeAssetTable which can be modified to be used for other asset types in the future.

Sample output:

+-------------+---------------------------------------------+------------+--------------+--------------+--------------+
| CLUSTER     | NAME                                        | ASSET TYPE | CPU COST     | RAM COST     | TOTAL COST   |
+-------------+---------------------------------------------+------------+--------------+--------------+--------------+
| cluster-one | gke-test-cluster-pool-1-9bb98ef8-cf3j       | e2-medium  |     0.540200 |     0.278558 |     0.818758 |
|             | gke-test-cluster-pool-1-9bb98ef8-kdsf       | e2-medium  |     0.540200 |     0.278558 |     0.818758 |
|             | gke-test-cluster-default-pool-d6266c7c-dqms | e2-medium  |     0.540200 |     0.278557 |     0.818758 |
|             | gke-test-cluster-pool-1-9bb98ef8-3w6g       | e2-medium  |     0.540200 |     0.278557 |     0.818758 |
+-------------+---------------------------------------------+------------+--------------+--------------+--------------+
| SUMMED      |                                             |            | USD 2.160800 | USD 1.114230 | USD 3.275032 |
+-------------+---------------------------------------------+------------+--------------+--------------+--------------+```

Tested locally and cross-referenced with kubecost Assets view.

@dwbrown2
Copy link
Contributor

Awesome!! Have a couple questions/thoughts:

  • Are these monthly cost figures by default? If so, should we highlight/indicate somehow?
  • I actually think this could make for a really cool blog post, i.e. Seeing the cost of Kubernetes cluster assets with kubectl
  • It seems like kubectl cost assets could be a super set of this command, plus LBs, disks, etc. Just an idea..

@michaelmdresser
Copy link
Contributor

michaelmdresser commented Jun 10, 2021

It seems like kubectl cost assets could be a super set of this command, plus LBs, disks, etc. Just an idea..

The trouble with this is that each asset type has different fields. We could certainly do kubectl cost assets and have it show only total cost and asset type, but I don't think we can do much more than that.

@dwbrown2
Copy link
Contributor

dwbrown2 commented Jun 10, 2021 via email

@kaelanspatel
Copy link
Contributor Author

Awesome!! Have a couple questions/thoughts:

  • Are these monthly cost figures by default? If so, should we highlight/indicate somehow?
  • I actually think this could make for a really cool blog post, i.e. Seeing the cost of Kubernetes cluster assets with kubectl
  • It seems like kubectl cost assets could be a super set of this command, plus LBs, disks, etc. Just an idea..

In regards to the first point, the monthly cost is by default. This is in line with the other kubectl cost commands, so I just made it that way for consistency's sake; like the other commands, changing from monthly projected to cumulative over the window is done using the --historical flag.

Copy link
Contributor

@michaelmdresser michaelmdresser left a comment

Choose a reason for hiding this comment

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

Great work so far. I'd also like to see an update to the README ("Usage", see the list of subcommands) and the usage section with an example of kubectl cost node

pkg/cmd/common.go Outdated Show resolved Hide resolved
pkg/cmd/common.go Outdated Show resolved Hide resolved
pkg/cmd/common.go Outdated Show resolved Hide resolved
pkg/cmd/node.go Outdated Show resolved Hide resolved
pkg/query/assetsapi.go Outdated Show resolved Hide resolved
Comment on lines 37 to 47
// aggregate, accumulate, and disableAdjustments are hardcoded;
// as other asset types are added in to be filtered by, this may change,
// but for now anything beyond isn't needed.

requestParams := map[string]string{
"window": p.Window,
"aggregate": "",
"accumulate": "true",
"disableAdjustments": "true",
"filterTypes": p.FilterTypes,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If they're hardcoded, don't provide them as options in the struct.

pkg/query/assetsapi.go Outdated Show resolved Hide resolved
pkg/cmd/output.go Outdated Show resolved Hide resolved
Comment on lines 43 to 45
"aggregate": "",
"accumulate": "true",
"disableAdjustments": "false",
Copy link
Contributor

Choose a reason for hiding this comment

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

For aggregate, accumulate, and disableAdjustments, either use the value from the parameter struct or remove the option from the parameter struct entirely. I would recommend removing these from the parameter struct entirely as I don't think we have a use case yet for configuring these and I don't anticipate one soon.

Copy link
Contributor

@michaelmdresser michaelmdresser left a comment

Choose a reason for hiding this comment

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

Great! I'd just ask that you update this list of subcommands:

There are several supported subcommands: `namespace`, `deployment`, `controller`, `label`, `pod`, and `tui`, which display cost information aggregated by the name of the subcommand (see Examples). Each subcommand has two primary modes, rate and non-rate. Rate (the default) displays the projected monthly cost based on the activity during the window. Non-rate (`--historical`) displays the total cost for the duration of the window.

@kaelanspatel kaelanspatel merged commit 2dc847e into main Jun 14, 2021
@tsunamishaun
Copy link

@michaelmdresser can this make its way into a release? Looking forward to using it :)

@michaelmdresser michaelmdresser deleted the kaelan-addassets branch September 10, 2021 14:30
@kaelanspatel
Copy link
Contributor Author

Hi @tsunamishaun.

With the latest release, the node subcommand should be ready for use. Hope it's useful!

@dwbrown2
Copy link
Contributor

Awesome, @kaelanspatel! Do we have plan to support an asset subcommand that would be a general iteration of all paid assets in this cluster? Happy to provide input if you do think something like this could be useful.

@kaelanspatel
Copy link
Contributor Author

@dwbrown2 Yes, that's definitely something we should do. I think the plan was to either eventually have an asset subcommand like you say (which could be filtered further e.g. asset node), or to simply add more general subcommands which correspond to assets like node does (e.g. lb, disk, cloud).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants