Skip to content

THREESCALE-14619: Fix the default value for CONFIG_LOG_PATH #462

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.

Sign up for GitHub

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

Open
mayorova wants to merge 1 commit into
base: master
Choose a base branch
from
Open

THREESCALE-14619: Fix the default value for CONFIG_LOG_PATH #462

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion openshift/backend-cron
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ DEFAULT_DELETE_STATS_FREQ = 60*60*24 # 1 day
# The logger used in the delete stats code defaults to /tmp/backend_logger.log
# When running this executable STDOUT is more convenient to check that
# everything is working correctly.
ENV['CONFIG_LOG_PATH'] ||= 'dev/stdout'
ENV['CONFIG_LOG_PATH'] ||= '/dev/stdout'
Copy link
Copy Markdown
Contributor

@akostadinov akostadinov Apr 22, 2026

Choose a reason for hiding this comment

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

what will happen if we don't set this variable? Will it log to stdout by default? It is worth trying because this is how https://redhat.atlassian.net/browse/THREESCALE-12057 was fixed.

Copy link
Copy Markdown
Contributor Author

@mayorova mayorova Apr 22, 2026

Choose a reason for hiding this comment

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

Well, this is what the PR fixes - if the variable is NOT set, previously, the error from the description is printed.
With this updated default, the log seems to be printed to the stdout successfully. This is what the backend-cron pod log looks like when CONFIG_LOG_PATH=/dev/stdout is set on the pod explicitly:

fatal: not a git repository (or any of the parent directories): .git
fatal: not a git repository (or any of the parent directories): .git
fatal: not a git repository (or any of the parent directories): .git
I, [2026-04-22T07:49:17.829096 #21] INFO -- : Going to delete the stats keys for these services: ["5"]
I, [2026-04-22T07:49:17.832578 #21] INFO -- : Finished deleting the stats keys for these services: ["5"]
** Invoke reschedule_failed_jobs (first_time)
** Execute reschedule_failed_jobs
Rescheduled: 0. Failed and discarded: 0. Pending failed jobs: 0.
** Invoke stats:cleanup (first_time)
** Execute stats:cleanup
fatal: not a git repository (or any of the parent directories): .git
** Invoke reschedule_failed_jobs (first_time)
** Execute reschedule_failed_jobs
Rescheduled: 0. Failed and discarded: 0. Pending failed jobs: 0.
fatal: not a git repository (or any of the parent directories): .git
** Invoke reschedule_failed_jobs (first_time)
** Execute reschedule_failed_jobs
Rescheduled: 0. Failed and discarded: 0. Pending failed jobs: 0.

Copy link
Copy Markdown
Contributor

@akostadinov akostadinov Apr 22, 2026

Choose a reason for hiding this comment

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

previously it was not unset but it was set to dev/stdout which expectedly cannot be found.

Copy link
Copy Markdown
Contributor

@akostadinov akostadinov Apr 22, 2026

Choose a reason for hiding this comment

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

Errno::ENOENT: No such file or directory @ rb_file_s_stat - dev/stdout (Errno::ENOENT)

This means it cannot find dev/stdout.

Copy link
Copy Markdown
Contributor

@jlledom jlledom Apr 22, 2026

Choose a reason for hiding this comment

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

I think the change is not wrong. According to this:

apisonator/lib/3scale/backend/configuration.rb

Lines 99 to 113 in 58baa5c

# often we don't have a log_file setting - generate it here from
# the log_path setting.
log_file = config.log_file
if !log_file || log_file.empty?
log_path = config.log_path
config.log_file = if log_path && !log_path.empty?
if File.stat(log_path).ftype == 'directory'
"#{log_path}/backend_logger.log"
else
log_path
end
else
ENV['CONFIG_LOG_FILE'] || STDOUT
end
end

This change would end up actually logging to /dev/stdout. I think removing the whole line would lead to the same result anyway, but I don't have the energy to test it and confirm it right now. IMO we should just merge this, fix the immediate problem and maybe review the whole logging system later on.

Copy link
Copy Markdown
Contributor

@akostadinov akostadinov Apr 22, 2026

Choose a reason for hiding this comment

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

Ok, I prefer not to have redundant settings in there. Also the variable doesn't seem to be used as intended as a PATH. But not super strong opinion. You can merge as you like it.


def run_rake_task_in_thread(name, freq, check_once = false)
Thread.new do
Expand Down