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 wildcard support for evpa container policy, and update the status… #309

Conversation

kitianFresh
Copy link
Contributor

… even if it is off mode, because we need to record the metrics

What type of PR is this?

  1. support wildcard for evpa container
  2. refine the metrics for evpa to configure grafana
  3. fix some bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #308

Special notes for your reviewer:

@kitianFresh kitianFresh requested a review from qmhu May 10, 2022 12:02
@github-actions
Copy link
Contributor

github-actions bot commented May 10, 2022

🎉 Successfully Build Images.

Overview: https://finops.coding.net/public-artifacts/gocrane/crane/packages

Image Pull Command
crane-agent:pr-309-3338ef9 docker pull finops-docker.pkg.coding.net/gocrane/crane/crane-agent:pr-309-3338ef9
dashboard:pr-309-3338ef9 docker pull finops-docker.pkg.coding.net/gocrane/crane/dashboard:pr-309-3338ef9
metric-adapter:pr-309-3338ef9 docker pull finops-docker.pkg.coding.net/gocrane/crane/metric-adapter:pr-309-3338ef9
craned:pr-309-3338ef9 docker pull finops-docker.pkg.coding.net/gocrane/crane/craned:pr-309-3338ef9

pkg/controller/evpa/container_policy.go Show resolved Hide resolved
if currentCpu.Cmp(recommendCpu) > 0 {
// scale down
currCopy := currentCpu.DeepCopy()
currCopy.Sub(recommendCpu)
metrics.EVPACpuScaleDownMilliCores.With(labels).Set(float64(currCopy.MilliValue()))
metrics.EVPACpuScaleDownMilliCores.With(labels).Set(float64(currCopy.MilliValue()) / 1000.)
Copy link
Member

Choose a reason for hiding this comment

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

Millicores is more accurate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Millicores is more accurate here.

I see that other metrics such as kube-state-metrics is not have unit in metricnaming, it is just use as a label to denote the unit. such as cores and bytes. for example, we use it to join with kube_pod_container_resource_requests.

it is better for us to do metircs and dashboard

Copy link
Member

Choose a reason for hiding this comment

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

ok, please also change variable name.

if currentMem.Cmp(recommendMem) > 0 {
// scale down
currCopy := currentMem.DeepCopy()
currCopy.Sub(recommendMem)
metrics.EVPAMemoryScaleDownMB.With(labels).Set(float64(currCopy.Value() / 1024 / 1024))
metrics.EVPAMemoryScaleDownMB.With(labels).Set(float64(currCopy.Value()))
Copy link
Member

Choose a reason for hiding this comment

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

Use MB unit here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use MB unit here.

same reason as above, i think we should not use the unit here, keep the original bytes. and use label to denote the unit

Copy link
Member

Choose a reason for hiding this comment

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

ok, please also change variable name.

… even if it is off mode, because we need to record the metrics
@kitianFresh kitianFresh force-pushed the feature/support-container-wildcard-for-evpa-containerPolicies branch from 4c01942 to 3338ef9 Compare May 10, 2022 14:26
@qmhu
Copy link
Member

qmhu commented May 11, 2022

/LGTM

@qmhu qmhu merged commit c059ac5 into gocrane:main May 11, 2022
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.

Support container wildcard for evpa containerPolicies
2 participants