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

Newdav maintenancemode #27821

Merged
merged 1 commit into from
May 12, 2017
Merged

Newdav maintenancemode #27821

merged 1 commit into from
May 12, 2017

Conversation

PVince81
Copy link
Contributor

@PVince81 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

Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤕

@ckamm
Copy link

ckamm commented May 8, 2017

Works for me

@@ -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: ../../
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clever!

@PVince81
Copy link
Contributor Author

PVince81 commented May 8, 2017

Rebased for CI

@PVince81
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

PVince81 commented May 9, 2017

boohoo the tests from this file pass locally

@PVince81
Copy link
Contributor Author

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
Copy link
Contributor

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

@PVince81
Copy link
Contributor Author

PVince81 commented May 9, 2017

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

@PVince81
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

PVince81 commented May 9, 2017

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

@SergioBertolinSG
Copy link
Contributor

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

@felixboehm felixboehm added the p1-urgent Critical issue, need to consider hotfix with just that issue label May 10, 2017
@PVince81
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

I'll take care of the splitting tomorrow

@PVince81
Copy link
Contributor Author

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
Copy link
Contributor Author

stable9.1: #27857

@PVince81
Copy link
Contributor Author

PVince81 commented May 12, 2017

[ERROR] Waited 600 seconds, no response

no service

@PVince81
Copy link
Contributor Author

rebased...

@DeepDiver1975 DeepDiver1975 merged commit 9f644c4 into master May 12, 2017
@DeepDiver1975 DeepDiver1975 deleted the newdav-maintenancemode branch May 12, 2017 09:17
@lock
Copy link

lock bot commented Aug 3, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4 - To release p1-urgent Critical issue, need to consider hotfix with just that issue sev1-critical
Projects
None yet
5 participants