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.

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

Handle SD_JOURNAL_INVALIDATE when following a journal #36064

Closed
wants to merge 1 commit into from

Conversation

nalind
Copy link
Contributor

@nalind nalind commented Jan 19, 2018

- What I did
This is taking a stab at fixing the open-file-handles-for-deleted-journal-files parts of #27343.

- How I did it
We weren't handling SD_JOURNAL_INVALIDATE results coming back from sd_journal_process() before, so I added logic to close and reopen the journal handle when we get that result.

- How to verify it
Run docker logs -f to tail the journaled output from a container that was started with --log-driver journald, and consult the list of open descriptors in the directory under /proc/PID/fd which corresponds to the docker process to see which journal files it has open.
While docker logs -f is running, use journalctl --vacuum-size or journalctl --vacuum-files to force the removal of one or more older journal files which dockerd had open, then consult its fd directory again: it should no longer have open descriptors for files which were removed (i.e., the kernel should not be noting any journal files which it has open as (deleted)).

- Description for the changelog

When following a journal in the journald log reader, correctly handle SD_JOURNAL_INVALIDATE results from sd_journal_process() by closing and reopening the journal handle, which closes all open journal files (including deleted ones) and opening all currently-available journal files.

When following a journal in the journald log reader, correctly handle
SD_JOURNAL_INVALIDATE results from sd_journal_process() by closing and
reopening the journal handle, which closes all open journal files
(including deleted ones) and opening all currently-available journal
files.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
@cpuguy83
Copy link
Member

Looking through the code, it seems this will terminate the follow regardless of the container exit or user request in this case (which it already did anyway, so nothing new). This doesn't seem quite like the correct behavior.

@nalind
Copy link
Contributor Author

nalind commented Jan 19, 2018

@cpuguy83 I think that's the behavior we get with json-file as well.

@cpuguy83
Copy link
Member

The json logger gets rotation events and re-opens the log.

@nalind
Copy link
Contributor Author

nalind commented Jan 19, 2018

@cpuguy83 Right. I think I'm misunderstanding what you're saying here - the reader should be continuing to follow the journal after it handles the SD_JOURNAL_INVALIDATE result.

@cpuguy83
Copy link
Member

Oh you're right, I guess I need to step away for a bit and come back to it.

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@boaz0 boaz0 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@portante
Copy link

FWIW, this is an example C program for what it takes to tail the journal without duplicates and not leaking file handles on journald rotation: https://gist.github.com/portante/ff7fb429c6f973aab377f7bb77b0ffdb

@nalind
Copy link
Contributor Author

nalind commented Jan 25, 2018

Hmm, further testing is turning up that we need to re-seek to the point where we were reading when we reopen the journal, and I'm seeing some blocking when writing messages to the logWatcher.Msg channel if a following client disconnects while we're doing a long read, so I think this needs some more work.

@nalind nalind closed this Feb 7, 2018
@nalind nalind deleted the journal-invalidate branch February 7, 2018 22:21
@nalind nalind restored the journal-invalidate branch February 7, 2018 22:21
@nalind nalind deleted the journal-invalidate branch February 7, 2018 22:22
@nalind
Copy link
Contributor Author

nalind commented Feb 7, 2018

I'm going to open a fresh PR for an updated patch, since reworking it to make more frequent calls to sd_journal_process() ends up undoing what we were proposing to change here.

@nalind
Copy link
Contributor Author

nalind commented Feb 8, 2018

Opened #36254 to try to solve this properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants