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

[fix] Don't backup cron files #442

Closed
wants to merge 1 commit into from
Closed

Conversation

@alexAubin
Copy link
Member

@alexAubin alexAubin commented Mar 16, 2018

The problem

c.f. https://dev.yunohost.org/issues/933

Solution

This is an alternative solution to #334

c.f. my comment here : #334 (comment) , I personally don't understand the motivation to backup cron jobs... They are neither configuration nor data, in the sense that they should be sat up by the postinstall or other relevant pieces of code...

For instance, we do not backup/restore the /etc/nginx/conf.d/domain.tld.conf files because the information that matters is the existence of the domain.tld domain, not the conf file itself since it is to be regenerated by the regen-conf...

If for instance we decide to change the cron job for dyndns domain for various reasons (e.g. change the frequency or optimize the way it works) then a backup before that moment, followed by a restore, will override the new cron with the old one...

PR Status

Done / ready for review

How to test

Create a backup, and check that no cron job is backuped..

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@zamentur
Copy link
Contributor

@zamentur zamentur commented Apr 29, 2018

It's a design question between backup strategy and the regen-conf feature.

We should backup all /etc (and may be complete mysql databases unreferenced by an app)... User don't know how to add some files to backup, /etc is not so big and there is a lot of configuration (yunohost or user config).

But we are not obliged to restore all part of the system configuration. Some users wants to get a "clean" install after restoring...

About cron, I don't see where is the mechanism to regenerate cron like:
/etc/cron.d/yunohost-applist-*
/etc/cron.daily/yunohost-fetch-appslists
/etc/cron.d/yunohost-dyndns
/etc/cron.d/yunohost-monitor

So remove this restore hook isn't enough.

For instance, we do not backup/restore the /etc/nginx/conf.d/domain.tld.conf files because the information that matters is the existence of the domain.tld domain, not the conf file itself since it is to be regenerated by the regen-conf...

It's not exact, we backup and restore it...

https://github.com/YunoHost/yunohost/blob/unstable/data/hooks/restore/29-conf_nginx

The regen con mechanism manages conflict with user change. If the user has change some things may be he/she wants to backup it and even restore it.

@alexAubin
Copy link
Member Author

@alexAubin alexAubin commented May 10, 2018

So in fact, yes, it appears that we do restore nginx configuration files. But since those are handled by the regen-conf, either : a) the regen-conf is properly reset so that those are re-written with up to date conf automatically, or b) the regen-conf is not properly reset and all restored nginx files are flagged as manually modified, and that might lead to trouble now or later.

Imho, the focus should be on restoring a clean, working setup. Restoring manually-edited configuration files, and/or files which are not handled by the regen-conf, is a hazard in the sense that we cannot ensure that it won't break the destination setups because of backward/forward-incompatible stuff...

But this whole question of backuping / restoring /etc/ as a whole is a different question so I would say it's outside of the scope here (though it's related) ...

About cron, I don't see where is the mechanism to regenerate cron

It's the same as the mechanism that initially creates them, which might be different for each cron. But for instance, /etc/cron.daily/yunohost-fetch-appslists is created during the postinstall. Crons like /etc/cron.d/yunohost-applist-* are outdated / legacy stuff, which is another example of why we shouldn't restore crons imho :/ (though the code handling the automatic migration is still there).

/etc/cron.d/yunohost-monitor is created when running monitor_enable()... in that case, I doubt that it's something done automatically during postinstall, but anyway I expect that restoring this cron alone doesn't make sense if you don't start and enable glances which is what the rest of monitor_enable does...

For /etc/cron.d/yunohost-dyndns, it's a bit trickier and needs testing to conclude I think, depending on when / how the other files are restored (in particular the keys) but in any case it's not future-proof to just cp this file imho :s

@zamentur
Copy link
Contributor

@zamentur zamentur commented May 15, 2018

yunohost-certificate-renew

@alexAubin alexAubin added this to the 3.x milestone Jun 13, 2018
@alexAubin alexAubin changed the base branch from unstable to stretch-unstable Jun 17, 2018
@alexAubin
Copy link
Member Author

@alexAubin alexAubin commented Nov 27, 2018

So imho the proper solution on this is to :

  • Have #480
  • Integrate a regen-conf script to maintain a single crontab-like file, which contains all the cron managed by yunohost
    • e.g. the regen-conf script shall detect that a dyndns domain is enabled, and include the corresponding cron rule

@Psycojoker
Copy link
Member

@Psycojoker Psycojoker commented Dec 14, 2018

So imho the proper solution on this is to :

* Have #480

* Integrate a regen-conf script to maintain a single crontab-like file, which contains all the cron managed by yunohost
  
  * e.g. the regen-conf script shall detect that a dyndns domain is enabled, and include the corresponding cron rule

Looks like the best.

I think at some point we should have a cron.py in YunoHost to integrate cron management or something but that would be long term.

@alexAubin
Copy link
Member Author

@alexAubin alexAubin commented Apr 18, 2019

#653 got merged so we could move forward with this

@alexAubin alexAubin force-pushed the stretch-unstable branch from 58ead9e to 40f48ff Oct 29, 2019
@zamentur zamentur removed this from the 4.x milestone Jan 3, 2021
@zamentur zamentur added this to the Horizon milestone Jan 3, 2021
@zamentur zamentur added this to Divergence of opinion in Pending Jan 4, 2021
@zamentur zamentur moved this from Divergence of opinion to Needs triage in Pending Jan 4, 2021
@zamentur zamentur removed this from the Horizon milestone Jan 4, 2021
@alexAubin alexAubin closed this Mar 16, 2021
Pending automation moved this from Needs triage to Graveyard Mar 16, 2021
@alexAubin alexAubin deleted the dont-backup-crons branch Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Pending
Graveyard
3 participants