feat: adding metrics-server and vpa as dependencies of the main chart #17

jfavellar90 · 2023-02-07T13:58:08Z

This PR is adding a couple of dependencies required to enable POD autoscaling in a Kubernetes cluster

openedx-webhooks · 2023-02-07T13:58:12Z

Thanks for the pull request, @jfavellar90! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

bradenmacdonald · 2023-02-08T20:49:48Z

tutor-multi-chart/Chart.yaml

+- name: vertical-pod-autoscaler
+  version: "6.0.3"
+  repository: https://cowboysysop.github.io/charts/
+  alias: vpa


Metrics server totally makes sense, though we do need to make sure we document a variable that can be used to not try to install it, for people who already have it installed on their cluster.

For VPA, how will we use it? See my questions/update on #2 (comment)

@bradenmacdonald I added a couple of conditions to define is the sub-charts are going to be installed or not.
Regarding the VPA topic, my approach is still naive for now. In the plugin, I created there are variables that enabled VPA for the main deployments (LMS, CMS, and workers) and creates VPA objects that will increase/decrease the allocated resources of every pod according to current cluster usage

More details here: https://www.kubecost.com/kubernetes-autoscaling/kubernetes-vpa/

bradenmacdonald · 2023-02-24T22:28:04Z

Thanks @jfavellar90. Would it be possible for you to also update the README for this repo to explain how to use it together with https://github.com/eduNEXT/tutor-contrib-pod-autoscaling ?

@gabor-boros would you like to review this?

jfavellar90 · 2023-03-06T02:35:40Z

Thanks @jfavellar90. Would it be possible for you to also update the README for this repo to explain how to use it together with https://github.com/eduNEXT/tutor-contrib-pod-autoscaling ?

@gabor-boros would you like to review this?

@bradenmacdonald sorry for the delayed answer, I'll do it over the course of tomorrow

jfavellar90 · 2023-03-07T13:48:50Z

@bradenmacdonald I updated the docs adding simple steps on how to enable the autoscaling feature with the plugin. Please let me know if you have any questions.

itsjeyd · 2023-03-14T14:47:02Z

Hi @jfavellar90, could you please bring your branch up-to-date with the main branch, and post an update here when you're done?

That would be great as it would help minimize the number of review cycles and get your changes ready for merge sooner.

jfavellar90 · 2023-03-20T13:25:10Z

@itsjeyd the PR is already rebased on top of the main branch. It's ready to review, please check the documentation I added here. Please let me know if there's anything left to do.

itsjeyd · 2023-03-21T08:23:00Z

@jfavellar90 Thanks for the update! This is ready for review now, and there is no action needed from you at the moment.

@bradenmacdonald @gabor-boros Over to you.

gabor-boros · 2023-03-21T12:05:24Z

@bradenmacdonald If I understand correctly, I would need to be a core contributor to review this, which would require a 20h commitment , that I have concerns with from Serenity point of view at the moment. Ref: #25 (comment)

adzuci · 2023-03-21T17:38:25Z

tutor-multi-chart/values.yaml

@@ -61,3 +61,19 @@ elasticsearch:
      xpack.security.transport.ssl.certificate: /usr/share/elasticsearch/config/certs/tls.crt
      xpack.security.transport.ssl.certificate_authorities: /usr/share/elasticsearch/config/certs/ca.crt
      xpack.security.transport.ssl.verification_mode: certificate
+
+# Configuration for the metrics server chart
+metricsserver:


Would it make sense to nest this block with other related blocks?

@adzuci Which other related blocks are you referring to exactly?

adzuci · 2023-03-21T17:42:19Z

tutor-multi-chart/values.yaml

+# Configuration for the metrics server chart
+metricsserver:
+   # Control the chart inclusion
+   enabled: false


I believe some other charts name this variable something like "installMetricsServer", though I'm not sure if that makes sense here. Could that pattern be useful here? Also, do we intend to enable people to link to an existing metrics server, but set this flag to false?

@adzuci I took this enabled flag from the kube-prometheus-stack Chart. Could you please link an example of the pattern you're proposing?
Regarding the metrics-server option, in the docs we specify that this dependency should be included only if still not present in the cluster. However, we still need to test what happens when trying to install metrics-server if already present in a different namespace.

The installation of the helm-chart will fail if metrics-server is present in a different namespace

itsjeyd · 2023-03-28T12:07:26Z

Thanks for reviewing @adzuci!

@jfavellar90 Back to you for addressing the comments above :)

bradenmacdonald

I tested this with vpa.enabled: true and metricsserver.enabled: true, and I got an error at first because those subcharts are missing from the charts directory. I had to run helm dependency update which made some changes to the Chart.lock file that need to be committed. Could you please do that and include the changes in this PR?

Then I'll approve it :) Everything else is working great.

README.md

jfavellar90 · 2023-04-05T14:27:22Z

I tested this with vpa.enabled: true and metricsserver.enabled: true, and I got an error at first because those subcharts are missing from the charts directory. I had to run helm dependency update which made some changes to the Chart.lock file that need to be committed. Could you please do that and include the changes in this PR?

Then I'll approve it :) Everything else is working great.

@bradenmacdonald I already committed the Chart.lock changes. However, I still need to run helm dependency update before installing the chart since the charts folder content is now ignored for tgz files. I think this is going to be solved once we package and make the helm chart publicly accessible.

itsjeyd · 2023-04-13T07:12:53Z

@jfavellar90

I think this is going to be solved once we package make the helm chart publicly accessible.

Is that something you're planning on doing in the context of this PR? If not, it might make sense to mark this PR as blocked on other work (that's why I'm asking).

- fixing docs typos - Adding example values for Helm Chart installation - Updating Chart.lock file

jfavellar90 · 2023-04-13T15:36:05Z

@itsjeyd

I think this is going to be solved once we package make the helm chart publicly accessible.

Is that something you're planning on doing in the context of this PR? If not, it might make sense to mark this PR as blocked on other work (that's why I'm asking).

In the context of this PR not, however, we should not block the PR on other work, what do you think @bradenmacdonald ?

bradenmacdonald · 2023-04-13T21:45:17Z

@jfavellar90 I'm not sure what you mean by making the helm chart publicly accessible. Do you mean publishing it on Artefact Hub? Because it is already public in this repo and it's working.

I still need to run helm dependency update before installing the chart since the charts folder content is now ignored for tgz files.

Maybe we shouldn't be ignoring that folder? Or we can ignore it and just tell people to run that command in the README.

This comes from Helm best practices: https://helm.sh/docs/chart_best_practices/dependencies/#versions

…ate command

jfavellar90 · 2023-04-14T04:24:04Z

I'm not sure what you mean by making the helm chart publicly accessible. Do you mean publishing it on Artefact Hub? Because it is already public in this repo and it's working

@bradenmacdonald Exactly, I meant to host the chart in a remote chart repo (this is indeed an item included on the next steps of this project). I'm sorry, I wasn't explaining my idea clearly.

I was taking as an example the Kube Prometheus Stack chart. This chart has few dependencies, however, the charts folder is ignored and not included in the chart. After diving into the code for a while I realized that this helm chart is packaged using Github actions, resulting in an artifact included in the chart GitHub releases. An example of this artifact can be found in this release, specifically the file with extension tgz. If this file is extracted, you'll find the charts folder and, inside, the dependencies of the chart.
This is done thanks to a tool called chart-releaser. We can consider using this to package and host the harmony chart on GitHub pages as Prometheus does with their community charts.

Maybe we shouldn't be ignoring that folder? Or we can ignore it and just tell people to run that command in the README.

For now, I opted for pushing the dependencies artifacts to the charts folder to prevent running the helm dependency update command on chart installation. Once we define a way to package and host the chart, this will no longer be necessary. Please let me know if you're able to install the chart with no issue :)

jfavellar90 · 2023-04-18T17:51:16Z

@bradenmacdonald just a couple of comments:

Based on the updates you applied to the global README file, I added the autoscaling docs in the Q&A section.
For the chart dependencies I'm now using patch-level versions, which are recommended here.

bradenmacdonald

I tested again, and it worked perfectly out of the box. metricsserver and VPA are both installed and running. Nice work @jfavellar90 :)

openedx-webhooks · 2023-04-18T22:22:01Z

@jfavellar90 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 7, 2023

jfavellar90 mentioned this pull request Feb 7, 2023

Add autoscaling #2

Closed

itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 8, 2023

bradenmacdonald reviewed Feb 8, 2023

View reviewed changes

bradenmacdonald mentioned this pull request Feb 27, 2023

Monitoring with Prometheus (and metrics-server) #3

Closed

jfavellar90 force-pushed the Jhony/autoscaling branch from bb04363 to d4212e7 Compare March 7, 2023 13:45

jfavellar90 marked this pull request as ready for review March 7, 2023 13:47

itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Mar 8, 2023

itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Mar 14, 2023

jfavellar90 force-pushed the Jhony/autoscaling branch from d4212e7 to 3eae75e Compare March 20, 2023 13:20

itsjeyd requested a review from bradenmacdonald March 21, 2023 08:21

itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Mar 21, 2023

itsjeyd assigned bradenmacdonald Mar 21, 2023

adzuci reviewed Mar 21, 2023

View reviewed changes

itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Mar 28, 2023

bradenmacdonald requested changes Apr 3, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

jfavellar90 added 3 commits April 13, 2023 10:07

feat: adding metrics-server and vpa as dependencies of the main chart

95f658e

chore: adding conditionals to enable/disable subcharts

784d10b

docs: complementing README file adding the autoscaling section

7afd718

fix: addressing PR comments

aaa2d35

- fixing docs typos - Adding example values for Helm Chart installation - Updating Chart.lock file

jfavellar90 force-pushed the Jhony/autoscaling branch from b3c08a5 to aaa2d35 Compare April 13, 2023 15:25

jfavellar90 added 2 commits April 13, 2023 23:14

fix: using patch level version match

2d5f139

This comes from Helm best practices: https://helm.sh/docs/chart_best_practices/dependencies/#versions

fix: pushing dependencies artifacts to prevent running dependency upd…

adc1ebd

…ate command

bradenmacdonald approved these changes Apr 18, 2023

View reviewed changes

bradenmacdonald merged commit ac53842 into openedx:main Apr 18, 2023

itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 20, 2023

jfavellar90 deleted the Jhony/autoscaling branch May 2, 2023 13:28

This was referenced May 30, 2023

Adding a subchart to host grafana as a global service #37

Closed

Adding a subchart to include prometheus as a data source for grafana #38

Closed

itsjeyd added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label Jul 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding metrics-server and vpa as dependencies of the main chart #17

feat: adding metrics-server and vpa as dependencies of the main chart #17

jfavellar90 commented Feb 7, 2023

openedx-webhooks commented Feb 7, 2023 •

edited

Loading

bradenmacdonald Feb 8, 2023

jfavellar90 Feb 21, 2023

jfavellar90 Feb 21, 2023

bradenmacdonald commented Feb 24, 2023

jfavellar90 commented Mar 6, 2023

jfavellar90 commented Mar 7, 2023

itsjeyd commented Mar 14, 2023

jfavellar90 commented Mar 20, 2023

itsjeyd commented Mar 21, 2023

gabor-boros commented Mar 21, 2023

adzuci Mar 21, 2023

jfavellar90 Apr 5, 2023 •

edited

Loading

adzuci Mar 21, 2023

jfavellar90 Apr 5, 2023

jfavellar90 Apr 5, 2023

itsjeyd commented Mar 28, 2023 •

edited

Loading

bradenmacdonald left a comment

jfavellar90 commented Apr 5, 2023 •

edited

Loading

itsjeyd commented Apr 13, 2023

jfavellar90 commented Apr 13, 2023 •

edited

Loading

bradenmacdonald commented Apr 13, 2023

jfavellar90 commented Apr 14, 2023 •

edited

Loading

jfavellar90 commented Apr 18, 2023

bradenmacdonald left a comment

openedx-webhooks commented Apr 18, 2023

feat: adding metrics-server and vpa as dependencies of the main chart #17

feat: adding metrics-server and vpa as dependencies of the main chart #17

Conversation

jfavellar90 commented Feb 7, 2023

openedx-webhooks commented Feb 7, 2023 • edited Loading

bradenmacdonald Feb 8, 2023

Choose a reason for hiding this comment

jfavellar90 Feb 21, 2023

Choose a reason for hiding this comment

jfavellar90 Feb 21, 2023

Choose a reason for hiding this comment

bradenmacdonald commented Feb 24, 2023

jfavellar90 commented Mar 6, 2023

jfavellar90 commented Mar 7, 2023

itsjeyd commented Mar 14, 2023

jfavellar90 commented Mar 20, 2023

itsjeyd commented Mar 21, 2023

gabor-boros commented Mar 21, 2023

adzuci Mar 21, 2023

Choose a reason for hiding this comment

jfavellar90 Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

adzuci Mar 21, 2023

Choose a reason for hiding this comment

jfavellar90 Apr 5, 2023

Choose a reason for hiding this comment

jfavellar90 Apr 5, 2023

Choose a reason for hiding this comment

itsjeyd commented Mar 28, 2023 • edited Loading

bradenmacdonald left a comment

Choose a reason for hiding this comment

jfavellar90 commented Apr 5, 2023 • edited Loading

itsjeyd commented Apr 13, 2023

jfavellar90 commented Apr 13, 2023 • edited Loading

bradenmacdonald commented Apr 13, 2023

jfavellar90 commented Apr 14, 2023 • edited Loading

jfavellar90 commented Apr 18, 2023

bradenmacdonald left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Apr 18, 2023

openedx-webhooks commented Feb 7, 2023 •

edited

Loading

jfavellar90 Apr 5, 2023 •

edited

Loading

itsjeyd commented Mar 28, 2023 •

edited

Loading

jfavellar90 commented Apr 5, 2023 •

edited

Loading

jfavellar90 commented Apr 13, 2023 •

edited

Loading

jfavellar90 commented Apr 14, 2023 •

edited

Loading