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

feat: Update cpu and memory charts on namespace #3711

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

KonradPietocha
Copy link
Contributor

@KonradPietocha KonradPietocha commented Feb 17, 2025

Description

Changes proposed in this pull request:

  • Moved old CPU and memory charts from namespace overview to resource quota view.
  • Added new CPU Usage and Memory Usage charts on the namespace overview.
  • Created appropriate tests for charts.

Related issue(s)
Closes #3592

Definition of done

  • The PR's title starts with one of the following prefixes:
    • feat: A new feature
    • fix: A bug fix
    • docs: Documentation only changes
    • refactor: A code change that neither fixes a bug nor adds a feature
    • test: Adding tests
    • revert: Revert commit
    • chore: Maintainance changes to the build process or auxiliary tools, libraries, workflows, etc.
  • Related issues are linked. To link internal trackers, use the issue IDs like backlog#4567
  • Explain clearly why you created the PR and what changes it introduces
  • All necessary steps are delivered, for example, tests, documentation, merging

@kyma-bot kyma-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cla: yes Indicates the PR's author has signed the CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 17, 2025
@KonradPietocha KonradPietocha changed the title Update cpu and memory charts on namespace feat: Update cpu and memory charts on namespace Feb 17, 2025
@KonradPietocha KonradPietocha marked this pull request as ready for review February 17, 2025 13:59
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2025
Copy link
Contributor

@dbadura dbadura left a comment

Choose a reason for hiding this comment

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

Partial review

return podsMetrics.reduce((accData, pod) => {
return {
cpu: {
usage: accData.cpu.usage + (pod?.cpu?.usage ?? 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider to wrap all addition by getCpus or getBytes functions because sometimes k8s can return values with different prefix.

@mrCherry97 mrCherry97 removed their assignment Feb 19, 2025
Copy link
Contributor

@dbadura dbadura left a comment

Choose a reason for hiding this comment

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

image
I have encountered such bug?
I deployed function with XS size and add any resource quota. The memory usage is 0. Is it okay?

<React.Fragment key="resource-quota-limits">
{(!!podsMetrics?.length || !isEmpty(resource?.status)) && (
<div className="cluster-stats sap-margin-tiny">
{mapUsagesToChartsData(podsMetrics).map((chartData, index) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about merging those 2 tables before rendering happen and then render it?
It would remove duplication of code.

} as UseGetOptions) as { data: NodeList | null };

// Finding capacity for a given usage.
const getAllocatable = (
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be move as separate function. It's very long and it's inside function

>
{scope.scopeName}
</Title>
(resource: ResourceQuotaProps) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you like to rename ResourceQuotaProps to ResourceQuota because this type looks like k8s ResourceQuota

cy.contains('Events')
.scrollIntoView()
.should('be.visible');
});

it('checks the visibility of charts', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this test checks errors and loading?
I think it should check if charts are available and visible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update CPU and Memory Charts on Namespace Overview to Match Clusters Overview Layout
4 participants