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

Fix Logstash crashes with env #7475

Merged
merged 2 commits into from
Jan 25, 2024
Merged

Fix Logstash crashes with env #7475

merged 2 commits into from
Jan 25, 2024

Conversation

kaisecheng
Copy link
Contributor

Fixed: #7450

Logstash crashes when env variable name is in the list of env2yaml
This PR changes the config init container to copy the logstash.yml to config Volume to allow updating the file.

The following resource should start without error

apiVersion: logstash.k8s.elastic.co/v1alpha1
kind: Logstash
metadata:
  name: logstash-sample
spec:
  count: 1
  version: 8.11.1
  podTemplate:
    spec:
      containers:
        - name: logstash
          env:
            - name: "NODE_NAME"
              value: "No_Crash!"

@kaisecheng kaisecheng requested a review from robbavey January 17, 2024 14:36
@thbkrkr thbkrkr added >bug Something isn't working v2.12.0 labels Jan 17, 2024
robbavey
robbavey approved these changes Jan 17, 2024
View reviewed changes
Copy link
Member

@robbavey robbavey left a comment

Choose a reason for hiding this comment

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

Tested - observed previous CrashLoopBackoff, now fixed

LGTM

@kaisecheng
Copy link
Contributor Author

@pebrc @thbkrkr @barkbay This is ready for review

pebrc
pebrc approved these changes Jan 23, 2024
View reviewed changes
pkg/controller/logstash/initcontainer.go
@@ -32,7 +32,7 @@ mount_path=` + volume.InitContainerConfigVolumeMountPath + `

cp -f /usr/share/logstash/config/*.* "$mount_path"

ln -sf ` + volume.InternalConfigVolumeMountPath + `/` + ConfigFileName + ` $mount_path
cp ` + volume.InternalConfigVolumeMountPath + `/` + ConfigFileName + ` $mount_path
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a comment explaining that Logstash rewrites the configuration file in the presence of environment variable and that that's why we have to copy it.

pkg/controller/logstash/initcontainer.go
@@ -32,7 +32,7 @@ mount_path=` + volume.InitContainerConfigVolumeMountPath + `

cp -f /usr/share/logstash/config/*.* "$mount_path"

ln -sf ` + volume.InternalConfigVolumeMountPath + `/` + ConfigFileName + ` $mount_path
cp ` + volume.InternalConfigVolumeMountPath + `/` + ConfigFileName + ` $mount_path
ln -sf ` + volume.InternalPipelineVolumeMountPath + `/` + PipelineFileName + ` $mount_path
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are pipeline files not rewritten?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pipeline files are not rewritten

@kaisecheng
Copy link
Contributor Author

@pebrc This is ready to merge. Thank you.

@pebrc pebrc merged commit 7ab8da0 into elastic:main Jan 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robbavey robbavey robbavey approved these changes

@pebrc pebrc pebrc approved these changes

@thbkrkr thbkrkr Awaiting requested review from thbkrkr

@barkbay barkbay Awaiting requested review from barkbay

Assignees
No one assigned
Labels
>bug Something isn't working :logstash v2.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Logstash crashes with env NODE_NAME
4 participants
@kaisecheng @pebrc @robbavey @thbkrkr