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.

Sign up for GitHub

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

Jump to bottom

Refactor stack monitoring templating #8327

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Refactor stack monitoring templating #8327

wants to merge 10 commits into from
+3,071 −517

Conversation

pebrc
Copy link
Collaborator

@pebrc pebrc commented Dec 12, 2024 •
edited
Loading

Fixes #8314

This PR introduces the following changes:

  • separate template rendering from the shared stack monitoring code as we have have introduced a few application specific config settings in the templates and Beat was not using the shared rendering to begin with
  • test the actual templates used in production code instead of a synthetic template in the shared code
  • modify existing tests to use snapshots instead of counting volumes/volume mounts (more verbose but easier to verify in my opinion, better diffs on test failures)

Not addressed:

  • stackmon.Metricbeat|Filebeat are called twice: once for generating the config secret and once to generate the side cars (duplicated work)

@botelastic botelastic bot added the triage label Dec 12, 2024
@botelastic botelastic bot removed the triage label Dec 12, 2024
@pebrc
Copy link
Collaborator Author

pebrc commented Dec 12, 2024

buildkite test this -f p=gke,t=Test.*StackMonitoring -m s=8.16.0,s=7.17.25,s=8.15.4

@pebrc pebrc marked this pull request as ready for review December 18, 2024 15:25
barkbay
barkbay reviewed Dec 19, 2024
View reviewed changes
pkg/controller/elasticsearch/stackmon/sidecar.go Outdated
return stackmon.BeatSidecar{}, err
}

caVolume, err := stackmon.CAVolume(client, k8s.ExtractNamespacedName(&es), esv1.ESNamer, commonv1.KbMonitoringAssociationType /*???*/, es.Spec.HTTP.TLS.Enabled())
Copy link
Contributor

Choose a reason for hiding this comment

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

I was just doing a first read of your PR and noticed the /*???*/, I agree that KbMonitoringAssociationType feels a bit odd here but did not dig enough to be sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wanted to investigate this myself too and forgot about it. It just has an effect on the name of the volume mount. Changing it would mean a restart for the cluster. But maybe we should do it nonetheless

@pebrc
Copy link
Collaborator Author

pebrc commented Dec 19, 2024

buildkite test this -f p=gke,t=Test.*StackMonitoring -m s=8.17.0,s=7.17.25,s=8.15.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@barkbay barkbay barkbay left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
>refactoring v3.0.0 (next)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make the stack monitoring templating more modular
2 participants
@pebrc @barkbay