-
Notifications
You must be signed in to change notification settings - Fork 859
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
feat: add metrics-adapter component to support centralized hpa #3578
Conversation
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.
The code gen is failing.
4f7c279
to
79436c5
Compare
Signed-off-by: jwcesign <jiangwei115@huawei.com>
b7ed104
to
7d712e1
Compare
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #3578 +/- ##
==========================================
- Coverage 55.86% 55.83% -0.03%
==========================================
Files 216 216
Lines 20121 20129 +8
==========================================
Hits 11240 11240
- Misses 8276 8284 +8
Partials 605 605
Flags with carried forward coverage won't be shown. Click here to find out more.
|
7d712e1
to
4177cb0
Compare
return nodeMetrics, nil | ||
} | ||
|
||
// In the previous step, if query with label selector, the name will be set to empty |
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 have some doubts. Why are there some metrics related to nodes here when there are no node objects in the Karmada control panel?
kubectl --kubeconfig karmada.kubeconfig get --raw /apis/metrics.k8s.io/v1beta1/nodes
# Some information will be returned.
kubectl --kubeconfig karmada.kubeconfig get nodes
# No resources found
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.
The HPA controller will exclusively utilize the /apis/metrics.k8s.io/v1beta1/pods
API. Therefore, it is acceptable if the other API is not available. In my opinion, users should use karmadactl top
to inquire about metrics.
However, if we do provide the metrics API, we cannot prevent users from using it. If we choose not to provide it, there will be a need for an explanation.
cc @RainbowMango for checking
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.
In my opinion, users should use karmadactl top to inquire about metrics.
For some UI projects, these project only use apis and commands are not used.
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 prefer to keep the node metrics as we are unsure if users will utilize them.
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.
To be continue.
From the test results, it seems that using I suggest introducing a
|
I think it's a good idea. cc @lonelyCZ , |
c6f5cb9
to
e1429fb
Compare
Signed-off-by: jwcesign <jiangwei115@huawei.com>
e1429fb
to
ae6c345
Compare
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.
Generally LGTM
} | ||
|
||
// PodLister is an internal lister for pods | ||
type PodLister struct { |
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.
This struct implement cache.GenericLister
interface. I'd like to add the following to ensure this interface is fully implemented.
var _ cache.GenericLister = &PodLister{}
var _ v1listers.NodeLister = &NodeLister{}
var _ api.MetricsGetter = &ResourceMetricsProvider{}
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.
When init the metrics server, we use these two listers, there will be type checking. So whether is it necessary?
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.
Generally looks good to me. But I didn't look carefully.
Please @chaunceyjiang @Poor12 take a look.
4fb2ee6
to
bf24272
Compare
b532f65
to
4cd3d3f
Compare
4cd3d3f
to
f87f74f
Compare
4e33e8c
to
8dc8c7e
Compare
Signed-off-by: jwcesign <jwcesign@gmail.com>
8dc8c7e
to
6dfa4d0
Compare
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.
/lgtm
/approve
/hold
(I need to get the image registry ready before moving forward)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Part of #3379
This PR focus on karmada-metrics-adapter, the responsibility is:
Which issue(s) this PR fixes:
Fixes #none
Special notes for your reviewer:
Install way
Test
Does this PR introduce a user-facing change?: