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.
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
Refactor use of system.hostfs to fix cgroup metrics #24334
Refactor use of system.hostfs to fix cgroup metrics #24334
Changes from all commits
b9d4097
db6c690
73d40bf
8b27483
3926d66
2ec54cd
bb574b2
624848a
b64eccd
e5348af
b9f10fc
aa5d38d
478c696
2a62f92
d3aacd6
fd7d246
c9acdb9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this always safe? By introducing
Hostfs
to thePath
struct we will allow users to overwrite the path in the configuration file already.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.
Maybe I'm missing something, but is that any different from
-E
flags?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.
Paths are configured twice I think. Once via
--path.X
on startup +--system.hostfs
.Then the config file is read and the path settings from the configuration file are applied again. The
-E
flags for path settings only become effective during the second phase. That is why the shim bash scripts just use--path.X
CLI flags and we do not attempt to overwrite the defaults in the config.The thing I was wondering: What if
path.hostfs
is configured, butsystem.hostfs
is not? In that case you will setpath.hostfs
to an empty string here. Is this on purpose? Why not:If the forced overwrite is on purpose a comment explaining the why would be helpful.
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.
Okay, so, technically speaking, there is no
path.hostfs
. There's onlysystem.hostfs
, and we just expand the mainPath
struct type withhostfs
, hence why that's being overwritten. Ideally it would only bepath.hostfs
, but I'm mostly worried about breaking changes here, since I'm considering this to be primarily a bugfix so we can properly sethostfs
in the cgroup self-monitoring.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 see. It is still kinda "obscure" and I'm sure someone (me?, future you?) might change the logic on purpose or by accident. Can you please add a code comment about the why? Having the why in the PR review it will be lost.
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.
Ah, good point.
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.
If this setting is used for other beats too, I think we could add it as
path.hostfs
for the config file setting, as other beats don't have asystem
module. But we can leave it for a future change.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.
@fearful-symmetry Is this the correct example? It seems to be a duplicate of the
logs
example one section up.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.
oops!
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.
Wdyt about calling it
path.hostfs
now that it is another configurable path, and not only used by the system module?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.
Yah, it might be worth it to have a second one for backwards compatibility?
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.
Yep, we would keep both at least till 8.0.