-
Notifications
You must be signed in to change notification settings - Fork 515
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
fix: fixes a regression that requires a log file in multi-tenant mode #2918
fix: fixes a regression that requires a log file in multi-tenant mode #2918
Conversation
Signed-off-by: Akiff Manji <[email protected]>
Signed-off-by: Akiff Manji <[email protected]>
Signed-off-by: Akiff Manji <[email protected]>
I don't see anything wrong with the changes made, but several integration tests are failing related to the logging config. |
@amanji — are you on this? |
@swcurran Yes. Just running local tests |
Signed-off-by: Akiff Manji <[email protected]>
Quality Gate passedIssues Measures |
FYI. The last fail wasn't related to the PR. I've seen it a couple times lately and I'm pretty sure it's related to the von-network nodes getting out of sync. It may help to wait longer for this test. Or a script to occasionally sync up the nodes. |
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.
Honestly, I'm still a bit confused by all this logging files and different configurations. I'm going to approve it though as you have detailed comments and looks like you've got a solid understanding.
It has been just as perplexing to me having gone through the code a few times. There are likely some historical design decisions that I'm not aware of. I do think this can be distilled further down but probably best left for another refactor. |
From an outsider, I can’t figure out why there is any configuration beyond the location of the file. AFAIK, what we need is to add the tenant ID to the log, so that later when the logs are processed, they can be separated. That should be the same whether it is single vs. multi-tenent. Or are we splitting the logs at the source and writing each one to a separate file? Is there a description of the design that to review. I’ve not looked at it, so I could be way off. |
This fixes a regression introduced by #2870 that required a log file to be present on file when using the default config setup for multi-tenant mode. The log file is only required if the user specifically sets the
--log-file
startup param.