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 support for cost difference in "predict" using new API #141

Merged
merged 2 commits into from
Feb 9, 2023

Conversation

michaelmdresser
Copy link
Contributor

@michaelmdresser michaelmdresser commented Feb 8, 2023

What does this PR change?

See release note. Diff behavior matches the current diff used by our new Admission Controller.

How does this PR impact users? (This is the kind of thing that goes in release notes!)

  • Added basic cost diffing support to kubectl cost predict. The default behavior will attempt to match workload specs with existing workloads tracked by Kubecost and will calculate a cost diff based on resource request. Disable with --no-diff.
  • Added a namespace to the kubectl cost predict workload name output. If a namespace is not set on a given workload, it will be inherited from your Kubeconfig context.

How was this PR tested?

  • Targeted a dev cluster and the nightly cluster using the updated test/multi.yaml file. Observed full cost diffs on dev cluster (KC deployed in nonstandard namespace) and a true diff on the nightly cluster, as expected.
→ go run cmd/kubectl-cost/kubectl-cost.go predict -f ./test/multi.yaml --window '2h'
+--------------------------------------------+------+-------+-----+------------+------------+-------------+------------+------------+-------------+
| WORKLOAD                                   | CPU  | MEM   | GPU | CPU/MO     | MEM/MO     | GPU/MO      | Δ CPU/MO   | Δ MEM/MO   | TOTAL/MO    |
+--------------------------------------------+------+-------+-----+------------+------------+-------------+------------+------------+-------------+
| default/Deployment/nginx-deployment        | 9    | 6Gi   | 0   | 207.72 USD |  18.54 USD |    0.00 USD | 207.72 USD |  18.54 USD |  226.26 USD |
| kubecost/Deployment/kubecost-cost-analyzer | 15m  | 9Mi   | 0   |   0.35 USD |   0.03 USD |    0.00 USD |  -4.40 USD |  -0.37 USD |    0.37 USD |
| default/Deployment/nginx-deployment-2      | 10   | 35Gi  | 0   | 230.80 USD | 108.15 USD |    0.00 USD | 230.80 USD | 108.15 USD |  338.95 USD |
| default/Pod/nginx-pod                      | 350m | 200Mi | 0   |   8.08 USD |   0.60 USD |    0.00 USD |   8.08 USD |   0.60 USD |    8.68 USD |
| default/Pod/nginx-pod                      | 350m | 200Mi | 3   |   8.08 USD |   0.60 USD | 2080.50 USD |   8.08 USD |   0.60 USD | 2089.18 USD |
+--------------------------------------------+------+-------+-----+------------+------------+-------------+------------+------------+-------------+
|                                            |      |       |     | 455.02 USD | 127.92 USD | 2080.50 USD | 450.28 USD | 127.53 USD | 2663.45 USD |
+--------------------------------------------+------+-------+-----+------------+------------+-------------+------------+------------+-------------+

Have you made an update to documentation?

Yes, README update in this PR.

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
@michaelmdresser michaelmdresser force-pushed the mmd/prediction-naive-diff-support branch from f6637ae to 0777015 Compare February 9, 2023 03:46
@michaelmdresser michaelmdresser marked this pull request as ready for review February 9, 2023 03:46
@AjayTripathy
Copy link
Contributor

Super cool! cc @kwombach12 for UX feedback but lgtm!

@kwombach12
Copy link

kwombach12 commented Feb 9, 2023

@michaelmdresser very cool. The data is readable and well-presented.

@michaelmdresser michaelmdresser merged commit 1f58521 into main Feb 9, 2023
@michaelmdresser michaelmdresser deleted the mmd/prediction-naive-diff-support branch February 9, 2023 18:38
@dwbrown2
Copy link
Contributor

I suggest we explicitly state that it's the diff if we're showing this by default... i.e. we could have one sentence before the table showing something like:

"Expected monthly cost change is as follows:"

vs

"Expected monthly total cost is as follows:"

Thoughts?

@michaelmdresser
Copy link
Contributor Author

The table shows both total cost and cost change on two resources: cpu and memory. If you think the delta symbol isn't universal enough to convey "difference" we can think up something to address it. I mainly want to avoid cluttering up the terminal output, as IMO its better for tools like this to be straight to the point with useful --help text if users are confused. Happy to update that, too if it isn't clear.

@dwbrown2
Copy link
Contributor

I see your point. I think some users may either miss or not understand the user of delta. One reason for the latter might be that it's not applied to "Total/Mo" Column. Should that be changed to Diff/Mo?

@michaelmdresser
Copy link
Contributor Author

The current predict API only diffs CPU and memory but kubectl cost predict also shows GPU cost. This means we can't put a diff on total cost, only on CPU and memory individually right now. So, the total/mo column right now is not a diff; even now I can recognize how that might be confusing.

I'm hoping it will be straightforward to address this UX shortcoming once I finish the better predict API, incorporating what we've learned.

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

Successfully merging this pull request may close these issues.

None yet

4 participants