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

Addressing some issues #100

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dorpauli
Copy link

Must read before submitting

Please, take a look to our contributing guidelines before submitting your pull request.
There's some simple rules that will help us to speed up the review process and avoid any misunderstanding

Contributors GuideLines

Status

READY

Description

In our organization we like to use OCS-Inventory with docker. So we tested a lot and encounter some things that could be improved. Internally we now build our own image, but i want to share the main things we invented. First of all we changed the sed commands in the entrypoints with perl, so that complex passwords with special characters are no problem anymore.
Secondly we implemented the opportunity to use the install.php script in the image, if it is necessary.
The last thing is, that we shrink the size of the image while using docker multistage build. Now the image is around 327 MB. Before the change it was 547 MB. The trick is to not have build-essential in one of the image layers.

I hope you can make use of it and that the addressed issues can be solved with that PR. Please comment if you have questions and sth. is unclear.

Related Issues

#99
#98
#81

Todos

Test environment

I did some tests with it, but didn't use nginx.

General informations

Docker host's operating system : Debian 11 and Ubuntu 22.04 (testet both)
Mysql Server version : MariaDB 10.11

Docker informations

Docker compose version : 1.25.0
Docker version : 20.10.5

Deploy Notes

Notes regarding deployment the contained body of work. These should note any dependencies changes,
logical changes, etc.

Impacted Areas in Application

List general components of the application that this PR will affect:

  • Dockerfile
  • Webserver folder with possibility to add install.php by purpose.
  • Entrypoints
  • Version

Now there should be no issues with f.e. complex passwords and so on.
I had problems with mine, because it has a lot of special characters...
Image size was very high, because of the build tools installed.
With this change it's possible to not remove install.php on container startup.
This is usefull if there is no DB.
@dorpauli
Copy link
Author

By the way, there is some strange behavior with mysql as you use it with docker-compose. After restart of the images mysql has some errors when an agent pushes information. He gets a http 500 in this case. Sometimes after a login in the dashboard, this issue was gone. With mariadb everything works great!

@dorpauli
Copy link
Author

I reverted back to solution without multi staged build, but use one RUN now.

@nroach44
Copy link

nroach44 commented Apr 1, 2023

I can confirm that this works fine, no issues so far when using it in lieu of the published image.

@tom-ph
Copy link

tom-ph commented Apr 3, 2023

Hi, since you're editing the Dockerfile can you apt-get install cron to also address #97?
Or can I ask you how did you manage to make the cron_all_software.php cronjob work (if you did) in order to update the software installed on the servers?

Thanks,
Tommaso

@dorpauli
Copy link
Author

dorpauli commented Apr 3, 2023

@tom-ph Yes i will do so. But i don't think, this is enough to activate cron for ocsinventory correctly. As far as i can see, the admin have to manually set the cronjobs in the container. Even if cron is installed. Every time a new image is used for the container, the cronjob is gone. So sth. in the entrypoint of the container with new parameters would be better i think.
Or maybe it is possible to mount the needed cronjobs in /etc/cron.daily?

EDIT: You also cannot edit crontab, if there is no editor like nano or vim...

@tom-ph
Copy link

tom-ph commented Apr 3, 2023

Thanks!
My idea was to add a script that copies the cron_all_software.php into the cron.daily folder to the docker-entrypoint.d folder. Of course this can only be done if cron is installed.
Maybe the script could be added to the repo, looking for an env variable that defaults to false.
Maybe I'll do a pull request after yours get accepted.

@dorpauli
Copy link
Author

dorpauli commented Apr 3, 2023

Another idea that orients on the ocs documentation came in my mind. Will push it tomorrow, if i have time for that. :)

@dorpauli
Copy link
Author

dorpauli commented Apr 4, 2023

@tom-ph have you read https://wiki.ocsinventory-ng.org/13.Docker-documentation/Using-the-docker-image/#crontab-implementation ? Implementing cron in the container seems unnecessary. You can just use cron on the server, where docker is running, and "docker exec" the cron scripts inside the container.

@tom-ph
Copy link

tom-ph commented Apr 4, 2023

Actually I didn't, but I don't like it for a number of reasons.
The first is conceptual: I don't think that the host should implement an application logic.
The second is practical: we are planning to migrate OCSInventory to our kubernetes cluster and this would be a overhead (and in my opinion this is because the conceptual choice is not the best).

I think that the minimum requirement should be having cron inside the container to give the flexibility to implement the crontabs based on each one's requirements.

The ideal solution would be to give the possibility to activate each of the cronjobs described here:
https://wiki.ocsinventory-ng.org/03.Basic-documentation/Understanding-OCS-Crontab/#understanding-ocs-crontab
just using environment variables that are false by default.

@dorpauli
Copy link
Author

dorpauli commented Apr 4, 2023

Good point @tom-ph ! I have been thinking of another solution and would like to hear your opinion.

To ensure admins can set up the cronjobs exactly as they need, I would provide the ability to use a cron file. This file can be included as volume in /etc/cron/docker-crontab. If the file exists, the entrypoint script runs crontab /etc/cron/docker-crontab and activates cron with cron -n. With this solution I do not have to define fixed times and commands.

@dorpauli dorpauli changed the title Addressing some issues WIP: Addressing some issues Apr 4, 2023
@dorpauli dorpauli marked this pull request as draft April 4, 2023 18:32
@tom-ph
Copy link

tom-ph commented Apr 4, 2023

I think this could be a great solution. If well documented, it could be an easy way to setup crontab with all the needed flexibility.

@tom-ph
Copy link

tom-ph commented Apr 6, 2023

Hey, let me know if I can help!
By the way, I think nano can be removed from packages with your proposed solution.

@dorpauli
Copy link
Author

dorpauli commented Apr 6, 2023

Hey, let me know if I can help! By the way, I think nano can be removed from packages with your proposed solution.

Hey, you can test it of course. I've build a new image here: https://hub.docker.com/r/dorpauli/ocsinventory-docker-image
Or you just build it yourself. It's up to you.

Of course, the documentation will need to be updated if my changes are used. At this point I would like to help the developers. They just have to give me the hint where to write down what.

@dorpauli dorpauli changed the title WIP: Addressing some issues Addressing some issues Jul 4, 2023
@nicolashlightvfx
Copy link

nicolashlightvfx commented Mar 6, 2024

any news about this pull request being merge ?
it's almost a year now
of course there's a test image here
but officially we still have to run the cron manually inside the docker, to refresh the list of software detected on machines.

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.

None yet

4 participants