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] Can't delete a system backup with webadmin #750

Open
wants to merge 5 commits into
base: stretch-unstable
from

Conversation

@zamentur
Copy link
Contributor

commented Jul 8, 2019

The problem

Backup info fails to display system backup. So the user can't delete it in Webadmin.

Traceback (most recent call last):
File “/usr/lib/python2.7/dist-packages/moulinette/interfaces/api.py”, line 439, in process
ret = self.actionsmap.process(arguments, timeout=30, route=_route)
File “/usr/lib/python2.7/dist-packages/moulinette/actionsmap.py”, line 523, in process
return func(**arguments)
File “/usr/lib/moulinette/yunohost/backup.py”, line 2292, in backup_info
key_info[“size”] = info[“size_details”][category][name]
TypeError: list indices must be integers, not str

https://forum.yunohost.org/t/impossible-de-supprimer-une-archive-de-sauvegarde/8393

Solution

Transform the list of string of hook by an object like:

{ 'hooks': ["..."], 'size': 542}

PR Status

Ready
Tested on a yunohost with same bug by running yunohost backup info

How to test

Try to get info on old backup with system hook include (not only with apps)

Validation

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

left a comment

Looks legit to me but I really don't know anything about backup.

@zamentur zamentur removed the important label Jul 8, 2019

@zamentur

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

I remove flag important, cause i was thinkig about case where the server could completely break due to this bug, but after think on it better it doesn't.

@alexAubin

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Uuuuh do you have a step by step way to reproduce this ? I can't reproduce the issue nor with an old backup (backup_system_from_2p4 used by pytests) or with a fresh backup ... (with and without the --with-details option)

@zamentur

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

Indeed i just check with a new backup, and format is different!

New format:

{"size_details": {"apps": {}, "system": {"conf_ldap": 4134}}, "description": "", "created_at": 1562680193, "apps": {}, "system": {"conf_ldap": {"/usr/share/yunohost/hooks/backup/05-conf_ldap": "succeed"}}, "size": 118861}

Old format generated on 2019/01/02 (probably with the testing at this date):

{"size_details": {"apps": {}, "system": {"conf_ldap": 3692}}, "description": "", "created_at": 1546422083, "apps": {}, "system": {"conf_ldap": ["/usr/share/yunohost/hooks/backup/05-conf_ldap"]}, "size": 116924}

I was not aware of this change.

So to not break system we need to convert properlly old backup info.

@alexAubin

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

But uh then is there a bug in any stable in the past, and if so how can someone reproduce it ? x_X

zamentur added some commits Jul 9, 2019

@zamentur

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

With this bug, all people with old backup (in this old format) have their webadmin broken and are obliged to remove the backup manually.

The change of format is due to this change i think:
16245f5

I don't know if this old format is a bug or not of the past stable. I think no. The bug seems created by this PR and the commit above: #699 (merged in 3.6).

@zamentur

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

SO to reproduce, i think you just need to run:
On 3.5.x

yunohost backup create --system conf_ldap -n toto

Upgrade to 3.6.x
And next, try to get info (or to remove the backup from webadmin)

yunohost backup info toto
if not isinstance(key_info, dict):
key_info = {x: 'succeed' for x in key_info}
info[category][name] = key_info

if name in info["size_details"][category].keys():

This comment has been minimized.

Copy link
@alexAubin

alexAubin Jul 9, 2019

Member

Hmyea in fact I don't know if it's so interesting to have all these {'path1':'succeed', 'path2':'succeed'} ... I'm not even sure that the value can be different than suceed ... So maybe having the dict is a buggy behavior (probably my fault ?) that wasn't noticed when implementing the whole size_details stuff and we should just stick with a list ?

This comment has been minimized.

Copy link
@zamentur

This comment has been minimized.

Copy link
@zamentur

zamentur Jul 9, 2019

Author Contributor

I try to create an history of this format:
2.4:

{"hooks": {"conf_ldap": ["/usr/share/yunohost/hooks/backup/05-conf_ldap"]}, "created_at": 1494198169, "apps": {}, "description": "", "size": 524892}

3.5, size_details and hooks is replaced by system

{"size_details": {"apps": {}, "system": {"conf_ldap": 3692}}, "description": "", "created_at": 1546422083, "apps": {}, "system": {"conf_ldap": ["/usr/share/yunohost/hooks/backup/05-conf_ldap"]}, "size": 116924}

Current 3.6, use a dict instead of a list

{"size_details": {"apps": {}, "system": {"conf_ldap": 4134}}, "description": "", "created_at": 1562680193, "apps": {}, "system": {"conf_ldap": {"/usr/share/yunohost/hooks/backup/05-conf_ldap": "succeed"}}, "size": 118861}

So now, if we change again, it will complexify the code. We could also decide to stop retrocompatibility of 2.4 format to simplify some code or decide to hotfix quickly 3.6 and ignore this new 3.6 format.

In a perfect world, and with the current features (not the future one's), i think the format should be:

{"description": "", "created_at": 1562680193, "apps": {}, "system": {"conf_ldap": {"hook": "/usr/share/yunohost/hooks/backup/05-conf_ldap", "size": 4134}}, "size": 118861}

So no size_details, and a dict to put some meta info about this "part of backup".

This comment has been minimized.

Copy link
@alexAubin

alexAubin Jul 9, 2019

Member

In a perfect world, and with the current features (not the future one's), i think the format should be:

"system": {"conf_ldap": {"hook": "/usr/share/yunohost/hooks/backup/05-conf_ldap", "size": 4134}}

We can't have this, because the value associated to conf_ldap could (theoretically) contain several paths ... so at least the value corresponding to hook should be a list (meh). That's why in 3.5 (and prior) it was a list of path, and in 3.6 it's a dict of path ... though imho having this dict with just "succeed" as value is a bug of 3.6, and we should just revert to having simple list of paths ...

This comment has been minimized.

Copy link
@zamentur

zamentur Jul 9, 2019

Author Contributor

Note, it could be a good idea to register in info, system_part and apps failed to backup... Indeed in webadmin it could be usefull to display a backup and see part in failures... So the idea of succeed/failed is not so bad.

And after some test, i discover it's possible to have several hooks attached to the same system part ! That's why a list was used and not just a string...

This comment has been minimized.

Copy link
@zamentur

zamentur Aug 7, 2019

Author Contributor

so at least the value corresponding to hook should be a list (meh)

It's exactly what the PR does

@zamentur

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

Note if it was working with backup_system_from_2p4 it was because YunoHost 2.4 backup info doesn't include 'size_details' key. So no sizes are added with this backup, in the webadmin in this case it display '0'.

@zamentur

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Are you ok to merge this ?

@Psycojoker

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Don't know the details but ok to merge on principle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.