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

Support priorityClassName in Helm chart #2345

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

EladDolev
Copy link
Contributor

Description

This PR adds support for defining priorityClassName on some of the pods

It is especially useful for the liqo-route as in real case scenarios it is not being schedule to some of the worker nodes and it breaks the network for anything which is running there. When assigning a high (enough) priority to liqo-route it is always scheduled to all of the worker nodes

How Has This Been Tested?

  • Test A
    Installed with default values -> no priorityClassName exists on any of the pods
  • Test B
    Assigned priorityClassName values to all relevant workloads and the workloads are running with priorityClassName

@adamjensenbot
Copy link
Collaborator

Hi @EladDolev. Thanks for your PR!

I am @adamjensenbot.
You can interact with me issuing a slash command in the first line of a comment.
Currently, I understand the following commands:

  • /rebase: Rebase this PR onto the master branch (You can add the option test=true to launch the tests
    when the rebase operation is completed)
  • /merge: Merge this PR into the master branch
  • /build Build Liqo components
  • /test Launch the E2E and Unit tests
  • /hold, /unhold Add/remove the hold label to prevent merging with /merge

Make sure this PR appears in the liqo changelog, adding one of the following labels:

  • kind/breaking: 💥 Breaking Change
  • kind/feature: 🚀 New Feature
  • kind/bug: 🐛 Bug Fix
  • kind/cleanup: 🧹 Code Refactoring
  • kind/docs: 📝 Documentation

@fra98
Copy link
Member

fra98 commented Feb 27, 2024

Hi @EladDolev, thanks for your contribution. It seems like a useful feature.

Btw, keep in mind that the liqo-route daemonset is going to be replaced in the next release, due to a major refactoring of the liqo networking.

@fra98 fra98 added the kind/feature New feature or request label Feb 27, 2024
Copy link
Member

@fra98 fra98 left a comment

Choose a reason for hiding this comment

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

Can you also add the PriorityClass value to the crdReplicator, discovery, and metric-agent pods?
Also, you can correctly generate the README.md file with the command make docs.

@EladDolev
Copy link
Contributor Author

Done as requested @fra98
Much appreciated 🙏

@fra98
Copy link
Member

fra98 commented Feb 29, 2024

LGTM!
Can you please rebase to the latest commit on master and squash the commits into one? Then we are ready to merge

@EladDolev
Copy link
Contributor Author

Done @fra98
Thanks !!

@fra98
Copy link
Member

fra98 commented Mar 1, 2024

/rebase test=true

@fra98
Copy link
Member

fra98 commented Mar 1, 2024

/rebase

Signed-off-by: Elad Dolev <dolevelad@gmail.com>
@fra98
Copy link
Member

fra98 commented Mar 1, 2024

/test

@fra98
Copy link
Member

fra98 commented Mar 1, 2024

/merge

@adamjensenbot adamjensenbot added the merge-requested Request bot merging (automatically managed) label Mar 1, 2024
@adamjensenbot adamjensenbot merged commit cc1034b into liqotech:master Mar 1, 2024
13 checks passed
@adamjensenbot adamjensenbot removed the merge-requested Request bot merging (automatically managed) label Mar 1, 2024
@EladDolev EladDolev deleted the priority_class branch March 1, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants