-
Notifications
You must be signed in to change notification settings - Fork 778
Add logs volume for file/metricbeat when stack monitoring is enabled with readOnlyRootFilesystem. #8606
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
Add logs volume for file/metricbeat when stack monitoring is enabled with readOnlyRootFilesystem. #8606
Conversation
Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
pebrc
left a comment
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.
Change looks good. I wonder if we should do something for Beats and Logstash as well. We do not configure a security context on those but if a user does we should be able to detect that and configure the additional volumes for stack monitoring. Wdyt?
…the podseccontext Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
Ok, this has been added for logstash as well, if any containers are found within the pod are found to have readOnlyRootFilesystem set. |
barkbay
left a comment
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.
I wonder if we should do something for Beats [...] as well.
For logs I just realized that in the case of Beats both the main container and the side car share the same /usr/share/filebeat/logs:
apiVersion: v1
kind: Pod
metadata:
name: filebeat-beat-filebeat-8jxr2
spec:
containers:
- name: filebeat
volumeMounts:
- mountPath: /usr/share/filebeat/logs ## same mount point
name: filebeat-logs ## same volume
- name: logs-monitoring-sidecar
volumeMounts:
- mountPath: /usr/share/filebeat/logs ## same mount point
name: filebeat-logs ## same volume
volumes:
- emptyDir: {}
name: filebeat-logsI don't known if it is expected that we are mixing the logs from these 2 processes in a same directory? (maybe it's fine, but it's a bit confusing)
For metrics we may need a volume for /usr/share/metricbeat/logs if a user or webhook set readOnlyRootFilesystem: true:
daemonSet:
podTemplate:
spec:
containers:
- name: metrics-monitoring-sidecar
securityContext:
runAsUser: 0
readOnlyRootFilesystem: true⬇️
root@gke-barkbay-dev-default-pool-68a11dd1-dtjf:/usr/share/metricbeat/logs# touch foo
touch: cannot touch 'foo': Read-only file system
I was wondering if we should not include these volumes by default?
| WithPreStopHook(*NewPreStopHook()) | ||
|
|
||
| builder, err = stackmon.WithMonitoring(ctx, client, builder, es) | ||
| builder, err = stackmon.WithMonitoring(ctx, client, builder, es, enableReadOnlyRootFilesystem) |
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.
nit: The way we decide to include those empty volumes is not the same between Elasticsearch and Logstash. For Logstash we try to infer that decision using isReadOnlyRootFilesystem(...) on the pod spec. While for Elasticsearch we are only using our custom logic, however enableReadOnlyRootFilesystem does not mean that the file system is not going to be "read-only". Maybe this has been enforced by the user in the ES pod template.
Should we be consistent and use isReadOnlyRootFilesystem(...) in all the controllers? (or just add the empty volumes by default as suggested in my previous comment)
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.
or just add the empty volumes by default as suggested in my previous comment
+1 to that proposal. It removes all the conditional logic and makes the code simpler and it is never a good idea to write into a containers filesystem no matter what the security settings are.
Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
I have created #8622 to address the duplicate volume mount issue. |
|
@naemono I think you are still missing the volume for metricbeat logs in the case of beats monitoring. |
Fix unit tests. Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
This has been resolved. Thanks for noting this. |
|
buildkite test this -f t=Test.*StackMonitoring -m p=gke,p=ocp |
Add constants for file/metricbeat volumes Signed-off-by: Michael Montgomery <mmontg1@gmail.com>
pebrc
left a comment
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
|
This was not labelled with the target release and is missing from the release notes. Also this triggeres a restart of the affected pods which we also do not document at the moment. |
resolves #8605
When
readOnlyRootFilesystemis enabled for Elasticsearch or Kibana and stack monitoring is enabled both filebeat and metricbeat need anemptyDirvolume to write logs otherwise they receive an error when attempting to write any logs to their logs directory as can happen when indexing errors occur, and a new event file is attempted to be written.