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

console: io backup restore #525

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

HofiOne
Copy link
Contributor

@HofiOne HofiOne commented Feb 21, 2025

console: add support for restoring the original console io handlers after an attached control client has finished its work

added optional ATTACH arguments to be able to control this better

  • --forever: after syslog-ng-ctl attach logs\stdio exits it will not redirect stdio/logs to syslog-ng
  • --dont-release: after syslog-ng-ctl attach logs\stdio exits it will not redirect stdio/logs to syslog-ng and will not release the stolen console so that all syslog-ng output will be continuously presented in the console syslog-ng-ct was invoked from

Signed-off-by: Hofi [email protected]

@HofiOne HofiOne changed the title Console io backup restore console: io backup restore Feb 21, 2025
@HofiOne HofiOne marked this pull request as draft February 22, 2025 09:40
@HofiOne HofiOne marked this pull request as ready for review February 22, 2025 11:22
@HofiOne HofiOne force-pushed the console-io-backup-restore branch from f096a01 to f9d4706 Compare February 22, 2025 11:41
{ "log-level", 0, 0, G_OPTION_ARG_CALLBACK, _store_log_level, "change syslog-ng log level", "<default|verbose|debug|trace>" },
{ "forever", 'f', 0, G_OPTION_ARG_NONE, &attach_options_forever, "do not pass logs/stdio back to syslog-ng after syslog-ng-ctl is exited", NULL },
{ "dont-release", 'd', 0, G_OPTION_ARG_NONE, &attach_options_dont_release_console, "do not pass logs/stdio back to syslog-ng after syslog-ng-ctl is exited and keep logging onto the calling console" },
Copy link
Member

Choose a reason for hiding this comment

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

when would we use these options? I'd not add them if possible. I do think that restoring the original console makes a lot of sense though.

One mode which we would need is where syslog-ng-ctl is actively reading the fds and writes them to its own stdout/stderr, so that we can grep the output.

Right now, the console redirection works but it cannot be filtered by grep, even if added to the end of the syslog-ng-ctl command line using a pipe.

e.g. this does not work: "syslog-ng-ctl attach logs | grep whatever"

All the logs will be dumped to stdout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just simply wanted to keep your original implementation at the same time (I just forgot to apply the --forever flag in the final version in mainloop-control)

Not restoring the original fds (--forever option) in syslog-ng might make sense sometimes, e.g. if the ctl caller wants to redirect the output fully, but wants to interrupt the local output sometimes then continues.

The --dont-release might make sense too if the ctl caller wants to exit immediately but keep the log flow to the current terminal (that also will free the syslog-ng _wait_until_peer_disappears thread nicely)

I think the piping/grep issue is independent of this change, it is an issue with the original implementation, but yes, it must be fixed, which I'll look at.

Also, I'd like to add a new option to control which fds the caller wants to attach to.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Agreed, that the piping/grep issue is independent, I was just mentioning it, as it was something I wanted to do at the original implementation but never got around to do it.

  2. the primary purpose of attaching to the stdio fds came as an idea as I wanted to attach to the "debugger" portion of syslog-ng. The logs command only came as an extra idea, as it is usually very difficult to get the internal logs if syslog-ng is running in the background and it was simple to add.

  3. I don't think redirecting the console and keeping it there for a longer period is an important use-case, that's why I was asking what was the intention, I might be missing a useful thing there.

Copy link
Contributor Author

@HofiOne HofiOne Feb 24, 2025

Choose a reason for hiding this comment

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

Right now, the console redirection works but it cannot be filtered by grep, even if added to the end of the syslog-ng-ctl command line using a pipe.

e.g. this does not work: "syslog-ng-ctl attach logs | grep whatever"

All the logs will be dumped to stdout.

This is related to my previous question in the global comments.

The actual behavior you described is normal, we redirect all the fds to their matching handles on the caller ctl console, so, there is no message on the stdout currently, syslog-ng logs the log_stderr (attach logs) onto the stderr as expected.

Please describe the required redirections for each of the attach targets as I questioned bellow to be able to fix these.

Thanks.

Copy link
Contributor Author

@HofiOne HofiOne Feb 24, 2025

Choose a reason for hiding this comment

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

  1. Agreed, that the piping/grep issue is independent, I was just mentioning it, as it was something I wanted to do at the original implementation but never got around to do it.

This is now normal as I described above, but can be handled easily like

syslog-ng-ctl attach logs --seconds 500 2>&1 | grep cURL

I do not think I will try to fix this as it would require an even more complex logic in the redirection (e.g. in case attach logs redirecting stderr to the caller's stdin instead of the matching pair)

@HofiOne HofiOne force-pushed the console-io-backup-restore branch from f9d4706 to ece79b0 Compare February 24, 2025 09:58
@HofiOne HofiOne marked this pull request as draft February 24, 2025 10:49
@HofiOne
Copy link
Contributor Author

HofiOne commented Feb 24, 2025

@bazsi the _parse_attach_command_args shows that the intended behavior is to handle only one of the attach target options in a given call, either STDIO, LOGS, or the DEBUGGER.
But the logic in the control_connection_attach will always steel multiple variations, e.g. if only LOGS is the target it will steel the STDIO as well automatically currently.

What are the intended, valid combinations that we would like to support here?

My bets are

STDIO option -> only STDIO
LOGS option -> only LOGS
DEBUGGER option -> only DEBUGGER (or STDIO too to get everything in the caller's console?!?)

Could you please confirm?

@bazsi
Copy link
Member

bazsi commented Feb 24, 2025

@bazsi the _parse_attach_command_args shows that the intended behavior is to handle only one of the attach target options in a given call, either STDIO, LOGS, or the DEBUGGER. But the logic in the control_connection_attach will always steel multiple variations, e.g. if only LOGS is the target it will steel the STDIO as well automatically currently.

I think we only need to handle one case at a time. And stdio I think we can easily remove. That was the first thing I implemented, when I realized I needed to set the log levels as well. So LOGS and DEBUGGER and both should run only as long as the ctl program is running.

We should restore to the original console (as you implemented) when we finish with any of these commands, so the next one can properly run.

What are the intended, valid combinations that we would like to support here?

My bets are

STDIO option -> only STDIO LOGS option -> only LOGS DEBUGGER option -> only DEBUGGER (or STDIO too to get everything in the caller's console?!?)

Could you please confirm?

I hope I addressed these with the response above. Let me know if it didn't or ping me on discord/reddit to chat.

@HofiOne HofiOne force-pushed the console-io-backup-restore branch from ece79b0 to 2dc8887 Compare February 25, 2025 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants