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
move function GetKubemarkMasterComponentResoureUsage and remove long-time TODO #87271
move function GetKubemarkMasterComponentResoureUsage and remove long-time TODO #87271
Conversation
/priority backlog |
return sshResult.Stdout, nil | ||
} | ||
|
||
// getKubemarkMasterComponentsResourceUsage returns the resource usage of kubemark which contains multiple combinations of cpu and memory usage for each pod name. |
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.
see https://github.com/kubernetes/kubernetes/pull/87271/files#diff-b498843cdcc70479a96c0cfffc6290afL44
This TODO from https://github.com/kubernetes/kubernetes/pull/38507/files#diff-b498843cdcc70479a96c0cfffc6290afR31 4 years ago.
GetKubemarkMasterComponentsResourceUsage is only used by https://github.com/kubernetes/kubernetes/pull/87271/files#diff-572985ec546140c5cf2bc24658132e83R187
so make it private.
For
TODO: figure out how to move this to kubemark directory (need to factor test SSH out of e2e framework
I think that it is good time to re-examine and evaluate this TODO
if someone wishes to write a separate proposal for this work, please create an issue or TODO(@github) named to this function, We can easily track this down
@@ -556,3 +557,65 @@ func (g *ContainerResourceGatherer) StopAndSummarize(percentiles []int, constrai | |||
} | |||
return &summary, nil | |||
} | |||
|
|||
// KubemarkResourceUsage is a struct for tracking the resource usage of kubemark. | |||
type KubemarkResourceUsage 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.
can this structure become private
kubemarkResourceUsage
?
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.
yes . sorry forget it
@@ -183,7 +184,7 @@ type resourceGatherWorker struct { | |||
func (w *resourceGatherWorker) singleProbe() { | |||
data := make(ResourceUsagePerContainer) | |||
if w.inKubemark { | |||
kubemarkData := GetKubemarkMasterComponentsResourceUsage() | |||
kubemarkData := getKubemarkMasterComponentsResourceUsage() |
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 think the move is fine, but IMO in the future kubemark specifics should be outside of the framework and in a new package:
test/e2e/scalability
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.
sure.
TODO: move this to test/e2e/scalability see https://github.com/kubernetes/kubernetes/pull/87271#discussion_r367217336
We can explain this here.How about it?
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.
lets not add the TODO just yet.
i would like to see what others think about the new proposed location and decoupling the framework from kubemark.
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.
ok
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123, tanjunchen 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
ref:#86763
Special notes for your reviewer:
see #87271 (comment)
/cc @neolit123
/cc @oomichi
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: