-
Notifications
You must be signed in to change notification settings - Fork 44
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
remove --verbose from rock entrypoint #495
remove --verbose from rock entrypoint #495
Conversation
@cjdcordeiro This should be ready for review. |
@linostar @cjdcordeiro there are many spread failures due to this change that need to be addressed. In particular, a concerning kind of failure is that many tests check docker logs and now the output of the services isn't going to the log at all, apparently (e.g. this check is failing). I think this is a breaking change to pull on the users - the logs for their rocks will suddenly be empty and they won't know until they check them (presumably because something broke and they need to check the logs) |
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.
You'll need to fix the spread tests that expect logs to be coming out by default. I suggest you fix them in a way that 1) in some cases you explicitly pass the --verbose
to then assert the output, and 2) in other cases there's simply no service output
Not necessarily a breaking change, but indeed something that is easy to miss. @linostar is giving people a heads-up on Discourse and later on in Matrix The TBH, this was an oversight because:
|
b2bf3aa
to
79caab3
Compare
I might very well be mistaken as I'm not too familiar with the internals of pebble and the integration with juju, and in that case, please enlighten me, but if I'm not completely mistaken this change potentially breaks the entire charm log flow, as logs no longer will be picked up in I'm also interested in knowing whether this change affects the pebble log forwarding feature at all or whether that will remain unaffected by the change prior to this change landing in main. |
thanks @simskij . that's important. let me add @tonyandrewmeyer and @benhoyt here, since this is also related to Pebble and the operator framework. @benhoyt, in short, the idea is that rocks would have pebble run without the --verbose from now on. Seeing @simskij's message above, do you see any way a charmer could enable the |
This is definitely important, however ... when Juju starts the workload containers it starts pebble with these args (code here):
So In addition, does This change also does not affect Pebble's log forwarding feature at all. That's configured in the Pebble config layer and is internal to Pebble. So I think we're good here as far as charming goes. Unless I'm missing something, it seems to me this would only apply when you're running a Rock via docker at the command line. That said, I think it's worth testing end-to-end with Juju and a Rock built with this non-verbose |
Tyvm @benhoyt ! That's useful. @linostar will take this opportunity to get started with Juju and charms, and give it a try. But @simskij , it would be paramount to have your appraisal, since you already have proper use cases. On your side, I think the test could be quite simple. Just take an existing rock, and do echo '''
FROM <your-rock>
ENTRYPOINT ["pebble", "enter"]
''' | docker build -t <your-quiet-rock> - Then use this new rock in your charm and assess the impact. |
5bd2e84
to
e29459f
Compare
e29459f
to
d094e94
Compare
@benhoyt hello charm debug logs
hola charm debug logs
So basically, |
4f4f25c
to
8c7b5bb
Compare
@tigarmo @cjdcordeiro Ready for review. |
3a0e5d3
to
16e6e71
Compare
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 thanks
16e6e71
to
27e5257
Compare
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 a test is still failing. pls re-request review once it passes
The error in question is unrelated and is due to fedora 37 not available for workers any more. I'm trying to fix that in PR #580. |
All related spread tests seem to pass, but some tests are being killed to timeout due to some slowness in spread workers (the exception is the fedora worker addressed in PR #580). Even when after re-running the spread tests several times, different tests fail every time. |
7fb0a41
to
2043e90
Compare
2043e90
to
2ac7088
Compare
14d0add
to
0a255d8
Compare
@tigarmo can you please merge this before something else get merged in main and cause a new conflict? |
Hello. I was wondering why some rocks randomly had service logs and other did not. It seems that the difference was the rockcraft version being used, and I eventually found the reason: this PR. This is a bit late at this point, but I'd like the We are using I see a lot of comments here are related to I am willing to work on adding that option to Best regards, Claudiu |
Hi Claudiu, thank you for the detailed feedback and for sharing your use case. This was indeed an intentional change, that despite its relation to Juju, was meant to fix what we believed to be an undesired default behavior -> being too verbose. While it might sound obvious that one always wants their service to print out its logs to STDOUT by default, in the case of a rock we must consider other factors:
The goal here is that one can always add In the near future, we might be considering some new features that may be useful for specific logging cases, like Log Forwarding and the ability to add more control, at build time, to what the OCI cmd should be. In the meantime, adding |
This was triggered by the existing pebble bug at canonical/pebble#339.
Action to be taken in Rockcraft:
--verbose
from the rock’s entrypoint. That should always have been the default behavior, also because pebble doesn’t have a--quiet
option. Those who want verbose mode, can always still pass --verbose at container deployment time.