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

config: add logging option #118

Closed
wants to merge 5 commits into from

Conversation

zyclonite
Copy link
Contributor

support switching between available logging drivers and provide options

@hashicorp-cla
Copy link

hashicorp-cla commented Jun 3, 2021

CLA assistant check
All committers have signed the CLA.

@towe75
Copy link
Collaborator

towe75 commented Jun 4, 2021

Hi @zyclonite. Thank you for your contribution. There is, however, a considerable overlapping with the PR #99. In addition to what you did here it's also streaming the logs back from the host journal into the plugin to expose it via nomad.

If i understand well we would loose the nomad log feature with your PR and journald log driver?

Maybe you can try your setup with a build from #99 to see if it satisfies your needs?
It's still WIP but i am not aware of any bugs aside test race-conditions.

@zyclonite
Copy link
Contributor Author

hi @towe75

by default the k8s-file is used with the path set by nomad, my change just gives you the option to switch to any other available log driver (mimics the docker driver version)
especially important to me is the possibility to set tags for journald logging, this is not supported by the #99 pr yet

i fear with just having the log-driver option, there will be limitations supporting all the features of the podman api

@towe75
Copy link
Collaborator

towe75 commented Jun 4, 2021

@zyclonite : yes, the tags feature is certainly nice. Actually it think that it should be up to the user to decide if he wants "journal only" or "journal plus nomad" logging. Duplicate logging wastes some cpu cycles and bytes after all.

This means that we could combine your PR and #99 and add distinct logdriver options for the two modes (journald with/without nomad streaming).

@drewbailey any opinion from your side? What is the actual docker driver behavior, would i loose nomad logging if i enable journald?

@zyclonite
Copy link
Contributor Author

zyclonite commented Jun 4, 2021

@towe75 sound great, is it possible to run with workflow in the mean time so i can fix potential issues already? i did test it already in the environment but maybe i missed some linting issues

@zyclonite
Copy link
Contributor Author

thank you for running the workflow, strangely i was not able to run make check on linux but on osx it worked now

@drewbailey
Copy link
Contributor

@towe75 I believe the podman driver currently relies solely on reading from the allocation directory. In order to support Nomad alloc log command/api we'll likely need to implement something similar to docklog https://github.com/hashicorp/nomad/tree/main/drivers/docker/docklog and stream logs from the podman API.

I think allowing users to specify which driver they want in the meantime is ok, but we should make sure the limitations are called out.

@towe75
Copy link
Collaborator

towe75 commented Jun 7, 2021

@drewbailey streaming logs is the reason for #99 :-) Please check the readme there, it gives a pro/con overview about logging options. This PR here would add one more variant: log to journal without logging to nomad. Whilst not often needed one might consider it because of io performance.

@zyclonite
Copy link
Contributor Author

or if somebody uses promtail or similar tools to collect all logs by default so there is no need for an additional stream ;)

@towe75
Copy link
Collaborator

towe75 commented Jun 7, 2021

@zyclonite : actually i am using promtail as well. But it's still nice to be able to quickly browse an alloc in the nomad ui and see the latest logs.

I'm playing with the idea for yet another side project related to nomad, logging and loki, see hashicorp/nomad#9211 .

Maybe you want to join me in this adventure ;-)

@zyclonite
Copy link
Contributor Author

@towe75 good joice, i really like it

we built our own orchestration ui and only fallback to the raw nomad ui if everything else breaks
so we are able to manage hundreds of dedicated nomad clusters and that's the reason why i need to tag the logs per job/service and promtail adds instance and cluster tags

your project sounds like a great idea which definitely adds value
for now my plan is to move the whole workload to podman :)

@towe75
Copy link
Collaborator

towe75 commented Jun 9, 2021

@zyclonite i propose to change you PR to use a log_driver="journald_only". My branch adds then later the actual log_driver="journald" which includes nomad streaming.

Is this fine for you or can you find a better value for this option?

@zyclonite
Copy link
Contributor Author

@towe75 in the docker driver there exists the disable_log_collection option (https://www.nomadproject.io/docs/drivers/docker#disable_log_collection)

if we would stick to a similar pattern it would make the migration between these two drivers easier for the user...

@towe75
Copy link
Collaborator

towe75 commented Jun 11, 2021

Ah, nice. I was not aware of this option. That's certainly better. Let's go for it. So you allow only journald in combination with disabled log collection, right? I will implement the other half later in my branch.

@zyclonite
Copy link
Contributor Author

i have not yet touched that option but i would say it makes sense to do it like you suggested

@towe75
Copy link
Collaborator

towe75 commented Jun 20, 2021

@zyclonite please check my commit fb9d52e . I merged your branch into #99 and also added the disable_log_collection driver option. It only needs a bit polishing and more tests.

Maybe you want to give it a try in your environment to see if it does what you want?
We can then close this PR, IMHO.

@zyclonite
Copy link
Contributor Author

zyclonite commented Jun 20, 2021

looks good at first sight, i will try to find some time to test it tomorrow

@zyclonite
Copy link
Contributor Author

@towe75 good news, i did build #99 and it does exactly what i would expect, so feel free to close this one, thank you

@towe75
Copy link
Collaborator

towe75 commented Jun 21, 2021

@zyclonite : nice, thank you for the feedback, discussion and of course the contributed code. Closing

@towe75 towe75 closed this Jun 21, 2021
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.

4 participants