-
Notifications
You must be signed in to change notification settings - Fork 717
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
Fix Logstash keystore performance #7642
Fix Logstash keystore performance #7642
Conversation
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.
Works as advertised, just a couple of naming nits
@@ -27,6 +27,8 @@ type InitContainerParameters struct { | |||
KeystoreAddCommand string | |||
// Keystore create command | |||
KeystoreCreateCommand string | |||
// ContainerCommand is the bash script to run in container | |||
ContainerCommand string |
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.
ContainerCommand string | |
CustomScript string |
This may be easier to follow, if we call this CustomScript
, and comment to state that this is a script to run that overrides the default Keystore script?
initContainersParameters = keystore.InitContainerParameters{ | ||
KeystoreCreateCommand: keystoreCommand + " create", | ||
KeystoreAddCommand: keystoreCommand + ` add "$key" < "$filename"`, | ||
KeystoreCreateCommand: "echo 'y' | /usr/share/logstash/bin/logstash-keystore create", |
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.
For consistency, we might want to think about using both KeystoreCreateCommand
and KeystoreAddCommand
here, and in the script, or just calling out to logstash-keystore
directly in the script.
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.
added the command for consistency
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.
LGTM other than small nit about using the same executable path consistently
Co-authored-by: Rob Bavey <rob.bavey@elastic.co>
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.
LGTM
@pebrc thanks for the review. This PR is ready to merge |
@robbavey can you approve to override your previous "change requested"? |
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.
LGTM
Approved |
The commit fixes TestLogstashKeystoreWithoutPassword . The Logstash pipeline gave false positive in #7642. Keystore variable cannot do string comparison in conditional statement test criteria (pipelines.main.plugins.filters.0.events.out == 1) gives flaky result as the filters position are not guaranteed Relates: #7642
Fixes: #7027
Logstash has performance issue when init container creates keystore with a couple of keys.
Since Logstash 8.12.0,
logstash-keystore
command supports adding multiple keys in one operation.This PR uses the command to add keys in a batch. The minimum version of Logstash currently running in ECK is 8.12.0
The steps to create keystore in Logstash
The following resources should start approximately in a minute