-
Notifications
You must be signed in to change notification settings - Fork 784
Logstash - add ability to reload pipeline(s) without triggering full pod restart #6674
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
Merged
robbavey
merged 13 commits into
elastic:feature/logstash
from
robbavey:reload_pipelines
Apr 21, 2023
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
0111ad9
Copy pipeline and logstash.yml to initContainer
robbavey b5f0ed8
Working
robbavey 8500980
Tidy up
robbavey 603a83e
Lint fixes
robbavey 0c3aeb4
Add e2e tests for pipeline changes
robbavey 505f1fc
Remove unnecessary configHash from pipeline reconciliation
robbavey a1aadae
Remove unused import
robbavey a5d84ce
Merge remote-tracking branch 'upstream/feature/logstash' into reload_…
robbavey 5843ff7
Update tests with pipeline reload functionality
robbavey d09f7fe
Mark pods as updated to speed up discovery and application of pipelin…
robbavey fb157ad
Apply suggestions from code review
robbavey 598fe50
Code review suggestions
robbavey 1884ff9
Apply suggestions from code review
robbavey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| // Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| // or more contributor license agreements. Licensed under the Elastic License 2.0; | ||
| // you may not use this file except in compliance with the Elastic License 2.0. | ||
|
|
||
| package logstash | ||
|
|
||
| import ( | ||
| corev1 "k8s.io/api/core/v1" | ||
| "k8s.io/apimachinery/pkg/api/resource" | ||
|
|
||
| logstashv1alpha1 "github.com/elastic/cloud-on-k8s/v2/pkg/apis/logstash/v1alpha1" | ||
| ) | ||
|
|
||
| const ( | ||
| InitConfigContainerName = "logstash-internal-init-config" | ||
|
|
||
| // InitConfigScript is a small bash script to prepare the logstash configuration directory | ||
| InitConfigScript = `#!/usr/bin/env bash | ||
| set -eu | ||
|
|
||
| init_config_initialized_flag=` + InitContainerConfigVolumeMountPath + `/elastic-internal-init-config.ok | ||
|
|
||
| if [[ -f "${init_config_initialized_flag}" ]]; then | ||
| echo "Logstash configuration already initialized." | ||
| exit 0 | ||
| fi | ||
|
|
||
| echo "Setup Logstash configuration" | ||
|
|
||
| mount_path=` + InitContainerConfigVolumeMountPath + ` | ||
|
|
||
| cp -f /usr/share/logstash/config/*.* "$mount_path" | ||
|
|
||
| ln -sf ` + InternalConfigVolumeMountPath + `/logstash.yml $mount_path | ||
| ln -sf ` + InternalPipelineVolumeMountPath + `/pipelines.yml $mount_path | ||
|
|
||
| touch "${init_config_initialized_flag}" | ||
| echo "Logstash configuration successfully prepared." | ||
| ` | ||
| ) | ||
|
|
||
| // initConfigContainer returns an init container that executes a bash script to prepare the logstash config directory. | ||
| // This copies files from the `config` folder of the docker image, and creates symlinks for the `logstash.yml` and | ||
| // `pipelines.yml` files created by the operator into a shared config folder to be used by the main logstash container. | ||
| // This enables dynamic reloads for `pipelines.yml`. | ||
| func initConfigContainer(ls logstashv1alpha1.Logstash) corev1.Container { | ||
| privileged := false | ||
|
|
||
| return corev1.Container{ | ||
| // Image will be inherited from pod template defaults | ||
| ImagePullPolicy: corev1.PullIfNotPresent, | ||
| Name: InitConfigContainerName, | ||
| SecurityContext: &corev1.SecurityContext{ | ||
| Privileged: &privileged, | ||
| }, | ||
| Command: []string{"/usr/bin/env", "bash", "-c", InitConfigScript}, | ||
| VolumeMounts: []corev1.VolumeMount{ | ||
| ConfigSharedVolume.InitContainerVolumeMount(), | ||
| ConfigVolume(ls).VolumeMount(), | ||
| PipelineVolume(ls).VolumeMount(), | ||
| }, | ||
|
|
||
| Resources: corev1.ResourceRequirements{ | ||
| Requests: map[corev1.ResourceName]resource.Quantity{ | ||
| corev1.ResourceMemory: resource.MustParse("50Mi"), | ||
| corev1.ResourceCPU: resource.MustParse("0.1"), | ||
| }, | ||
| Limits: map[corev1.ResourceName]resource.Quantity{ | ||
| // Memory limit should be at least 12582912 when running with CRI-O | ||
| corev1.ResourceMemory: resource.MustParse("50Mi"), | ||
| corev1.ResourceCPU: resource.MustParse("0.1"), | ||
| }, | ||
| }, | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,19 +5,20 @@ | |
| package logstash | ||
|
|
||
| import ( | ||
| "hash" | ||
|
|
||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
|
|
||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
|
|
||
| logstashv1alpha1 "github.com/elastic/cloud-on-k8s/v2/pkg/apis/logstash/v1alpha1" | ||
| "github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/annotation" | ||
| "github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/labels" | ||
| "github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/reconciler" | ||
| "github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/tracing" | ||
| "github.com/elastic/cloud-on-k8s/v2/pkg/controller/logstash/pipelines" | ||
| ) | ||
|
|
||
| func reconcilePipeline(params Params, configHash hash.Hash) error { | ||
| func reconcilePipeline(params Params) error { | ||
| defer tracing.Span(¶ms.Context)() | ||
|
|
||
| cfgBytes, err := buildPipeline(params) | ||
|
|
@@ -36,12 +37,16 @@ func reconcilePipeline(params Params, configHash hash.Hash) error { | |
| }, | ||
| } | ||
|
|
||
| if _, err = reconciler.ReconcileSecret(params.Context, params.Client, expected, ¶ms.Logstash); err != nil { | ||
| if _, err := reconciler.ReconcileSecret(params.Context, params.Client, expected, ¶ms.Logstash, | ||
| reconciler.WithPostUpdate(func() { | ||
| annotation.MarkPodsAsUpdated(params.Context, params.Client, | ||
| client.InNamespace(params.Logstash.Namespace), | ||
| NewLabelSelectorForLogstash(params.Logstash), | ||
| ) | ||
| }), | ||
| ); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| _, _ = configHash.Write(cfgBytes) | ||
|
|
||
| return nil | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. diff --git a/pkg/controller/common/reconciler/secret.go b/pkg/controller/common/reconciler/secret.go
index 0b6026f87..50004fd80 100644
--- a/pkg/controller/common/reconciler/secret.go
+++ b/pkg/controller/common/reconciler/secret.go
@@ -30,11 +30,17 @@ const (
SoftOwnerKindLabel = "eck.k8s.elastic.co/owner-kind"
)
+func WithPostUpdate(f func()) func(p *Params) {
+ return func(p *Params) {
+ p.PostUpdate = f
+ }
+}
+
// ReconcileSecret creates or updates the actual secret to match the expected one.
// Existing annotations or labels that are not expected are preserved.
-func ReconcileSecret(ctx context.Context, c k8s.Client, expected corev1.Secret, owner client.Object) (corev1.Secret, error) {
+func ReconcileSecret(ctx context.Context, c k8s.Client, expected corev1.Secret, owner client.Object, opts ...func(*Params)) (corev1.Secret, error) {
var reconciled corev1.Secret
- if err := ReconcileResource(Params{
+ params := Params{
Context: ctx,
Client: c,
Owner: owner,
@@ -54,7 +60,11 @@ func ReconcileSecret(ctx context.Context, c k8s.Client, expected corev1.Secret,
reconciled.Annotations = maps.Merge(reconciled.Annotations, expected.Annotations)
reconciled.Data = expected.Data
},
- }); err != nil {
+ }
+ for _, opt := range opts {
+ opt(¶ms)
+ }
+ if err := ReconcileResource(params); err != nil {
return corev1.Secret{}, err
}
return reconciled, nil
diff --git a/pkg/controller/logstash/pipeline.go b/pkg/controller/logstash/pipeline.go
index 6cbfee388..447ed7b8b 100644
--- a/pkg/controller/logstash/pipeline.go
+++ b/pkg/controller/logstash/pipeline.go
@@ -41,7 +41,13 @@ func reconcilePipeline(params Params) error {
},
}
- if err := reconcileSecretWithFastUpdate(params, expected); err != nil {
+ if _, err := reconciler.ReconcileSecret(params.Context, params.Client, expected, ¶ms.Logstash,
+ reconciler.WithPostUpdate(func() {
+ annotation.MarkPodsAsUpdated(params.Context, params.Client,
+ client.InNamespace(params.Logstash.Namespace),
+ NewLabelSelectorForLogstash(params.Logstash),
+ )
+ })); err != nil {
return err
}
return nilIf we want to reuse the existing secret reconciliation we could add a slice of option functions at the end |
||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we pass the
configHashwhenconfig.reload.automaticequals false?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good question. I'm erring on the side of 'no' at the moment, but I think this is something that could change after the technical preview depending on feedback.
My reasoning on this is that the
false(default) value of non-k8s logstash doesn't react to pipeline changes at all, and to change this semantic to restart logstash completely on pipeline changes feels like very different behaviour.Thinking about how we could add flexibility, I wonder if we might want to introduce something for ECK here, along the lines of:
config.reload.restart_policy: detected_only|all|none, which would either setconfig.reload.automatic: truefordetected_only, andfalseforallornone, passing theconfigHashif the value isall, and not if it isnone.cc @flexitrev, @roaksoax, @jsvd