From: Giuseppe Lavagetto Date: Fri, 16 Feb 2018 16:08:58 +0000 (+0100) Subject: Expose the latest modified index seen by EtcdConfig X-Git-Tag: 1.31.0-rc.0~469^2 X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=2a77862c522dd0de76498f905aede9d334618e44 Expose the latest modified index seen by EtcdConfig While not immediately useful for fecthing the configuration, this is basically a version information about the configuration currently loaded from etcd. A typical use-case for this index is monitoring the freshness of the configuration across a cluster of servers. Bug: T182597 Change-Id: I58189d36a5b620fb44323bae87257f085a64386e --- diff --git a/includes/config/EtcdConfig.php b/includes/config/EtcdConfig.php index 3811da325c..7020159fd7 100644 --- a/includes/config/EtcdConfig.php +++ b/includes/config/EtcdConfig.php @@ -119,6 +119,11 @@ class EtcdConfig implements Config, LoggerAwareInterface { return $this->procCache['config'][$name]; } + public function getModifiedIndex() { + $this->load(); + return $this->procCache['modifiedIndex']; + } + /** * @throws ConfigException */ @@ -151,13 +156,17 @@ class EtcdConfig implements Config, LoggerAwareInterface { // refresh the cache from etcd, using a mutex to reduce stampedes... if ( $this->srvCache->lock( $key, 0, $this->baseCacheTTL ) ) { try { - list( $config, $error, $retry ) = $this->fetchAllFromEtcd(); - if ( is_array( $config ) ) { + $etcdResponse = $this->fetchAllFromEtcd(); + $error = $etcdResponse['error']; + if ( is_array( $etcdResponse['config'] ) ) { // Avoid having all servers expire cache keys at the same time $expiry = microtime( true ) + $this->baseCacheTTL; $expiry += mt_rand( 0, 1e6 ) / 1e6 * $this->skewCacheTTL; - - $data = [ 'config' => $config, 'expires' => $expiry ]; + $data = [ + 'config' => $etcdResponse['config'], + 'expires' => $expiry, + 'modifiedIndex' => $etcdResponse['modifiedIndex'] + ]; $this->srvCache->set( $key, $data, BagOStuff::TTL_INDEFINITE ); $this->logger->info( "Refreshed stale etcd configuration cache." ); @@ -165,7 +174,7 @@ class EtcdConfig implements Config, LoggerAwareInterface { return WaitConditionLoop::CONDITION_REACHED; } else { $this->logger->error( "Failed to fetch configuration: $error" ); - if ( !$retry ) { + if ( !$etcdResponse['retry'] ) { // Fail fast since the error is likely to keep happening return WaitConditionLoop::CONDITION_FAILED; } @@ -195,9 +204,10 @@ class EtcdConfig implements Config, LoggerAwareInterface { } /** - * @return array (config array or null, error string, allow retries) + * @return array (containing the keys config, error, retry, modifiedIndex) */ public function fetchAllFromEtcd() { + // TODO: inject DnsSrvDiscoverer in order to be able to test this method $dsd = new DnsSrvDiscoverer( $this->host ); $servers = $dsd->getServers(); if ( !$servers ) { @@ -209,8 +219,8 @@ class EtcdConfig implements Config, LoggerAwareInterface { $server = $dsd->pickServer( $servers ); $host = IP::combineHostAndPort( $server['target'], $server['port'] ); // Try to load the config from this particular server - list( $config, $error, $retry ) = $this->fetchAllFromEtcdServer( $host ); - if ( is_array( $config ) || !$retry ) { + $response = $this->fetchAllFromEtcdServer( $host ); + if ( is_array( $response['config'] ) || $response['retry'] ) { break; } @@ -218,12 +228,12 @@ class EtcdConfig implements Config, LoggerAwareInterface { $servers = $dsd->removeServer( $server, $servers ); } while ( $servers ); - return [ $config, $error, $retry ]; + return $response; } /** * @param string $address Host and port - * @return array (config array or null, error string, whether to allow retries) + * @return array (containing the keys config, error, retry, modifiedIndex) */ protected function fetchAllFromEtcdServer( $address ) { // Retrieve all the values under the MediaWiki config directory @@ -233,19 +243,21 @@ class EtcdConfig implements Config, LoggerAwareInterface { 'headers' => [ 'content-type' => 'application/json' ] ] ); + $response = [ 'config' => null, 'error' => null, 'retry' => false, 'modifiedIndex' => 0 ]; + static $terminalCodes = [ 404 => true ]; if ( $rcode < 200 || $rcode > 399 ) { - return [ - null, - strlen( $rerr ) ? $rerr : "HTTP $rcode ($rdesc)", - empty( $terminalCodes[$rcode] ) - ]; + $response['error'] = strlen( $rerr ) ? $rerr : "HTTP $rcode ($rdesc)"; + $response['retry'] = empty( $terminalCodes[$rcode] ); + return $response; } + try { - return [ $this->parseResponse( $rbody ), null, false ]; + $parsedResponse = $this->parseResponse( $rbody ); } catch ( EtcdConfigParseError $e ) { - return [ null, $e->getMessage(), false ]; + $parsedResponse = [ 'error' => $e->getMessage() ]; } + return array_merge( $response, $parsedResponse ); } /** @@ -264,8 +276,8 @@ class EtcdConfig implements Config, LoggerAwareInterface { "Unexpected JSON response: Missing or invalid node at top level." ); } $config = []; - $this->parseDirectory( '', $info['node'], $config ); - return $config; + $lastModifiedIndex = $this->parseDirectory( '', $info['node'], $config ); + return [ 'modifiedIndex' => $lastModifiedIndex, 'config' => $config ]; } /** @@ -275,8 +287,10 @@ class EtcdConfig implements Config, LoggerAwareInterface { * @param string $dirName The relative directory name * @param array $dirNode The decoded directory node * @param array &$config The output array + * @return int lastModifiedIndex The maximum last modified index across all keys in the directory */ protected function parseDirectory( $dirName, $dirNode, &$config ) { + $lastModifiedIndex = 0; if ( !isset( $dirNode['nodes'] ) ) { throw new EtcdConfigParseError( "Unexpected JSON response in dir '$dirName'; missing 'nodes' list." ); @@ -290,16 +304,19 @@ class EtcdConfig implements Config, LoggerAwareInterface { $baseName = basename( $node['key'] ); $fullName = $dirName === '' ? $baseName : "$dirName/$baseName"; if ( !empty( $node['dir'] ) ) { - $this->parseDirectory( $fullName, $node, $config ); + $lastModifiedIndex = max( + $this->parseDirectory( $fullName, $node, $config ), + $lastModifiedIndex ); } else { $value = $this->unserialize( $node['value'] ); if ( !is_array( $value ) || !array_key_exists( 'val', $value ) ) { throw new EtcdConfigParseError( "Failed to parse value for '$fullName'." ); } - + $lastModifiedIndex = max( $node['modifiedIndex'], $lastModifiedIndex ); $config[$fullName] = $value['val']; } } + return $lastModifiedIndex; } /** diff --git a/tests/phpunit/includes/config/EtcdConfigTest.php b/tests/phpunit/includes/config/EtcdConfigTest.php index c833934bfd..07dbd002c1 100644 --- a/tests/phpunit/includes/config/EtcdConfigTest.php +++ b/tests/phpunit/includes/config/EtcdConfigTest.php @@ -17,14 +17,23 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { ->getMock(); } - private function createSimpleConfigMock( array $config ) { + private static function createEtcdResponse( array $response ) { + $baseResponse = [ + 'config' => null, + 'error' => null, + 'retry' => false, + 'modifiedIndex' => 0, + ]; + return array_merge( $baseResponse, $response ); + } + + private function createSimpleConfigMock( array $config, $index = 0 ) { $mock = $this->createConfigMock(); $mock->expects( $this->once() )->method( 'fetchAllFromEtcd' ) - ->willReturn( [ - $config, - null, // error - false // retry? - ] ); + ->willReturn( self::createEtcdResponse( [ + 'config' => $config, + 'modifiedIndex' => $index, + ] ) ); return $mock; } @@ -70,6 +79,17 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { $config->get( 'unknown' ); } + /** + * @covers EtcdConfig::getModifiedIndex + */ + public function testGetModifiedIndex() { + $config = $this->createSimpleConfigMock( + [ 'some' => 'value' ], + 123 + ); + $this->assertSame( 123, $config->getModifiedIndex() ); + } + /** * @covers EtcdConfig::__construct */ @@ -81,6 +101,7 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { ->willReturn( [ 'config' => [ 'known' => 'from-cache' ], 'expires' => INF, + 'modifiedIndex' => 123 ] ); $config = $this->createConfigMock( [ 'cache' => $cache ] ); @@ -95,11 +116,8 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'class' => HashBagOStuff::class ] ] ); $config->expects( $this->once() )->method( 'fetchAllFromEtcd' ) - ->willReturn( [ - [ 'known' => 'from-fetch' ], - null, // error - false // retry? - ] ); + ->willReturn( self::createEtcdResponse( + [ 'config' => [ 'known' => 'from-fetch' ], ] ) ); $this->assertSame( 'from-fetch', $config->get( 'known' ) ); } @@ -166,7 +184,8 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'cache' => $cache, ] ); $mock->expects( $this->once() )->method( 'fetchAllFromEtcd' ) - ->willReturn( [ [ 'known' => 'from-fetch' ], null, false ] ); + ->willReturn( + self::createEtcdResponse( [ 'config' => [ 'known' => 'from-fetch' ] ] ) ); $this->assertSame( 'from-fetch', $mock->get( 'known' ) ); } @@ -191,7 +210,7 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'cache' => $cache, ] ); $mock->expects( $this->once() )->method( 'fetchAllFromEtcd' ) - ->willReturn( [ null, 'Fake error', false ] ); + ->willReturn( self::createEtcdResponse( [ 'error' => 'Fake error', ] ) ); $this->setExpectedException( ConfigException::class ); $mock->get( 'key' ); @@ -213,6 +232,7 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { [ 'config' => [ 'known' => 'from-cache' ], 'expires' => INF, + 'modifiedIndex' => 123 ] ) ); // .. misses lock @@ -241,6 +261,7 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { ->willReturn( [ 'config' => [ 'known' => 'from-cache' ], 'expires' => INF, + 'modifiedIndex' => 0, ] ); $cache->expects( $this->never() )->method( 'lock' ); @@ -266,6 +287,7 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { ->willReturn( [ 'config' => [ 'known' => 'from-cache' ], 'expires' => INF, + 'modifiedIndex' => 0, ] ); $cache->expects( $this->never() )->method( 'lock' ); @@ -292,6 +314,7 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { [ 'config' => [ 'known' => 'from-cache-expired' ], 'expires' => -INF, + 'modifiedIndex' => 0, ] ); // .. gets lock @@ -303,7 +326,7 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'cache' => $cache, ] ); $mock->expects( $this->once() )->method( 'fetchAllFromEtcd' ) - ->willReturn( [ [ 'known' => 'from-fetch' ], null, false ] ); + ->willReturn( self::createEtcdResponse( [ 'config' => [ 'known' => 'from-fetch' ] ] ) ); $this->assertSame( 'from-fetch', $mock->get( 'known' ) ); } @@ -321,6 +344,7 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { [ 'config' => [ 'known' => 'from-cache-expired' ], 'expires' => -INF, + 'modifiedIndex' => 0, ] ); // .. gets lock @@ -332,7 +356,7 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'cache' => $cache, ] ); $mock->expects( $this->once() )->method( 'fetchAllFromEtcd' ) - ->willReturn( [ null, 'Fake failure', true ] ); + ->willReturn( self::createEtcdResponse( [ 'error' => 'Fake failure', 'retry' => true ] ) ); $this->assertSame( 'from-cache-expired', $mock->get( 'known' ) ); } @@ -350,6 +374,7 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { ->willReturn( [ 'config' => [ 'known' => 'from-cache-expired' ], 'expires' => -INF, + 'modifiedIndex' => 0, ] ); // .. misses lock $cache->expects( $this->once() )->method( 'lock' ) @@ -374,16 +399,16 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'body' => json_encode( [ 'node' => [ 'nodes' => [ [ 'key' => '/example/foo', - 'value' => json_encode( [ 'val' => true ] ) + 'value' => json_encode( [ 'val' => true ] ), + 'modifiedIndex' => 123 ], ] ] ] ), 'error' => '', ], - 'expect' => [ - [ 'foo' => true ], // data - null, - false // retry - ], + 'expect' => self::createEtcdResponse( [ + 'config' => [ 'foo' => true ], // data + 'modifiedIndex' => 123 + ] ), ], '200 OK - Empty dir' => [ 'http' => [ @@ -393,25 +418,27 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'body' => json_encode( [ 'node' => [ 'nodes' => [ [ 'key' => '/example/foo', - 'value' => json_encode( [ 'val' => true ] ) + 'value' => json_encode( [ 'val' => true ] ), + 'modifiedIndex' => 123 ], [ 'key' => '/example/sub', 'dir' => true, + 'modifiedIndex' => 234, 'nodes' => [], ], [ 'key' => '/example/bar', - 'value' => json_encode( [ 'val' => false ] ) + 'value' => json_encode( [ 'val' => false ] ), + 'modifiedIndex' => 125 ], ] ] ] ), 'error' => '', ], - 'expect' => [ - [ 'foo' => true, 'bar' => false ], // data - null, - false // retry - ], + 'expect' => self::createEtcdResponse( [ + 'config' => [ 'foo' => true, 'bar' => false ], // data + 'modifiedIndex' => 125 // largest modified index + ] ), ], '200 OK - Recursive' => [ 'http' => [ @@ -422,25 +449,28 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { [ 'key' => '/example/a', 'dir' => true, + 'modifiedIndex' => 124, 'nodes' => [ [ 'key' => 'b', 'value' => json_encode( [ 'val' => true ] ), + 'modifiedIndex' => 123, + ], [ 'key' => 'c', 'value' => json_encode( [ 'val' => false ] ), + 'modifiedIndex' => 123, ], ], ], ] ] ] ), 'error' => '', ], - 'expect' => [ - [ 'a/b' => true, 'a/c' => false ], // data - null, - false // retry - ], + 'expect' => self::createEtcdResponse( [ + 'config' => [ 'a/b' => true, 'a/c' => false ], // data + 'modifiedIndex' => 123 // largest modified index + ] ), ], '200 OK - Missing nodes at second level' => [ 'http' => [ @@ -451,15 +481,14 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { [ 'key' => '/example/a', 'dir' => true, + 'modifiedIndex' => 0, ], ] ] ] ), 'error' => '', ], - 'expect' => [ - null, - "Unexpected JSON response in dir 'a'; missing 'nodes' list.", - false // retry - ], + 'expect' => self::createEtcdResponse( [ + 'error' => "Unexpected JSON response in dir 'a'; missing 'nodes' list.", + ] ), ], '200 OK - Directory with non-array "nodes" key' => [ 'http' => [ @@ -475,11 +504,9 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { ] ] ] ), 'error' => '', ], - 'expect' => [ - null, - "Unexpected JSON response in dir 'a'; 'nodes' is not an array.", - false // retry - ], + 'expect' => self::createEtcdResponse( [ + 'error' => "Unexpected JSON response in dir 'a'; 'nodes' is not an array.", + ] ), ], '200 OK - Correctly encoded garbage response' => [ 'http' => [ @@ -489,11 +516,9 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'body' => json_encode( [ 'foo' => 'bar' ] ), 'error' => '', ], - 'expect' => [ - null, - "Unexpected JSON response: Missing or invalid node at top level.", - false // retry - ], + 'expect' => self::createEtcdResponse( [ + 'error' => "Unexpected JSON response: Missing or invalid node at top level.", + ] ), ], '200 OK - Bad value' => [ 'http' => [ @@ -503,30 +528,27 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'body' => json_encode( [ 'node' => [ 'nodes' => [ [ 'key' => '/example/foo', - 'value' => ';"broken{value' + 'value' => ';"broken{value', + 'modifiedIndex' => 123, ] ] ] ] ), 'error' => '', ], - 'expect' => [ - null, // data - "Failed to parse value for 'foo'.", - false // retry - ], + 'expect' => self::createEtcdResponse( [ + 'error' => "Failed to parse value for 'foo'.", + ] ), ], '200 OK - Empty node list' => [ 'http' => [ 'code' => 200, 'reason' => 'OK', 'headers' => [], - 'body' => '{"node":{"nodes":[]}}', + 'body' => '{"node":{"nodes":[], "modifiedIndex": 12 }}', 'error' => '', ], - 'expect' => [ - [], // data - null, - false // retry - ], + 'expect' => self::createEtcdResponse( [ + 'config' => [], // data + ] ), ], '200 OK - Invalid JSON' => [ 'http' => [ @@ -536,11 +558,9 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'body' => '', 'error' => '(curl error: no status set)', ], - 'expect' => [ - null, // data - "Error unserializing JSON response.", - false // retry - ], + 'expect' => self::createEtcdResponse( [ + 'error' => "Error unserializing JSON response.", + ] ), ], '404 Not Found' => [ 'http' => [ @@ -550,11 +570,9 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'body' => '', 'error' => '', ], - 'expect' => [ - null, // data - 'HTTP 404 (Not Found)', - false // retry - ], + 'expect' => self::createEtcdResponse( [ + 'error' => 'HTTP 404 (Not Found)', + ] ), ], '400 Bad Request - custom error' => [ 'http' => [ @@ -564,11 +582,10 @@ class EtcdConfigTest extends PHPUnit\Framework\TestCase { 'body' => '', 'error' => 'No good reason', ], - 'expect' => [ - null, // data - 'No good reason', - true // retry - ], + 'expect' => self::createEtcdResponse( [ + 'error' => 'No good reason', + 'retry' => true, // retry + ] ), ], ]; }