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

Improve Fleet performance #12896

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

aruiz14
Copy link
Contributor

@aruiz14 aruiz14 commented Dec 17, 2024

Summary

Issue #12895

Performance improvements while keeping the same behavior.

Occurred changes and/or fixed issues

(Changes are logically split into different commits)

  • Standardize on clusterID vs. "clusterLabel" (which normally refers to the management Cluster resource name, while the Fleet Cluster name is usually the same as the "display" name).
    • The clusterId is compound by ${namespace}/${name} of the Cluster resource, making it easier to work with.
    • The management "Cluster" object (clusterLabel) is only really needed for calculating the detailLocation of deployed resouces, as they will link to the Explorer, which uses the management clusters' names.
  • Refactor usages of clusterResourceStatus, which is always combined with a find by clusterLabel (now clusterId).
    • Used to obtain the resource statuses count per cluster.
    • Used to get the cluster state (based in the resource statuses aggregation).
    • This was being performed for every cluster individually, so it impacts performance exponentially to the number downstream clusters.
    • Reworking this allows to finally get rid of this function completely 🎉 .
  • The Continuous Delivery dashboard iterates over all gitrepos/bundles/clusters in order obtain the badges and tooltip for every single entry in the table.
    • I've computed the necessary information in a single place and passed it to the relevant functions so they can easily pick what they need, hence greatly reducing the number of iterations (see screenshot below)
  • Small optimizations (e.g. use a variable to avoid calling the same getters multiple times within the same function).

Technical notes summary

Above changes were driven by capturing and analyzing Performance traces with the Chrome Dev Tools, on a local Rancher installation where many sample workloads were installed.
See a summary of CPU usage over 30s of loading the CD dashboard:

  • Before:
    Screenshot 2024-12-10 at 12 42 50
  • After:
    Screenshot 2024-12-10 at 12 42 59

Areas or cases that should be tested

  • Continuous Delivery:
    • Dashboard
    • Cluster list
    • Cluster details
    • GitRepo list
    • GitRepo details

Areas which could experience regressions

These changes aim for improving performance while not introducing any changes to the information being displayed.
I've performed several tests using a local UI build and couldn't find any difference, but I may have missed something.

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

@aruiz14 aruiz14 added this to the v2.11.0 milestone Dec 17, 2024
@aruiz14 aruiz14 requested a review from richard-cox December 17, 2024 11:25
@aruiz14 aruiz14 force-pushed the perf-improvements branch 2 times, most recently from 1783856 to 47662d6 Compare December 17, 2024 16:18
@torchiaf torchiaf self-requested a review January 7, 2025 08:49
@richard-cox
Copy link
Member

@aruiz14 i had to re-run all jobs given the build job output had expired, this has brought in an issue in check-i18n-links that should be fixed with a rebase

@aruiz14 aruiz14 force-pushed the perf-improvements branch 2 times, most recently from c6c3951 to 8479f60 Compare January 9, 2025 16:49
aruiz14 added a commit that referenced this pull request Jan 15, 2025
@torchiaf
Copy link
Member

torchiaf commented Jan 16, 2025

@aruiz14 I found a difference in status colors between main and your branch.
I'm not sure if it is a wanted change or implicit bug fix:

Current branch vs main
image

However, Bundles Ready count is wrong, it should be 1/1

Copy link
Member

@torchiaf torchiaf left a comment

Choose a reason for hiding this comment

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

Left some comments about Dashboard -> Status columns.

shell/models/fleet.cattle.io.gitrepo.js Outdated Show resolved Hide resolved
@aruiz14 aruiz14 requested a review from torchiaf January 21, 2025 12:36
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.

3 participants