r109428 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r109427‎ | r109428 | r109429 >
Date:19:57, 18 January 2012
Author:aaron
Status:ok (Comments)
Tags:filebackend 
Comment:
In SwiftFileBackend:
* r108944: doCleanInternal() should delete empty containers only if the container dir was given.
* Made doSecureInternal() set container permissions. Also renamed swiftProxyUser -> swiftAnonUser.
* Made doGetFileStat() respect the 'latest' parameter (using r109235).
* Fixed connTTL default and renamed it to authTTL. Also added explicit close() call to getConnection().
* Reduced RTTs in doPrepareInternal() by checking getContainer() first (which is process cached).
* Killed an RTT in doStoreInternal(), doCreateInternal(), and doCopyInternal() by using create_object(). Also cleaned up logic with regards to the destination CF_Object object fields getting preloaded before write().
* Cleanups to getLocalCopy(); only create the tmp file if get_object() succeeds to short-circuits things.
* Made getContainer() limit the container cache size for sanity.
* Simplified doDeleteInternal() code a bit.
* Renamed $destRel => $dstRel for consistency.
Modified paths:
  • /trunk/phase3/includes/filerepo/backend/SwiftFileBackend.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/filerepo/backend/SwiftFileBackend.php
——@@ -9,9 +9,9 @@
1010 /**
1111 * Class for an OpenStack Swift based file backend.
1212 *
13 - * This requires that the php-cloudfiles library is present,
14 - * which is available at https://github.com/rackspace/php-cloudfiles.
15 - * All of the library classes must be registed in $wgAutoloadClasses.
 13+ * This requires the SwiftCloudFiles MediaWiki extension, which includes
 14+ * the php-cloudfiles library (https://github.com/rackspace/php-cloudfiles).
 15+ * php-cloudfiles requires the curl, fileinfo, and mb_string PHP extensions.
1616 *
1717 * Status messages should avoid mentioning the Swift account name
1818 * Likewise, error suppression should be used to avoid path disclosure.
——@@ -22,35 +22,40 @@
2323 class SwiftFileBackend extends FileBackend {
2424 /** @var CF_Authentication */
2525 protected $auth; // Swift authentication handler
 26+ protected $authTTL; // integer seconds
 27+ protected $swiftAnonUser; // string; username to handle unauthenticated requests
 28+ protected $maxContCacheSize = 20; // integer; max containers with entries
2629
2730 /** @var CF_Connection */
2831 protected $conn; // Swift connection handle
29 - protected $connTTL = 120; // integer seconds
3032 protected $connStarted = 0; // integer UNIX timestamp
3133 protected $connContainers = array(); // container object cache
3234
33 - protected $swiftProxyUser; // string
34 -
3535 /**
3636 * @see FileBackend::__construct()
3737 * Additional $config params include:
3838 * swiftAuthUrl : Swift authentication server URL
39 - * swiftUser : Swift user used by MediaWiki
 39+ * swiftUser : Swift user used by MediaWiki (account:username)
4040 * swiftKey : Swift authentication key for the above user
41 - * swiftProxyUser : Swift user used for end-user hits to proxy server
 41+ * swiftAuthTTL : Swift authentication TTL (seconds)
 42+ * swiftAnonUser : Swift user used for end-user requests (account:username)
4243 * shardViaHashLevels : Map of container names to the number of hash levels
4344 */
4445 public function __construct( array $config ) {
4546 parent::__construct( $config );
4647 // Required settings
4748 $this->auth = new CF_Authentication(
48 - $config['swiftUser'], $config['swiftKey'], null, $config['swiftAuthUrl'] );
 49+ $config['swiftUser'],
 50+ $config['swiftKey'],
 51+ null, // account; unused
 52+ $config['swiftAuthUrl']
 53+ );
4954 // Optional settings
50 - $this->connTTL = isset( $config['connTTL'] )
51 - ? $config['connTTL']
52 - : 60; // some sane number
53 - $this->swiftProxyUser = isset( $config['swiftProxyUser'] )
54 - ? $config['swiftProxyUser']
 55+ $this->authTTL = isset( $config['swiftAuthTTL'] )
 56+ ? $config['authTTL']
 57+ : 120; // some sane number
 58+ $this->swiftAnonUser = isset( $config['swiftAnonUser'] )
 59+ ? $config['swiftAnonUser']
5560 : '';
5661 $this->shardViaHashLevels = isset( $config['shardViaHashLevels'] )
5762 ? $config['shardViaHashLevels']
——@@ -73,15 +78,23 @@
7479 protected function doCreateInternal( array $params ) {
7580 $status = Status::newGood();
7681
77 - list( $dstCont, $destRel ) = $this->resolveStoragePathReal( $params['dst'] );
78 - if ( $destRel === null ) {
 82+ list( $dstCont, $dstRel ) = $this->resolveStoragePathReal( $params['dst'] );
 83+ if ( $dstRel === null ) {
7984 $status->fatal( 'backend-fail-invalidpath', $params['dst'] );
8085 return $status;
8186 }
8287
83 - // (a) Check the destination container
 88+ // (a) Check the destination container and object
8489 try {
8590 $dContObj = $this->getContainer( $dstCont );
 91+ if ( empty( $params['overwriteDest'] ) ) {
 92+ $destObj = $dContObj->create_object( $dstRel );
 93+ // Check if the object already exists (fields populated)
 94+ if ( $destObj->last_modified ) {
 95+ $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
 96+ return $status;
 97+ }
 98+ }
8699 } catch ( NoSuchContainerException $e ) {
87100 $status->fatal( 'backend-fail-create', $params['dst'] );
88101 return $status;
——@@ -94,31 +107,14 @@
95108 return $status;
96109 }
97110
98 - // (b) Check if the destination object already exists
99 - try {
100 - $dContObj->get_object( $destRel ); // throws NoSuchObjectException
101 - // NoSuchObjectException not thrown: file must exist
102 - if ( empty( $params['overwriteDest'] ) ) {
103 - $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
104 - return $status;
105 - }
106 - } catch ( NoSuchObjectException $e ) {
107 - // NoSuchObjectException thrown: file does not exist
108 - } catch ( InvalidResponseException $e ) {
109 - $status->fatal( 'backend-fail-connect', $this->name );
110 - return $status;
111 - } catch ( Exception $e ) { // some other exception?
112 - $status->fatal( 'backend-fail-internal', $this->name );
113 - $this->logException( $e, __METHOD__, $params );
114 - return $status;
115 - }
116 -
117 - // (c) Get a SHA-1 hash of the object
 111+ // (b) Get a SHA-1 hash of the object
118112 $sha1Hash = wfBaseConvert( sha1( $params['content'] ), 16, 36, 31 );
119113
120 - // (d) Actually create the object
 114+ // (c) Actually create the object
121115 try {
122 - $obj = $dContObj->create_object( $destRel );
 116+ // Create a fresh CF_Object with no fields preloaded.
 117+ // We don't want to preserve headers, metadata, and such.
 118+ $obj = new CF_Object( $dContObj, $dstRel, false, false ); // skip HEAD
123119 // Note: metadata keys stored as [Upper case char][[Lower case char]...]
124120 $obj->metadata = array( 'Sha1base36' => $sha1Hash );
125121 $obj->write( $params['content'] );
——@@ -140,15 +136,23 @@
141137 protected function doStoreInternal( array $params ) {
142138 $status = Status::newGood();
143139
144 - list( $dstCont, $destRel ) = $this->resolveStoragePathReal( $params['dst'] );
145 - if ( $destRel === null ) {
 140+ list( $dstCont, $dstRel ) = $this->resolveStoragePathReal( $params['dst'] );
 141+ if ( $dstRel === null ) {
146142 $status->fatal( 'backend-fail-invalidpath', $params['dst'] );
147143 return $status;
148144 }
149145
150 - // (a) Check the destination container
 146+ // (a) Check the destination container and object
151147 try {
152148 $dContObj = $this->getContainer( $dstCont );
 149+ if ( empty( $params['overwriteDest'] ) ) {
 150+ $destObj = $dContObj->create_object( $dstRel );
 151+ // Check if the object already exists (fields populated)
 152+ if ( $destObj->last_modified ) {
 153+ $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
 154+ return $status;
 155+ }
 156+ }
153157 } catch ( NoSuchContainerException $e ) {
154158 $status->fatal( 'backend-fail-copy', $params['src'], $params['dst'] );
155159 return $status;
——@@ -161,26 +165,7 @@
162166 return $status;
163167 }
164168
165 - // (b) Check if the destination object already exists
166 - try {
167 - $dContObj->get_object( $destRel ); // throws NoSuchObjectException
168 - // NoSuchObjectException not thrown: file must exist
169 - if ( empty( $params['overwriteDest'] ) ) {
170 - $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
171 - return $status;
172 - }
173 - } catch ( NoSuchObjectException $e ) {
174 - // NoSuchObjectException thrown: file does not exist
175 - } catch ( InvalidResponseException $e ) {
176 - $status->fatal( 'backend-fail-connect', $this->name );
177 - return $status;
178 - } catch ( Exception $e ) { // some other exception?
179 - $status->fatal( 'backend-fail-internal', $this->name );
180 - $this->logException( $e, __METHOD__, $params );
181 - return $status;
182 - }
183 -
184 - // (c) Get a SHA-1 hash of the object
 169+ // (b) Get a SHA-1 hash of the object
185170 $sha1Hash = sha1_file( $params['src'] );
186171 if ( $sha1Hash === false ) { // source doesn't exist?
187172 $status->fatal( 'backend-fail-copy', $params['src'], $params['dst'] );
——@@ -188,9 +173,11 @@
189174 }
190175 $sha1Hash = wfBaseConvert( $sha1Hash, 16, 36, 31 );
191176
192 - // (d) Actually store the object
 177+ // (c) Actually store the object
193178 try {
194 - $obj = $dContObj->create_object( $destRel );
 179+ // Create a fresh CF_Object with no fields preloaded.
 180+ // We don't want to preserve headers, metadata, and such.
 181+ $obj = new CF_Object( $dContObj, $dstRel, false, false ); // skip HEAD
195182 // Note: metadata keys stored as [Upper case char][[Lower case char]...]
196183 $obj->metadata = array( 'Sha1base36' => $sha1Hash );
197184 $obj->load_from_filename( $params['src'], True ); // calls $obj->write()
——@@ -220,16 +207,24 @@
221208 return $status;
222209 }
223210
224 - list( $dstCont, $destRel ) = $this->resolveStoragePathReal( $params['dst'] );
225 - if ( $destRel === null ) {
 211+ list( $dstCont, $dstRel ) = $this->resolveStoragePathReal( $params['dst'] );
 212+ if ( $dstRel === null ) {
226213 $status->fatal( 'backend-fail-invalidpath', $params['dst'] );
227214 return $status;
228215 }
229216
230 - // (a) Check the source and destination containers
 217+ // (a) Check the source/destination containers and destination object
231218 try {
232219 $sContObj = $this->getContainer( $srcCont );
233220 $dContObj = $this->getContainer( $dstCont );
 221+ if ( empty( $params['overwriteDest'] ) ) {
 222+ $destObj = $dContObj->create_object( $dstRel );
 223+ // Check if the object already exists (fields populated)
 224+ if ( $destObj->last_modified ) {
 225+ $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
 226+ return $status;
 227+ }
 228+ }
234229 } catch ( NoSuchContainerException $e ) {
235230 $status->fatal( 'backend-fail-copy', $params['src'], $params['dst'] );
236231 return $status;
——@@ -242,28 +237,9 @@
243238 return $status;
244239 }
245240
246 - // (b) Check if the destination object already exists
 241+ // (b) Actually copy the file to the destination
247242 try {
248 - $dContObj->get_object( $destRel ); // throws NoSuchObjectException
249 - // NoSuchObjectException not thrown: file must exist
250 - if ( empty( $params['overwriteDest'] ) ) {
251 - $status->fatal( 'backend-fail-alreadyexists', $params['dst'] );
252 - return $status;
253 - }
254 - } catch ( NoSuchObjectException $e ) {
255 - // NoSuchObjectException thrown: file does not exist
256 - } catch ( InvalidResponseException $e ) {
257 - $status->fatal( 'backend-fail-connect', $this->name );
258 - return $status;
259 - } catch ( Exception $e ) { // some other exception?
260 - $status->fatal( 'backend-fail-internal', $this->name );
261 - $this->logException( $e, __METHOD__, $params );
262 - return $status;
263 - }
264 -
265 - // (c) Actually copy the file to the destination
266 - try {
267 - $sContObj->copy_object_to( $srcRel, $dContObj, $destRel );
 243+ $sContObj->copy_object_to( $srcRel, $dContObj, $dstRel );
268244 } catch ( NoSuchObjectException $e ) { // source object does not exist
269245 $status->fatal( 'backend-fail-copy', $params['src'], $params['dst'] );
270246 } catch ( InvalidResponseException $e ) {
——@@ -288,24 +264,11 @@
289265 return $status;
290266 }
291267
292 - // (a) Check the source container
293268 try {
294269 $sContObj = $this->getContainer( $srcCont );
 270+ $sContObj->delete_object( $srcRel );
295271 } catch ( NoSuchContainerException $e ) {
296272 $status->fatal( 'backend-fail-delete', $params['src'] );
297 - return $status;
298 - } catch ( InvalidResponseException $e ) {
299 - $status->fatal( 'backend-fail-connect', $this->name );
300 - return $status;
301 - } catch ( Exception $e ) { // some other exception?
302 - $status->fatal( 'backend-fail-internal', $this->name );
303 - $this->logException( $e, __METHOD__, $params );
304 - return $status;
305 - }
306 -
307 - // (b) Actually delete the object
308 - try {
309 - $sContObj->delete_object( $srcRel );
310273 } catch ( NoSuchObjectException $e ) {
311274 if ( empty( $params['ignoreMissingSource'] ) ) {
312275 $status->fatal( 'backend-fail-delete', $params['src'] );
——@@ -326,15 +289,42 @@
327290 protected function doPrepareInternal( $fullCont, $dir, array $params ) {
328291 $status = Status::newGood();
329292
 293+ // (a) Check if container already exists
330294 try {
331 - $this->createContainer( $fullCont );
 295+ $contObj = $this->getContainer( $fullCont );
 296+ // NoSuchContainerException not thrown: container must exist
 297+ return $status; // already exists
 298+ } catch ( NoSuchContainerException $e ) {
 299+ // NoSuchContainerException thrown: container does not exist
332300 } catch ( InvalidResponseException $e ) {
333301 $status->fatal( 'backend-fail-connect', $this->name );
 302+ return $status;
334303 } catch ( Exception $e ) { // some other exception?
335304 $status->fatal( 'backend-fail-internal', $this->name );
336305 $this->logException( $e, __METHOD__, $params );
 306+ return $status;
337307 }
338308
 309+ // (b) Create container as needed
 310+ try {
 311+ $contObj = $this->createContainer( $fullCont );
 312+ if ( $this->swiftAnonUser != '' ) {
 313+ // Make container public to end-users...
 314+ $status->merge( $this->setContainerAccess(
 315+ $contObj,
 316+ array( $this->auth->username, $this->swiftAnonUser ), // read
 317+ array( $this->auth->username ) // write
 318+ ) );
 319+ }
 320+ } catch ( InvalidResponseException $e ) {
 321+ $status->fatal( 'backend-fail-connect', $this->name );
 322+ return $status;
 323+ } catch ( Exception $e ) { // some other exception?
 324+ $status->fatal( 'backend-fail-internal', $this->name );
 325+ $this->logException( $e, __METHOD__, $params );
 326+ return $status;
 327+ }
 328+
339329 return $status;
340330 }
341331
——@@ -343,7 +333,32 @@
344334 */
345335 protected function doSecureInternal( $fullCont, $dir, array $params ) {
346336 $status = Status::newGood();
347 - // @TODO: restrict container from $this->swiftProxyUser
 337+
 338+ if ( $this->swiftAnonUser != '' ) {
 339+ // Restrict container from end-users...
 340+ try {
 341+ // doPrepareInternal() should have been called,
 342+ // so the Swift container should already exist...
 343+ $contObj = $this->getContainer( $fullCont ); // normally a cache hit
 344+ // NoSuchContainerException not thrown: container must exist
 345+ if ( !isset( $contObj->mw_wasSecured ) ) {
 346+ $status->merge( $this->setContainerAccess(
 347+ $contObj,
 348+ array( $this->auth->username ), // read
 349+ array( $this->auth->username ) // write
 350+ ) );
 351+ // @TODO: when php-cloudfiles supports container
 352+ // metadata, we can make use of that to avoid RTTs
 353+ $contObj->mw_wasSecured = true; // avoid useless RTTs
 354+ }
 355+ } catch ( InvalidResponseException $e ) {
 356+ $status->fatal( 'backend-fail-connect', $this->name );
 357+ } catch ( Exception $e ) { // some other exception?
 358+ $status->fatal( 'backend-fail-internal', $this->name );
 359+ $this->logException( $e, __METHOD__, $params );
 360+ }
 361+ }
 362+
348363 return $status;
349364 }
350365
——@@ -353,6 +368,11 @@
354369 protected function doCleanInternal( $fullCont, $dir, array $params ) {
355370 $status = Status::newGood();
356371
 372+ // Only containers themselves can be removed, all else is virtual
 373+ if ( $dir != '' ) {
 374+ return $status; // nothing to do
 375+ }
 376+
357377 // (a) Check the container
358378 try {
359379 $contObj = $this->getContainer( $fullCont, true );
——@@ -397,16 +417,15 @@
398418
399419 $stat = false;
400420 try {
401 - $container = $this->getContainer( $srcCont );
402 - // @TODO: handle 'latest' param as "X-Newest: true"
403 - $obj = $container->get_object( $srcRel );
 421+ $contObj = $this->getContainer( $srcCont );
 422+ $srcObj = $contObj->get_object( $srcRel, $this->headersFromParams( $params ) );
404423 // Convert dates like "Tue, 03 Jan 2012 22:01:04 GMT" to TS_MW
405 - $date = DateTime::createFromFormat( 'D, d F Y G:i:s e', $obj->last_modified );
 424+ $date = DateTime::createFromFormat( 'D, d F Y G:i:s e', $srcObj->last_modified );
406425 if ( $date ) {
407426 $stat = array(
408427 'mtime' => $date->format( 'YmdHis' ),
409 - 'size' => $obj->content_length,
410 - 'sha1' => $obj->metadata['Sha1base36']
 428+ 'size' => $srcObj->content_length,
 429+ 'sha1' => $srcObj->metadata['Sha1base36']
411430 );
412431 } else { // exception will be caught below
413432 throw new Exception( "Could not parse date for object {$srcRel}" );
——@@ -465,6 +484,7 @@
466485 */
467486 public function getFileListPageInternal( $fullCont, $dir, $after, $limit ) {
468487 $files = array();
 488+
469489 try {
470490 $container = $this->getContainer( $fullCont );
471491 $files = $container->list_objects( $limit, $after, "{$dir}/" );
——@@ -517,7 +537,8 @@
518538
519539 try {
520540 $output = fopen( 'php://output', 'w' );
521 - $obj = new CF_Object( $cont, $srcRel, False, False ); // skip HEAD request
 541+ // FileBackend::streamFile() already checks existence
 542+ $obj = new CF_Object( $cont, $srcRel, false, false ); // skip HEAD request
522543 $obj->stream( $output, $this->headersFromParams( $params ) );
523544 } catch ( InvalidResponseException $e ) { // 404? connection problem?
524545 $status->fatal( 'backend-fail-stream', $params['src'] );
——@@ -538,23 +559,22 @@
539560 return null;
540561 }
541562
542 - // Get source file extension
543 - $ext = FileBackend::extensionFromPath( $srcRel );
544 - // Create a new temporary file...
545 - $tmpFile = TempFSFile::factory( wfBaseName( $srcRel ) . '_', $ext );
546 - if ( !$tmpFile ) {
547 - return null;
548 - }
549 -
 563+ $tmpFile = null;
550564 try {
551565 $cont = $this->getContainer( $srcCont );
552566 $obj = $cont->get_object( $srcRel );
553 - $handle = fopen( $tmpFile->getPath(), 'w' );
554 - if ( $handle ) {
555 - $obj->stream( $handle, $this->headersFromParams( $params ) );
556 - fclose( $handle );
557 - } else {
558 - $tmpFile = null; // couldn't open temp file
 567+ // Get source file extension
 568+ $ext = FileBackend::extensionFromPath( $srcRel );
 569+ // Create a new temporary file...
 570+ $tmpFile = TempFSFile::factory( wfBaseName( $srcRel ) . '_', $ext );
 571+ if ( $tmpFile ) {
 572+ $handle = fopen( $tmpFile->getPath(), 'w' );
 573+ if ( $handle ) {
 574+ $obj->stream( $handle, $this->headersFromParams( $params ) );
 575+ fclose( $handle );
 576+ } else {
 577+ $tmpFile = null; // couldn't open temp file
 578+ }
559579 }
560580 } catch ( NoSuchContainerException $e ) {
561581 $tmpFile = null;
——@@ -587,6 +607,30 @@
588608 }
589609
590610 /**
 611+ * Set read/write permissions for a Swift container
 612+ *
 613+ * @param $contObj CF_Container Swift container
 614+ * @param $readGrps Array Swift users who can read (account:user)
 615+ * @param $writeGrps Array Swift users who can write (account:user)
 616+ * @return Status
 617+ */
 618+ protected function setContainerAccess(
 619+ CF_Container $contObj, array $readGrps, array $writeGrps
 620+ ) {
 621+ $creds = $contObj->cfs_auth->export_credentials();
 622+
 623+ $url = $creds['storage_url'] . '/' . rawurlencode( $contObj->name );
 624+
 625+ // Note: 10 second timeout consistent with php-cloudfiles
 626+ $req = new CurlHttpRequest( $url, array( 'method' => 'POST', 'timeout' => 10 ) );
 627+ $req->setHeader( 'X-Auth-Token', $creds['auth_token'] );
 628+ $req->setHeader( 'X-Container-Read', implode( ',', $readGrps ) );
 629+ $req->setHeader( 'X-Container-Write', implode( ',', $writeGrps ) );
 630+
 631+ return $req->execute(); // should return 204
 632+ }
 633+
 634+ /**
591635 * Get a connection to the Swift proxy
592636 *
593637 * @return CF_Connection|false
——@@ -596,9 +640,13 @@
597641 if ( $this->conn === false ) {
598642 return false; // failed last attempt
599643 }
600 - // Authenticate with proxy and get a session key.
601 - // Session keys expire after a while, so we renew them periodically.
602 - if ( $this->conn === null || ( time() - $this->connStarted ) > $this->connTTL ) {
 644+ // Session keys expire after a while, so we renew them periodically
 645+ if ( $this->conn && ( time() - $this->connStarted ) > $this->authTTL ) {
 646+ $this->conn->close(); // close active cURL connections
 647+ $this->conn = null;
 648+ }
 649+ // Authenticate with proxy and get a session key...
 650+ if ( $this->conn === null ) {
603651 $this->connContainers = array();
604652 try {
605653 $this->auth->authenticate();
——@@ -631,7 +679,12 @@
632680 }
633681 if ( !isset( $this->connContainers[$container] ) ) {
634682 $contObj = $conn->get_container( $container );
635 - // Exception not thrown: container must exist
 683+ // NoSuchContainerException not thrown: container must exist
 684+ if ( count( $this->connContainers ) >= $this->maxContCacheSize ) { // trim cache?
 685+ reset( $this->connContainers );
 686+ $key = key( $this->connContainers );
 687+ unset( $this->connContainers[$key] );
 688+ }
636689 $this->connContainers[$container] = $contObj; // cache it
637690 }
638691 return $this->connContainers[$container];

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r108944In SwiftFileBackend:...aaron23:11, 14 January 2012
r109235Support custom headers for HEAD operations, e.g. ("X-Newest: true"); read() a...aaron22:44, 17 January 2012

Comments

#Comment by Tim Starling (talk | contribs)   02:21, 1 February 2012

Would it be possible to move setContainerAccess() to cloudfiles after the 1.19 branch point?

Status & tagging log