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

[bug] pebble "enter" keeps logging, with unexpected indentation, when combined with "exec" #339

Closed
cjdcordeiro opened this issue Dec 7, 2023 · 9 comments · Fixed by #394
Assignees
Labels
Bug An undesired feature ;-)

Comments

@cjdcordeiro
Copy link
Collaborator

Description

When using pebble enter (which is the case for all rocks), Pebble will keep logging to STDOUT, regardless of the provided subcommand.

Not only that, but it appears that there's also a flushing problem, as the logs come out with a weird indentation.

Reproduce

  1. create a noisy check
summary: test
description: test

checks:
  foo:
    override: replace
    period: 2s
    exec:
      command: fail
  1. run pebble enter --verbose exec -it bash

Here's the problem:

cris@cris-laptop:~ $ pebble enter --verbose exec -it bash
2023-12-07T15:22:32.934Z [pebble] Started daemon.
2023-12-07T15:22:32.939Z [pebble] POST /v1/exec 4.734384ms 202
                                                              2023-12-07T15:22:32.943Z [pebble] GET /v1/tasks/36/websocket/control 3.656673ms 200
                                                                                                                                                 2023-12-07T15:22:32.943Z [pebble] GET /v1/tasks/36/websocket/stdio 29.98µs 200
                                                                                                                                                                                                                               cris@cris-laptop:~ $ 2023-12-07T15:22:34.935Z [pebble] Check "foo" failure 1 (threshold 3): exec: "fail": executable file not found in $PATH
                                                                                 2023-12-07T15:22:36.935Z [pebble] Check "foo" failure 2 (threshold 3): exec: "fail": executable file not found in $PATH

cris@cris-laptop:~ $ 
cris@cris-laptop:~ $ 
cris@cris-laptop:~ $ 
cris@cris-laptop:~ $ 2023-12-07T15:22:38.935Z [pebble] Check "foo" failure 3 (threshold 3): exec: "fail": executable file not found in $PATH
                                                                                                                                            2023-12-07T15:22:38.935Z [pebble] Check "foo" failure threshold 3 hit, triggering action

cris@cris-laptop:~ $ 
cris@cris-laptop:~ $ 
cris@cris-laptop:~ $ date
Thu Dec  7 04:22:40 PM CET 2023
cris@cris-laptop:~ $ 2023-12-07T15:22:40.936Z [pebble] Check "foo" failure 4 (threshold 3): exec: "fail": executable file not found in $PATH
                                                                                                                                            2023-12-07T15:22:42.935Z [pebble] Check "foo" failure 5 (threshold 3): exec: "fail": executable file not found in $PATH
                                                                                                                                                                                                                                                                   ^C
cris@cris-laptop:~ $ 2023-12-07T15:22:44.936Z [pebble] Check "foo" failure 6 (threshold 3): exec: "fail": executable file not found in $PATH

exit
2023-12-07T15:22:45.277Z [pebble] GET /v1/changes/36/wait 12.333227355s 200
                                                                           cris@cris-laptop:~ $ 

Proposal

Do not print logs when pebble enter exec (especially if exec is running in interactive mode)

@cjdcordeiro cjdcordeiro added the Bug An undesired feature ;-) label Dec 7, 2023
@cjdcordeiro
Copy link
Collaborator Author

(fyi @rebornplusplus )

@benhoyt
Copy link
Contributor

benhoyt commented Dec 7, 2023

The indentation thing is interesting. It has something to do with how the terminal device driver automatically injects CR before LF when output is written (and we're using a PTY / pseudo-terminal here). I haven't figured out the details yet, but see more at https://unix.stackexchange.com/a/151963

Not also that logs go to stderr, not stdout (I'll change that in the issue description).

I'm not sure just disabling logs is what we want here, or what the fix for the CR/LF issue is. Do you have time to investigate further? I think there's enough subtlety here that it's worth writing a one-pager spec on your findings and a proposed fix so we can discuss further.

@cjdcordeiro
Copy link
Collaborator Author

thanks for the stderr fix.

I think disabling the logs is the expected UX. Especially if the user is interacting with the exec command. the CR/LF fix could be acceptable if one is doing docker run <img> exec <cmd>. However, with docker run -it <img> --run exec -it <cmd> I'd like not to have the underlying logs pollute the interactive terminal.

rebornplusplus added a commit to rebornplusplus/pebble that referenced this issue Jan 29, 2024
This commit disables logs completely for ``pebble enter --verbose exec
-it <cmd..>`` even if the --verbose flag is passed.

Interactive applications, like Vim, can be run as an exec command:

    pebble enter --verbose exec -it vim

Since enter runs the pebble daemon and executes the exec command
afterwards, the same TTY that will be made RAW by exec to run
interactive applications like Vim is also used for logging and service
outputs. Thus, the logs coming from pebble find the TTY RAW and a weird
indentation is introduced. [1]

The issue however is not limited to only indentation. For example, if a
text editor was being run with exec -it, like Vim, the incoming logs
would go to the text editor and may change the content of the editor’s
buffer.

This commit disables logs completely for ``pebble enter --verbose exec
-it`` even if the --verbose flag is passed. Since logs are currently
disabled for ``pebble enter exec`` if --verbose flag is not passed and
-i, -t are assumed if stdin and stdout are TTYs, the commit only ignores
the --verbose flag in cases where exec will make the TTY RAW.

References:
- [1] canonical#339
@benhoyt
Copy link
Contributor

benhoyt commented Mar 13, 2024

Closing this issue, as this is being addressed by removing --verbose as the default in Rock entrypoints: canonical/rockcraft#495

@benhoyt benhoyt closed this as not planned Won't fix, can't repro, duplicate, stale Mar 13, 2024
@cjdcordeiro
Copy link
Collaborator Author

@benhoyt this is still gonna be addressed as canonical/rockcraft#495 just makes this issue harder to notice, but it is still there. The idea is to completely disallow --verbose to be used with enter exec

@benhoyt
Copy link
Contributor

benhoyt commented Mar 13, 2024

Got it -- in that case, re-opening.

@benhoyt benhoyt reopened this Mar 13, 2024
rebornplusplus added a commit to rebornplusplus/pebble that referenced this issue Mar 21, 2024
This commit blocks the usage of ``--verbose`` option in "enter" command
whenever the "exec" subcommand follows. Thus, the following will result
in errors:

    pebble enter --verbose [enter-OPTS*] exec [exec-OPTS] <cmd..>

Everything else, however, are kept unchanged. So the following is okay:

    pebble enter [enter-OPTS*] exec [exec-OPTS] <cmd..>

Note that enter-OPTS* refers to all "enter" options except -v,
--verbose.

Originated from canonical#339.
@benhoyt
Copy link
Contributor

benhoyt commented May 17, 2024

Closing this Pebble issue as it's not really a Pebble problem as such, and will be addressed when canonical/rockcraft#495 is merged.

@benhoyt benhoyt closed this as not planned Won't fix, can't repro, duplicate, stale May 17, 2024
@cjdcordeiro
Copy link
Collaborator Author

You should re-open @benhoyt , given the reason mentioned above #339 (comment)

@benhoyt
Copy link
Contributor

benhoyt commented May 23, 2024

Oops, re-opening until #394 is merged.

@benhoyt benhoyt reopened this May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug An undesired feature ;-)
Projects
None yet
3 participants