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

[helm] add kube-state-metric subchart dependency #6495

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

pkoutsovasilis
Copy link
Contributor

@pkoutsovasilis pkoutsovasilis commented Jan 8, 2025

What does this PR do?

This PR refactors the Elastic Agent Helm chart to include the kube-state-metrics (KSM) chart as a subchart dependency. The changes enable users to seamlessly deploy KSM alongside the Elastic Agent for all different deployment options, improving the out-of-the-box experience for Kubernetes monitoring.

Key changes include:

  • Addition of the KSM Helm chart as a subchart dependency.
  • Introduction of values that allow enabling or disabling KSM deployment (kube-state-metrics.enabled) and deploying Elastic Agent as a sidecar container to the former (kubernetes.state.agentAsSidecar.enabled)
    • PS: now that kube-state-metrics derive from another subchart deploying elastic-agent through ECK as a sidecar is not feasible. values.schema.json has been updated to prevent such configuration
  • Refactoring of the Helm chart structure to incorporate KSM configurations and clean-up unused code.
    • The templates that regard the input of kube-state-metrics in kubernetes integration got significantly reduced resulting in an easier to follow code
    • The templates that regard the k8s tuning of each preset based on the Helm values got migrated to a separate folder to promote segregation from input-related templates, thus resulting in an easier to follow code

This PR consists of 3 commits:

  • 546ec26 just introduce the kube-state-metric subchart dependency
  • 27748fa the actual changes of this PR
  • a019dbf examples showcasing the new functionality

since a019dbf, which is the main change of this PR, is +529 additions -1268 deletions I deem that this PR complies with the team PR policies but happy to try to split it up in smaller chunks if it doesn't 🙂

Why is it important?

This PR is important because it enhances the Elastic Agent Helm chart to simplify the deployment and management of Kubernetes monitoring. By including kube-state-metrics as a subchart dependency, the PR significantly reduces user friction and ensures better alignment with Kubernetes integration requirements.

The refactoring also:

  • Improves maintainability by reducing template complexity and segregating logic for better readability.
  • Provides flexibility for users to enable or disable KSM and enable or disable embedding elastic-agent as a sidecar to the former, catering to diverse use cases.

Checklist

  • I have read and understood the pull request guidelines of this project.
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool
  • I have added an integration test or an E2E test

Disruptive User Impact

This PR introduces a new default behavior that includes the kube-state-metrics chart as a dependency. By default:

  • kube-state-metrics is deployed. Users must explicitly disable it by setting kube-state-metrics.enabled=false to opt out of deploying
  • all kube-state-metrics related metrics are now captured by the cluster-wide agent preset. Users opting to deploy elastic-agent as a sidecar to kube-state-metrics must explicitly enable it in the values file by setting kubernetes.state.agentAsSidecar.enabled=true.
  • Unsupported configurations, such as deploying Elastic Agent as a sidecar to KSM managed by ECK, are now restricted.

For users upgrading existing installations, ksmSharded preset is gonna be uninstalled and their kube-state-metrics related flow will follow the logic above

How to test this PR locally

  • Follow one of the newly introduced examples
  • Run k8s integration tests
    mage integration:auth
    PLATFORMS=linux/arm64 EXTERNAL=true SNAPSHOT=true PACKAGES=docker mage -v package 
    INSTANCE_PROVISIONER=kind STACK_PROVISIONER=stateful K8S_VERSION=v1.31.1 SNAPSHOT=true mage integration:kubernetes
    

Related issues

@pkoutsovasilis pkoutsovasilis added Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team skip-changelog backport-8.x Automated backport to the 8.x branch with mergify labels Jan 8, 2025
@pkoutsovasilis pkoutsovasilis self-assigned this Jan 8, 2025
@pkoutsovasilis pkoutsovasilis marked this pull request as ready for review January 9, 2025 13:29
@pkoutsovasilis pkoutsovasilis requested a review from a team as a code owner January 9, 2025 13:29
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a question on the dependency keeping in sync with release.

@@ -5,3 +5,8 @@ kubeVersion: ">= 1.27.0-0"
type: application
appVersion: 9.0.0
version: 9.0.0-beta
dependencies:
- name: kube-state-metrics
version: "5.28.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to add any automation to keep this in-sync with released versions or is this going to be manual?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point 👌 I have to investigate how we could employ renovate to do that 🙂 do you believe this could be a follow-up PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

absolutely, in a follow up

this is already large enough ;-)

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

Looks good to me. I flagged some minor typos. I've also noticed that we're committing charts/ - that should be in .gitignore in my opinion.

@pkoutsovasilis
Copy link
Contributor Author

pkoutsovasilis commented Jan 14, 2025

Hey @swiatekm 👋 so just to understand, you are against committing charts/ however to me having it in the repo, it allows a developer/CI run to install the Helm chart directly from the diskpath. If charts/kube-state-metric-*.tgz is missing then you have either helm depedency build or helm package before being able to install the helm chart. And to this exact end, having it in our repo makes our CI potentially less flaky as there are no external dependencies to be downloaded during helm install. So what is your proposal on these points if we don't commit it?

@swiatekm
Copy link
Contributor

Hey @swiatekm 👋 so just to understand, you are against committing charts/ however to me having it in the repo, it allows a developer/CI run to install the Helm chart directly from the diskpath. If charts/kube-state-metric-*.tgz is missing then you have either helm depedency build or helm package before being able to install the helm chart. And to this exact end, having it in our repo makes our CI potentially less flaky as there are no external dependencies to be downloaded during helm install. So what is your proposal on these points if we don't commit it?

What you're proposing is vendoring the Helm Chart's dependencies. I'm not opposed to that in principle, but is there any particular reason to do it here, and not for the agent application itself? Package managers exist so dependencies can be installed in a reproducible and consistent manner. Users will install this Chart from a Helm repo, and I don't see much reason why we wouldn't require developers to do the same.

Separately, it's another artifact we'd have to keep in sync with Chart.lock.

@pkoutsovasilis
Copy link
Contributor Author

Separately, it's another artifact we'd have to keep in sync with Chart.lock.

I am not quite sure I understand why the above is an issue; in my thinking we could employ renovate to keep this in sync?

But again what is the counter-proposal here? Specifically, how should we change the Helm render examples, integration framework, etc. to accommodate for the chart/* to be omitted?

@pkoutsovasilis
Copy link
Contributor Author

@swiatekm 👋 As agreed after our offline sync here is the commit 3349bb0 that allows us to not have the chart/* inside the elastic-agent chart but makes sure that our mage targets and CI tests build the chart dependencies properly

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

👍

deploy/helm/elastic-agent/.gitignore Outdated Show resolved Hide resolved
Copy link

@pkoutsovasilis pkoutsovasilis merged commit edd342a into elastic:main Jan 17, 2025
13 checks passed
@pkoutsovasilis pkoutsovasilis deleted the helm/ksm_dependency branch January 17, 2025 01:07
mergify bot pushed a commit that referenced this pull request Jan 17, 2025
* feat: add ksm subchart dependency

* feat: refactor elastic-agent chart to utilise ksm subchart

* feat: add new ksm examples

* fix: typos

* feat: remove charts directory

* fix: reintroduce Chart.lock

(cherry picked from commit edd342a)

# Conflicts:
#	deploy/helm/elastic-agent/Chart.yaml
#	deploy/helm/elastic-agent/examples/eck/rendered/manifest.yaml
#	deploy/helm/elastic-agent/examples/kubernetes-default/rendered/manifest.yaml
#	deploy/helm/elastic-agent/examples/kubernetes-hints-autodiscover/rendered/manifest.yaml
#	deploy/helm/elastic-agent/examples/multiple-integrations/rendered/manifest.yaml
#	deploy/helm/elastic-agent/examples/user-service-account/rendered/manifest.yaml
pkoutsovasilis added a commit that referenced this pull request Jan 17, 2025
#6534)

* [helm] add kube-state-metric subchart dependency (#6495)

* feat: add ksm subchart dependency

* feat: refactor elastic-agent chart to utilise ksm subchart

* feat: add new ksm examples

* fix: typos

* feat: remove charts directory

* fix: reintroduce Chart.lock

(cherry picked from commit edd342a)

# Conflicts:
#	deploy/helm/elastic-agent/Chart.yaml
#	deploy/helm/elastic-agent/examples/eck/rendered/manifest.yaml
#	deploy/helm/elastic-agent/examples/kubernetes-default/rendered/manifest.yaml
#	deploy/helm/elastic-agent/examples/kubernetes-hints-autodiscover/rendered/manifest.yaml
#	deploy/helm/elastic-agent/examples/multiple-integrations/rendered/manifest.yaml
#	deploy/helm/elastic-agent/examples/user-service-account/rendered/manifest.yaml

* fix: update chart.yaml and examples for 8.x

---------

Co-authored-by: Panos Koutsovasilis <panos.koutsovasilis@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify skip-changelog Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants