Detect breaking apps when enabled in the web UI #17435

Closed
PVince81 opened this Issue Jul 7, 2015 · 25 comments

Comments

Projects
None yet
10 participants
@PVince81
Member

PVince81 commented Jul 7, 2015

See #14754 (comment)

Basically the workflow would be as follows:

  1. User enables "someapp" in the apps manager
  2. Frontend sends a request to the Server to enable the app
  3. Server enables the app
  4. Immediately after the previous request finished, Frontend sends another parallel request to a special route to check if Server still works (to see whether the app broke something with autoloader, missing classes, etc)
  5. If Server did not give a proper response:
    1. Frontend sends a request to a privileged route of Server (one available for the admin but that does not enable any app)
    2. Server disables the broken app
    3. Frontend aknowledges this by showing a proper error message in the UI
    4. End
  6. Frontend displays success

@Raydiation @icewind1991 @MorrisJobke @nickvergessen

Regarding command line enabling, a similar workflow could be used.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2015

Member

A similar workflow should be used when clicking "Update", if possible.

Member

PVince81 commented Jul 7, 2015

A similar workflow should be used when clicking "Update", if possible.

@nickvergessen

This comment has been minimized.

Show comment
Hide comment
@nickvergessen

nickvergessen Jul 7, 2015

Contributor

We are already pinging the server to get the navigation entries: abuse 😦

Contributor

nickvergessen commented Jul 7, 2015

We are already pinging the server to get the navigation entries: abuse 😦

@PVince81

This comment has been minimized.

Show comment
Hide comment
Member

PVince81 commented Jul 7, 2015

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2015

Member

@nickvergessen really ? Then we could reuse that.

Member

PVince81 commented Jul 7, 2015

@nickvergessen really ? Then we could reuse that.

@oparoz

This comment has been minimized.

Show comment
Hide comment
@oparoz

oparoz Jul 7, 2015

Contributor

How are you going to test if the app works? A Ping to its main route to see if you get a 200?

Contributor

oparoz commented Jul 7, 2015

How are you going to test if the app works? A Ping to its main route to see if you get a 200?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2015

Member

Another suggestion I made on IRC was about having a temporary app config value "temp_enabled_until". The server would write a timestamp there which would leave the app enabled for a short time window. If within this time window the client's "ping" succeeded, the call would change it to permanently enabled. If the ping did not succeed, the app will auto-disable after the timeout.
The auto-disable is achieved by checking the timestamp in this value when retrieving the list of enabled apps.

But this could cause other issues like if an app takes longer than the timeout to enable itself / init/migrate the database.

I added this here for the sake of completeness in case it might spark even better ideas 😉

Member

PVince81 commented Jul 7, 2015

Another suggestion I made on IRC was about having a temporary app config value "temp_enabled_until". The server would write a timestamp there which would leave the app enabled for a short time window. If within this time window the client's "ping" succeeded, the call would change it to permanently enabled. If the ping did not succeed, the app will auto-disable after the timeout.
The auto-disable is achieved by checking the timestamp in this value when retrieving the list of enabled apps.

But this could cause other issues like if an app takes longer than the timeout to enable itself / init/migrate the database.

I added this here for the sake of completeness in case it might spark even better ideas 😉

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2015

Member

@oparoz that could be an idea too. But here the main goal was to make sure the app doesn't brick ownCloud... which is even worse than just having a broken app route.

Member

PVince81 commented Jul 7, 2015

@oparoz that could be an idea too. But here the main goal was to make sure the app doesn't brick ownCloud... which is even worse than just having a broken app route.

@oparoz

This comment has been minimized.

Show comment
Hide comment
@oparoz

oparoz Jul 7, 2015

Contributor

@PVince81 - But if oC is bricked then you're never going to get a reply or be able to disable the app, unless you're calling occ, no?

Contributor

oparoz commented Jul 7, 2015

@PVince81 - But if oC is bricked then you're never going to get a reply or be able to disable the app, unless you're calling occ, no?

@BernhardPosselt

This comment has been minimized.

Show comment
Hide comment
@BernhardPosselt

BernhardPosselt Jul 7, 2015

Contributor

After a small delay, Frontend sends another parallel request to a special route to check if Server still works (to see whether the app broke something with autoloader, missing classes, etc)

Should be "immediately after the previous request finished"

Contributor

BernhardPosselt commented Jul 7, 2015

After a small delay, Frontend sends another parallel request to a special route to check if Server still works (to see whether the app broke something with autoloader, missing classes, etc)

Should be "immediately after the previous request finished"

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2015

Member

@oparoz yes, if OC is bricked you cannot disable the app. That's why we need a new special route that doesn't enable any apps to bypass the bricking. That route would disable it.

Member

PVince81 commented Jul 7, 2015

@oparoz yes, if OC is bricked you cannot disable the app. That's why we need a new special route that doesn't enable any apps to bypass the bricking. That route would disable it.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2015

Member

@Raydiation thanks, updated

Member

PVince81 commented Jul 7, 2015

@Raydiation thanks, updated

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jul 7, 2015

Member

Regarding: protect use from apps breaking the instance - could this be a solution: https://github.com/fieryprophet/php-sandbox

Member

DeepDiver1975 commented Jul 7, 2015

Regarding: protect use from apps breaking the instance - could this be a solution: https://github.com/fieryprophet/php-sandbox

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@oparoz

This comment has been minimized.

Show comment
Hide comment
@oparoz

oparoz Jul 7, 2015

Contributor

I'm a bit worried about the overhead when using such parsers. Looking for data as I write this...
Runkit is on Github https://github.com/zenovich/runkit/

Contributor

oparoz commented Jul 7, 2015

I'm a bit worried about the overhead when using such parsers. Looking for data as I write this...
Runkit is on Github https://github.com/zenovich/runkit/

@BernhardPosselt

This comment has been minimized.

Show comment
Hide comment
@BernhardPosselt

BernhardPosselt Jul 7, 2015

Contributor

Probably needs to be built in PHP rather than run on top of PHP otherwise like @oparoz mentioned this will be way to slow.

Contributor

BernhardPosselt commented Jul 7, 2015

Probably needs to be built in PHP rather than run on top of PHP otherwise like @oparoz mentioned this will be way to slow.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@oparoz

This comment has been minimized.

Show comment
Hide comment
@oparoz

oparoz Jul 7, 2015

Contributor

^ builtin

Yes, but with this requirement
"Regardless of which version of PHP is in use it must be compiled with the --enable-maintainer-zts option"

Contributor

oparoz commented Jul 7, 2015

^ builtin

Yes, but with this requirement
"Regardless of which version of PHP is in use it must be compiled with the --enable-maintainer-zts option"

@BernhardPosselt

This comment has been minimized.

Show comment
Hide comment
@BernhardPosselt

BernhardPosselt Jul 7, 2015

Contributor

Usual PHP problem "well its possilbe but it wont work for your users"

Contributor

BernhardPosselt commented Jul 7, 2015

Usual PHP problem "well its possilbe but it wont work for your users"

@enoch85

This comment has been minimized.

Show comment
Hide comment
Member

enoch85 commented Jul 7, 2015

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2015

Member

Proof of concept here: #17451

Member

PVince81 commented Jul 7, 2015

Proof of concept here: #17451

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2015

Member

Just tried ./occ with a broken app. It is still possible to set "maintenance" mode to true in the config to get back access to ./occ app:disable broken_app. So CLI isn't too big an issue.

Member

PVince81 commented Jul 7, 2015

Just tried ./occ with a broken app. It is still possible to set "maintenance" mode to true in the config to get back access to ./occ app:disable broken_app. So CLI isn't too big an issue.

@tflidd

This comment has been minimized.

Show comment
Hide comment
@tflidd

tflidd Aug 16, 2015

Contributor

@PVince81 I have installed the latest news app on owncloud (news app requires php5.5+, I have only php5.4). I tried to disable via ./occ app:disable news, which throws an error. And I can't turn it into the maintenance mode via ./occ!
#18315 (comment)

Contributor

tflidd commented Aug 16, 2015

@PVince81 I have installed the latest news app on owncloud (news app requires php5.5+, I have only php5.4). I tried to disable via ./occ app:disable news, which throws an error. And I can't turn it into the maintenance mode via ./occ!
#18315 (comment)

@MorrisJobke

This comment has been minimized.

Show comment
Hide comment
@MorrisJobke

MorrisJobke Aug 16, 2015

Member

@LukasReschke can the integrity check help here too?

Member

MorrisJobke commented Aug 16, 2015

@LukasReschke can the integrity check help here too?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Aug 17, 2015

Member

@tflidd you can enable maintenance mode by editing config/config.php and adding/setting "maintenance" to true

Member

PVince81 commented Aug 17, 2015

@tflidd you can enable maintenance mode by editing config/config.php and adding/setting "maintenance" to true

@cmonteroluque cmonteroluque added this to the 9.0-next milestone Sep 21, 2015

@jospoortvliet jospoortvliet referenced this issue in owncloud/updater Jan 12, 2016

Merged

More unit tests and enhancements #213

2 of 6 tasks complete

@cmonteroluque cmonteroluque modified the milestones: 9.1-next, 9.0-current Feb 23, 2016

@LukasReschke

This comment has been minimized.

Show comment
Hide comment
@LukasReschke

LukasReschke Mar 1, 2016

Member

Probably fixed by #17451? Thus closing. Please reopen if I'm wrong.

Member

LukasReschke commented Mar 1, 2016

Probably fixed by #17451? Thus closing. Please reopen if I'm wrong.

@MorrisJobke MorrisJobke modified the milestones: 9.0-current, 9.1-next Mar 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment