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

[jfavellar90]

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

[openedx-webhooks]

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]

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]

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]

@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]

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]

@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]

@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]

@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: https://github.com/openedx/openedx-k8s-harmony/issues/25#issuecomment-1477716458

[itsjeyd]

Thanks for reviewing @adzuci!

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

[jfavellar90]

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]

@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).

[jfavellar90]

@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]

@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.

[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

@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]

@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.

[openedx-webhooks]

@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.