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

Add logrotate to the Docker image #17609

Closed

Conversation

ksuderman
Copy link
Contributor

Add logrotate to the Docker image build.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. Build the Docker image
    2. Launch Galaxy using the new image
    3. Login to one of Galaxy pods
    4. Ensure which logrotate returns /usr/sbin/logrotate and the /etc/logrotate.d directory is empty.

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@ksuderman ksuderman requested a review from nuwang March 5, 2024 20:27
@ksuderman ksuderman added this to the 24.0 milestone Mar 5, 2024
@ksuderman
Copy link
Contributor Author

I am not sure why, but including logrotate in with the previous RUN apt-get install step causes the adduser step to fail, possibly due to the --no-install-recommends? Placing the install step prior to the adduser step also causes the adduser step to fail. However, placing it after the adduser step works. Color me confused.

Copy link
Member

@nuwang nuwang left a comment

Choose a reason for hiding this comment

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

Is this because some logs are getting saved in the container? If so, I think we should stop that from happening. If any logs are getting saved in the container, the logs will be lost whenever the container restarts. Instead, the logging needs to be reconfigured so it goes into the console logs, which can then be inspected with docker log or managed with log shipping tools.

@github-actions github-actions bot modified the milestones: 24.0, 24.1 Mar 5, 2024
@ksuderman
Copy link
Contributor Author

ksuderman commented Mar 5, 2024

The maintence.sh script sends output to a log file that I have been directing to /galaxy/server/database. The problems I've encountered with the console logs is they get deleted with the pod. Since the maintenance.sh script runs as a CronJob the pod is long gone before I can look at the logs. Of all the ways to solve that problem (re-writing the maintenance.sh script, shipping the logs, etc.) the easiest, for now, would be to simply rotate that log file. Getting this into the 24.0 release at least gives us that option.

@nuwang
Copy link
Member

nuwang commented Mar 6, 2024

I still don't think logrotate should be in this container (among other things, we would have to violate the single-responsibility principle and manage more than one process in the container). Some options we could use:

  1. Increase log-retention k8s wide through kubelet configuration - https://kubernetes.io/docs/concepts/cluster-administration/logging/#log-rotation
  2. Ue a side-car container, something like: https://github.com/blacklabelops/logrotate that can be optionally enabled. Disabled by default and provided as an option for basic management of logs.
  3. Install cluster-level loggers like fluent-bit, fluentd etc. - which we could recommend as the production option.

@nuwang
Copy link
Member

nuwang commented Mar 6, 2024

Thinking about this a bit more, perhaps we should remove both gxadmin and logrotate, and package them into a utility container for use in maintenance, logging etc. Neither of them need to be in the galaxy container? Looks like gxadmin calls pgcleanup, so it may indeed need to live in this container after all. Alternatively, we could extend the base container and layer gxadmin+logrotate on top in a separate utility container.

@ksuderman
Copy link
Contributor Author

I still don't think logrotate should be in this container (among other things, we would have to violate the single-responsibility principle and manage more than one process in the container).

All good points. However logrotate would not run as a process in the Galaxy pods, it is just that the CronJobs use the Galaxy image so that is where I stuck it. Having a separate image based on BusyBox or Alpine is something I considered and is likely a better solution if we don't want logrotate in the Galaxy image. I'll close this PR and investigate those options.

@ksuderman ksuderman closed this Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants