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

Newdav maintenancemode #27821

Merged
merged 1 commit into from May 12, 2017

Conversation

Projects
None yet
5 participants
@PVince81
Member

PVince81 commented May 5, 2017

Description

Enable maintenance mode plugin also for the new DAV endpoint.

Related Issue

Fixes owncloud/client#5738

How Has This Been Tested?

Integration tests.
Local test with a simple PROPFIND with maintenance mode enabled.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Tagged as critical because it causes data loss with "Local" external storage and potentially other system-wide storages.

Needs backport to stable9.1.

@SergioBertolinSG I need your help with behat: the @AfterScenario that disables maintenance mode after the test runs too late. So while the tests pass, the other @AfterScenario code fails because maintenance mode is still active at this point. Any ideas how to change the priority ?

@DeepDiver1975 please review the fix

@DeepDiver1975

🤕

@ckamm

This comment has been minimized.

Show comment
Hide comment
@ckamm

ckamm May 8, 2017

Member

Works for me

Member

ckamm commented May 8, 2017

Works for me

Show outdated Hide outdated tests/integration/config/behat.yml
@@ -13,15 +13,15 @@ default:
- admin
regular_user_password: 123456
mailhog_url: http://127.0.0.1:8025/api/v2/messages
- CommandLineContext:
baseUrl: http://localhost:8080
ocPath: ../../

This comment has been minimized.

@PVince81

PVince81 May 8, 2017

Member

clever!

@PVince81

PVince81 May 8, 2017

Member

clever!

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 8, 2017

Member

Rebased for CI

Member

PVince81 commented May 8, 2017

Rebased for CI

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 9, 2017

Member
14:37:46     /var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-new-endpoint.feature:5
14:37:46     /var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-new-endpoint.feature:547
14:37:46     /var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-old-endpoint.feature:5
14:37:46     /var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-old-endpoint.feature:490
Member

PVince81 commented May 9, 2017

14:37:46     /var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-new-endpoint.feature:5
14:37:46     /var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-new-endpoint.feature:547
14:37:46     /var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-old-endpoint.feature:5
14:37:46     /var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-old-endpoint.feature:490
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 9, 2017

Member

one more remaining, will have a look:

/var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-old-endpoint.feature:490
Member

PVince81 commented May 9, 2017

one more remaining, will have a look:

/var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-old-endpoint.feature:490
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 9, 2017

Member

boohoo the tests from this file pass locally

Member

PVince81 commented May 9, 2017

boohoo the tests from this file pass locally

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 9, 2017

Member

more exact output from Jenkins (which I do not see locally):

13:15:57   Scenario: Maintenance mode                        # /var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-old-endpoint.feature:490
13:15:58     Given maintenance mode is enabled               # CommandLineContext::maintenanceModeIs()
13:15:58 [Tue May  9 13:15:58 2017] ::1:54497 [207]: /remote.php/webdav
13:15:58     When Connecting to dav endpoint as user "admin" # FeatureContext::connectingToDavEndpointAsUser()
13:15:58     Then the HTTP status code should be "503"       # FeatureContext::theHTTPStatusCodeShouldBe()
13:15:58       Failed asserting that 207 matches expected '503'.
13:15:58 [Tue May  9 13:15:58 2017] ::1:54502 [207]: /remote.php/dav/systemtags/
13:15:58 [Tue May  9 13:15:58 2017] Login failed: 'user0' (Remote IP: '::1')
13:15:59 [Tue May  9 13:15:58 2017] ::1:54505 [401]: /remote.php/webdav/myFileToTag.txt
13:15:59 [Tue May  9 13:15:59 2017] ::1:54506 [404]: /remote.php/dav/addressbooks/users/admin/MyAddressbook
13:15:59 [Tue May  9 13:15:59 2017] ::1:54509 [404]: /remote.php/dav/calendars/admin/MyCalendar
Member

PVince81 commented May 9, 2017

more exact output from Jenkins (which I do not see locally):

13:15:57   Scenario: Maintenance mode                        # /var/lib/jenkins/workspace/owncloud-core_core_PR-27821-CG35ITJCHSNDATF4YR5HHRU2J7EA77TPHL4EG53CZCPDPGIQ2GDA/tests/integration/features/webdav-related-old-endpoint.feature:490
13:15:58     Given maintenance mode is enabled               # CommandLineContext::maintenanceModeIs()
13:15:58 [Tue May  9 13:15:58 2017] ::1:54497 [207]: /remote.php/webdav
13:15:58     When Connecting to dav endpoint as user "admin" # FeatureContext::connectingToDavEndpointAsUser()
13:15:58     Then the HTTP status code should be "503"       # FeatureContext::theHTTPStatusCodeShouldBe()
13:15:58       Failed asserting that 207 matches expected '503'.
13:15:58 [Tue May  9 13:15:58 2017] ::1:54502 [207]: /remote.php/dav/systemtags/
13:15:58 [Tue May  9 13:15:58 2017] Login failed: 'user0' (Remote IP: '::1')
13:15:59 [Tue May  9 13:15:58 2017] ::1:54505 [401]: /remote.php/webdav/myFileToTag.txt
13:15:59 [Tue May  9 13:15:59 2017] ::1:54506 [404]: /remote.php/dav/addressbooks/users/admin/MyAddressbook
13:15:59 [Tue May  9 13:15:59 2017] ::1:54509 [404]: /remote.php/dav/calendars/admin/MyCalendar
@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG May 9, 2017

Member

It is connecting using "admin" but you have changed it here 4787fb7#diff-dc84a6418f59a28b93942dd9dd005b6bR842

Member

SergioBertolinSG commented May 9, 2017

It is connecting using "admin" but you have changed it here 4787fb7#diff-dc84a6418f59a28b93942dd9dd005b6bR842

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 9, 2017

Member

@SergioBertolinSG but then it should fail locally ? Are you able to reproduce it ?

Member

PVince81 commented May 9, 2017

@SergioBertolinSG but then it should fail locally ? Are you able to reproduce it ?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 9, 2017

Member

It is connecting using "admin" but you have changed it here 4787fb7#diff-dc84a6418f59a28b93942dd9dd005b6bR842

Connecting as "admin" in the maintenance tests is the correct scenario.

Tried re-checking out the branch locally, removing all extra apps, the test still doesn't fail.

Member

PVince81 commented May 9, 2017

It is connecting using "admin" but you have changed it here 4787fb7#diff-dc84a6418f59a28b93942dd9dd005b6bR842

Connecting as "admin" in the maintenance tests is the correct scenario.

Tried re-checking out the branch locally, removing all extra apps, the test still doesn't fail.

@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG May 9, 2017

Member

Nevermind my previous comment.

It is working fine for me:

[Tue May  9 13:24:13 2017] Exception: {"Message":"HTTP\/1.1 503 System in maintenance mode.","Exception":"Sabre\\DAV\\Exception\\ServiceUnavailable","Code":0,"Trace":"#0 [internal function]: OCA\\DAV\\Connector\\Sabre\\MaintenancePlugin->checkMaintenanceMode(Object(Sabre\\HTTP\\Request), Object(Sabre\\HTTP\\Response))\n#1 \/var\/www\/html\/trashbin_vincent\/lib\/composer\/sabre\/event\/lib\/EventEmitterTrait.php(105): call_user_func_array(Array, Array)\n#2 \/var\/www\/html\/trashbin_vincent\/lib\/composer\/sabre\/dav\/lib\/DAV\/Server.php(466): Sabre\\Event\\EventEmitter->emit('beforeMethod', Array)\n#3 \/var\/www\/html\/trashbin_vincent\/lib\/composer\/sabre\/dav\/lib\/DAV\/Server.php(254): Sabre\\DAV\\Server->invokeMethod(Object(Sabre\\HTTP\\Request), Object(Sabre\\HTTP\\Response))\n#4 \/var\/www\/html\/trashbin_vincent\/apps\/dav\/appinfo\/v1\/webdav.php(63): Sabre\\DAV\\Server->exec()\n#5 \/var\/www\/html\/trashbin_vincent\/remote.php(165): require_once('\/var\/www\/html\/t...')\n#6 {main}","File":"\/var\/www\/html\/trashbin_vincent\/apps\/dav\/lib\/Connector\/Sabre\/MaintenancePlugin.php","Line":83,"User":false}
[Tue May  9 13:24:13 2017] 127.0.0.1:33919 [503]: /remote.php/webdav
    When Connecting to dav endpoint as user "admin" # FeatureContext::connectingToDavEndpointAsUser()
Member

SergioBertolinSG commented May 9, 2017

Nevermind my previous comment.

It is working fine for me:

[Tue May  9 13:24:13 2017] Exception: {"Message":"HTTP\/1.1 503 System in maintenance mode.","Exception":"Sabre\\DAV\\Exception\\ServiceUnavailable","Code":0,"Trace":"#0 [internal function]: OCA\\DAV\\Connector\\Sabre\\MaintenancePlugin->checkMaintenanceMode(Object(Sabre\\HTTP\\Request), Object(Sabre\\HTTP\\Response))\n#1 \/var\/www\/html\/trashbin_vincent\/lib\/composer\/sabre\/event\/lib\/EventEmitterTrait.php(105): call_user_func_array(Array, Array)\n#2 \/var\/www\/html\/trashbin_vincent\/lib\/composer\/sabre\/dav\/lib\/DAV\/Server.php(466): Sabre\\Event\\EventEmitter->emit('beforeMethod', Array)\n#3 \/var\/www\/html\/trashbin_vincent\/lib\/composer\/sabre\/dav\/lib\/DAV\/Server.php(254): Sabre\\DAV\\Server->invokeMethod(Object(Sabre\\HTTP\\Request), Object(Sabre\\HTTP\\Response))\n#4 \/var\/www\/html\/trashbin_vincent\/apps\/dav\/appinfo\/v1\/webdav.php(63): Sabre\\DAV\\Server->exec()\n#5 \/var\/www\/html\/trashbin_vincent\/remote.php(165): require_once('\/var\/www\/html\/t...')\n#6 {main}","File":"\/var\/www\/html\/trashbin_vincent\/apps\/dav\/lib\/Connector\/Sabre\/MaintenancePlugin.php","Line":83,"User":false}
[Tue May  9 13:24:13 2017] 127.0.0.1:33919 [503]: /remote.php/webdav
    When Connecting to dav endpoint as user "admin" # FeatureContext::connectingToDavEndpointAsUser()
@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG May 9, 2017

Member

I can reproduce this moving this scenario https://github.com/owncloud/core/pull/27821/files#diff-0d2db90e35aaf54e2c763f470420c026R547

into webdav-related-old-endpoint.feature.

I think the problem happens when running all the tests together. When running isolated it doesn't fail.

Member

SergioBertolinSG commented May 9, 2017

I can reproduce this moving this scenario https://github.com/owncloud/core/pull/27821/files#diff-0d2db90e35aaf54e2c763f470420c026R547

into webdav-related-old-endpoint.feature.

I think the problem happens when running all the tests together. When running isolated it doesn't fail.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 9, 2017

Member

So it's likely a side effect ? Maybe in some case the maintenance mode isn't properly disabled

Member

PVince81 commented May 9, 2017

So it's likely a side effect ? Maybe in some case the maintenance mode isn't properly disabled

@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG May 9, 2017

Member

I'm trying something, I'll tell you if it succeds.

Member

SergioBertolinSG commented May 9, 2017

I'm trying something, I'll tell you if it succeds.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 10, 2017

Member

Tests are delayed now. We're experiencing env issues and potential race conditions.

So far it seems that while maintenance mode is enabled, the call still goes through when running the test on some env, and only sometimes. 1 of 5 test runs on the same env will fail... Suspecting PHP 7 bug with proc_close so far which maybe doesn't wait for the command to finish.

Needs further research...

Member

PVince81 commented May 10, 2017

Tests are delayed now. We're experiencing env issues and potential race conditions.

So far it seems that while maintenance mode is enabled, the call still goes through when running the test on some env, and only sometimes. 1 of 5 test runs on the same env will fail... Suspecting PHP 7 bug with proc_close so far which maybe doesn't wait for the command to finish.

Needs further research...

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 10, 2017

Member

Observed so far:

  • Jenkins has PHP 7.0.10 and has this failure
  • @SergioBertolinSG's env has PHP 7.0.11 and it fails there too, but randomly
  • my env has PHP 7.0.15 and I don't see any failure (had the tests running in a loop)
Member

PVince81 commented May 10, 2017

Observed so far:

  • Jenkins has PHP 7.0.10 and has this failure
  • @SergioBertolinSG's env has PHP 7.0.11 and it fails there too, but randomly
  • my env has PHP 7.0.15 and I don't see any failure (had the tests running in a loop)
@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG May 10, 2017

Member

After adding this:

diff --git a/apps/dav/lib/Connector/Sabre/MaintenancePlugin.php b/apps/dav/lib/Connector/Sabre/MaintenancePlugin.php
index e648dce5e6..bf309b0aa0 100644
--- a/apps/dav/lib/Connector/Sabre/MaintenancePlugin.php
+++ b/apps/dav/lib/Connector/Sabre/MaintenancePlugin.php
@@ -76,6 +76,7 @@ class MaintenancePlugin extends ServerPlugin {
         * @return bool
         */
        public function checkMaintenanceMode() {
+       \OCP\Util::writeLog('DEBUG', 'maintenance: ' . $this->config->getSystemValue('maintenance', false), \OCP\Util::DEBUG);
                if ($this->config->getSystemValue('singleuser', false)) {
                        throw new ServiceUnavailable('System in single user mode.');
                }

And run with log level 0, I can see

And maintenance mode is enabled # CommandLineContext::maintenanceModeIs()
[Wed May 10 16:24:24 2017] maintenance: 
[Wed May 10 16:24:24 2017] 127.0.0.1:51562 [207]: /remote.php/dav/files/admin
When Connecting to dav endpoint as user "admin" # FeatureContext::connectingToDavEndpointAsUser()
Then the HTTP status code should be "503" # FeatureContext::theHTTPStatusCodeShouldBe()
Failed asserting that 207 matches expected '503'.

The enabling maintenance mode doesn't seems to work fine.

Member

SergioBertolinSG commented May 10, 2017

After adding this:

diff --git a/apps/dav/lib/Connector/Sabre/MaintenancePlugin.php b/apps/dav/lib/Connector/Sabre/MaintenancePlugin.php
index e648dce5e6..bf309b0aa0 100644
--- a/apps/dav/lib/Connector/Sabre/MaintenancePlugin.php
+++ b/apps/dav/lib/Connector/Sabre/MaintenancePlugin.php
@@ -76,6 +76,7 @@ class MaintenancePlugin extends ServerPlugin {
         * @return bool
         */
        public function checkMaintenanceMode() {
+       \OCP\Util::writeLog('DEBUG', 'maintenance: ' . $this->config->getSystemValue('maintenance', false), \OCP\Util::DEBUG);
                if ($this->config->getSystemValue('singleuser', false)) {
                        throw new ServiceUnavailable('System in single user mode.');
                }

And run with log level 0, I can see

And maintenance mode is enabled # CommandLineContext::maintenanceModeIs()
[Wed May 10 16:24:24 2017] maintenance: 
[Wed May 10 16:24:24 2017] 127.0.0.1:51562 [207]: /remote.php/dav/files/admin
When Connecting to dav endpoint as user "admin" # FeatureContext::connectingToDavEndpointAsUser()
Then the HTTP status code should be "503" # FeatureContext::theHTTPStatusCodeShouldBe()
Failed asserting that 207 matches expected '503'.

The enabling maintenance mode doesn't seems to work fine.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 10, 2017

Member

Let's not waste more time on this testing issue and get the main fix merged.
We can continue researching this weird behavior in a separate PR that adds maintenance mode tests. I don't see these tests as really critical or helpful in the short term.

Member

PVince81 commented May 10, 2017

Let's not waste more time on this testing issue and get the main fix merged.
We can continue researching this weird behavior in a separate PR that adds maintenance mode tests. I don't see these tests as really critical or helpful in the short term.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 10, 2017

Member

I'll take care of the splitting tomorrow

Member

PVince81 commented May 10, 2017

I'll take care of the splitting tomorrow

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 11, 2017

Member

I've removed the test related commits from this PR, so only the fix remains.

The tests were moved to another branch and PR here #27856

Member

PVince81 commented May 11, 2017

I've removed the test related commits from this PR, so only the fix remains.

The tests were moved to another branch and PR here #27856

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 11, 2017

Member

stable9.1: #27857

Member

PVince81 commented May 11, 2017

stable9.1: #27857

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 12, 2017

Member

[ERROR] Waited 600 seconds, no response

no service

Member

PVince81 commented May 12, 2017

[ERROR] Waited 600 seconds, no response

no service

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 12, 2017

Member

rebased...

Member

PVince81 commented May 12, 2017

rebased...

@DeepDiver1975 DeepDiver1975 merged commit 9f644c4 into master May 12, 2017

3 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@DeepDiver1975 DeepDiver1975 deleted the newdav-maintenancemode branch May 12, 2017

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