YunoHost-Apps / ffsync_ynh

Mozilla’s Sync Server package for YunoHost
GNU Affero General Public License v3.0
15 stars 9 forks source link

Use new helpers, add backup/restore and uwsgi #6

Closed Jibec closed 5 years ago

Jibec commented 6 years ago

Hi, I wanted to add backup and restore, and I finally rewrote this package.

Some part I did not understood where removed (like swap) or I didn't like the easy_install ran as root.

Using gunicorn looked like to require a port, I decided to use uwsgi to have a socket instead.

I'm unsure it works, package_check gives weird results (upgrade doesn't work with package_check, but works fine on my VM). Please have a looks and play with it :)

Do not install this in production if you are not ready to loose your data.

Jibec commented 6 years ago

this work is now ready for testing, please come and try, but use it carefully, everything is in your database, to be safe, just dump it!

rigelk commented 6 years ago

Since I don't use YunoHost anymore in production, I can't test it fully right now. Look very nice though! I will be testing it and doing a proper review in the upcoming days if I manage to free some time for a proper test.

Jibec commented 6 years ago

I see time is hard to find! Do you mind transfering this application to me?

Jibec commented 6 years ago

@maniackcrudelis I can't figure out the reason why upgrade from fd6350495d5a1d864ae30e1a61e18939fdb6a428 and 267ccc21f7b52d22bc3d5b9cd6239857b9a82aad fails as it works fine in my VM... I would appreciate it you may have a look.

@rigelk it also includes a maintainer change, (btw, beudbeud was written)

Josue-T commented 6 years ago

Hello, Thanks @Jibec for your work. I think it could be good to go forward with this PR because I have the same issue https://github.com/YunoHost-Apps/ffsync_ynh/issues/7 and it might affect many user. This PR should really improve the package. I allow myself to just review a little bit the code.

Jibec commented 6 years ago

Thanks for your comments. I won't work on this package anymore until I'm sure it can be merged. Please let me know if you can get approval from rigelk or YunoHost Apps team.

rigelk commented 6 years ago

@Jibec I can approve and merge the request when you deem it ready.

Jibec commented 6 years ago

oh, good to know you answers :) Then I'll read and answer questions, but I expect you to give feedback from times to time if you're merging requests ;)

Jibec commented 6 years ago

It now looks like to works fine from old install and every basic scenario.

Please test upgrade: yunohost app install https://github.com/YunoHost-Apps/ffsync_ynh/ && yunohost app upgrade ffsync -u https://github.com/jibec/ffsync_ynh/

rigelk commented 6 years ago

Works for me!

Jibec commented 6 years ago

Thanks for the test. Can you confirm you did test with real data? I have to say I don't have time yet to make a real end user test. I assume using a dedicated Firefox profile would do the job (create the profile, sync data, remove the profile, upgrade app, create a new profile, sync data: should bring back previous data). Is it a good way to test or do you know other?

Jibec commented 6 years ago

Sorry, at the moment I don't understand the added value of this rule. Please stop coming back on this subject here. You are really welcome to open a discussion on a public place so people can understand and debate.

licryle commented 6 years ago

hmm, on a clean 3.0 install, the upgrade fails on me: Note: I did update the pip script, maybe it's the culprit but unsure how to revert it at this time.

$ sudo pip install --ignore-installed --no-cache-dir pip
...
$ sudo yunohost app upgrade ffsync -u https://github.com/jibec/ffsync_ynh/
Upgrading apps ffsync
Upgrading app ffsync...
Warning: This app doesn't have any backup script.
Warning: 2018-07-17 04:19:14 URL:https://codeload.github.com/mozilla-services/syncserver/tar.gz/1.8.0 [11213/11213] -> "app.tar.gz" [1]
Warning:   Failed building wheel for cryptography
Warning:   Failed cleaning build dir for cryptography
Warning:   Failed building wheel for cffi
Warning: Command "/var/www/ffsync/local/bin/python2 -u -c "import setuptools, tokenize;__file__='/tmp/pip-install-3bc00N/cffi/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" install --record /tmp/pip-record-Ch3vy7/install-record.txt --single-version-externally-managed --compile --install-headers /var/www/ffsync/local/include/site/python2.7/cffi" failed with error code 1 in /tmp/pip-install-3bc00N/cffi/
Warning: !!
Warning:   ffsync's script has encountered an error. Its execution was cancelled.
Warning: !!
Warning: Upgrade failed.
Warning: 
Error: Unable to upgrade ffsync
Error: No app to upgrade
Jibec commented 6 years ago

I think you broke your own system by running your pip command as root and systemwide. Try with a new system to confirm that, I can't reproduce your issue.

Josue-T commented 6 years ago

@licryle do you have any more log about why it fail (mybe in /var/log/yunohost/yunohost-cli.log if you did the upgrade by the cli). You can get this by this command grep "yunohost.hook <lambda>" /var/log/yunohost/yunohost-cli.log

licryle commented 6 years ago

Just did a clean install:

The failing error during the upgrade is actually the same as during the install, however the install is deemed successful, while the upgrade is not. Reminder before the error:

Warning:   Failed building wheel for cryptography
Warning:   Failed cleaning build dir for cryptography
Warning:   Failed building wheel for cffi

In the cli logs, there was: 2018-07-18 02:59:26,532 DEBUG yunohost.hook <lambda> - [11549.1] Package libffi was not found in the pkg-config search path.

Found on some other githubs that this is probably due to a lack of some libraries: $ sudo apt-get install build-essential libssl-dev libffi-dev python-dev (Reference: https://github.com/sriyegna/Pikaptcha/issues/50 )

Now, it installs, but the service is ill configured: Failed to get status information via dbus for service uwsgi-app@ffsync.service, systemctl didn't recognize this service ('NoSuchUnit').

Also creates some issues with various API calls (like installing a certificate, I get a "path" exception). removing the app solves it, but it doesn't seem to be stable on my end.

How are you doing your setup?

Jibec commented 6 years ago

Again these dammed arm card. I'll stop this support someday as it drives me crazy. You spend months to make it work fine and arm comes to make you loose even more time. As you did the work let's try to make it work.

Josue-T commented 6 years ago

A new time, if we don't have any more log we can't know what it happens !! And I can't help.

Josue-T commented 6 years ago

Mybe If you upgraded pip to the version 10.0.1 your have the issue https://github.com/pypa/pip/issues/5366 (you can check your pip version by pip list | grep pip) I had the same problem. The solution is mybe to type pip uninstall pip to remove the pip version installed by pip. After you might use pip from the debian package.

licryle commented 6 years ago

Sorry JB! I'll see if I can dig into it tomorrow, any way I can help you, or ideas on what I could investigate? I have some knowledge of Yuno as I've dev-ed a small app, but it's still a little shallow! Thanks,

licryle commented 6 years ago

Ok, I've done more research on the issue and here is what I found... I'm still doubtful this is arm specific, and I'm surprised you don't experience the same issues on your end.

1- The manifest declares the var "path", but your install/upgrade script moved to "path_url", which creates an error during install about the "path" variable not found. This is blocking the installation of subsequent apps, with the cryptic error message "ERROR This url is not available or conflicts with an already installed app" Here's a patch, keeping "path" in settings, which is what the example does here: https://github.com/YunoHost/example_ynh/blob/master/scripts/install#L61 I can do a pull requests, I tested install & upgrade: https://github.com/licryle/ffsync_ynh/commit/5ad912e6c9ddfd4ff75bbd174fe3181a32ea6579

2- Considering my previous comment, we should probably add the dependency, or part of "build-essential libssl-dev libffi-dev". Happy to do a pull request: https://github.com/licryle/ffsync_ynh/commit/10f63a275504a925fefbd4ae6556561e53e3a7e9

3- Upon upgrade, the server actually works but... A- Not via the configured service file uwsgi-app@ffsync.service. This one fails to start, and that is shown in the Yunohost UI. I'm not sure why it fails. B- Instead it seems through the master uwsgi service, which writes its logs into /var/log/uwsgi/app/ffsync because that's how you configured uwsgi-app@.service I need to dig more into this, I'm not fully familiar with services, although I've played with them & openvpn before :)

Jibec commented 6 years ago

I thank you for spending time on this. I'll try to give it some more testing soon.

licryle commented 6 years ago

There you go: https://github.com/licryle/ffsync_ynh/commit/d762eaba85c240d778116fd8de8a8520c1ab4ee9

Fundamentally, I don't think we should leverage the autostart/execute from UWSGI. My understanding of Yuno architecture is that each app should be independant, and the previous iteration made it "auto start" based on UWSGI. If other apps did the same, they'd kind of depend on the same configuration of UWSGI.

In my patch, I've fixed that so there's a single service, named after the app name to avoid confusion, and also adjusted the log paths to not be under UWSGI paths, but under its own name.

Happy to push a pull request.

Josue-T commented 6 years ago

@licryle I don't really know why you want to change the uwsgi helper. I would say either the experimental helper is buggy and in this case it could be good to do a PR to fix that here either it's the implementation of this helper which is buggy or the bug is in an other place.

My point of view is that it's true to share the config between all app because it is a helper which is specifically made to be be shared between all apps.

licryle commented 6 years ago

Oh, right. I ignored this helper was part of the (future) core helpers. Then yes, you're right, the helper should be fixed to let individual services run rather than beng triggered by the central helper. Otherwise, there wouldn't be granular start/stop in systemctl//the Yuno admin if I'm not mistaken.

Jibec commented 6 years ago

nothing says the uwsgi helpers will part of future core helpers. I do use it because uwsgi is really difficult to make it work, so I use the same method everywhere I can, so I can use the feedback on one app for all of them.

I would be great to have your feedback on ARM support. I you reach any other issue, please provide the full debug trace so I can figure out the issue (I can't reproduce any of your issues).

licryle commented 6 years ago

Well, f6b79fe will certainly help with one of the 3 issues I've raised, but not the 2 other ones.

Reminder:

licryle commented 6 years ago

I see this topic is closed, but I don't think it's working, are you tracking (1) and (3) independently? Happy to help as mentioned before.

Josue-T commented 6 years ago

Hello,

I have pushed some fix, so it might be better now.

I have also tested the migration from the old package with my prod instance (on ARM) and it was good.

I think it should be ready for some other tests.

Josue-T commented 6 years ago

If nobody mind I purpose to merge this in 10 days

Jibec commented 6 years ago

I'm against this merge as you can't add me as maintainer (I canceled my pull request).

I'm fine with the idea this apps is taken over by you, and that my work may help this app to move forward! I'm sorry I couldn't say yes to all requests and decided to stop my contribution (for multiple good and bad reasons, mostly unrelated to this package).

Keep the hard work!

Josue-T commented 6 years ago

Ok,

I you want I can give you the access to this repos. It's you want.

Josue-T commented 6 years ago

Hello,

I discovered some issue (when I restart the server) with uwsgi. @Jibec what is the reason to not use the main uwsgi service (which automatically start all service in /etc/uwsgi/app-enabled) ?

Josue-T commented 6 years ago

Hello, I have pushed some new fix which might fix some problem especially on the reboot. Without this fix ffsync didn't started correctly on the reboot.

If somebody can try this it could be good. And if it's ok for everybody I purpose to merge this in 10 days if I don't find some new issues.

About the maintainer who will be the maintainer of this app ?

Josue-T commented 5 years ago

Let's merge this.

I'll be the maintainer of this app (if nobody mind).

Jibec commented 5 years ago

I'm explicitly OK for you being maintainer of this app. -- Jean-Baptiste Holcroft

rigelk commented 5 years ago

Same for me.