Merge lp:~james-w/launchpad/drop-filecmp into lp:launchpad

[James Westby]

Drop the use of filecmp from generate_ppa_htaccess

= Summary =

When looking at generate_ppa_htaccess we noticed a pessimism
for the common case that we suspect will be adding extra
I/O operations to the run of the script, and so not be helping
with germanium's load.

This branch drops that, and moves to just overwriting the current
.htpasswd file in all cases, even if it wouldn't change the content.

== Proposed fix ==

Do that.

== Pre-implementation notes ==

None.

== LOC Rationale ==

-14 lines.

== Implementation details ==

Deletes the code. Also drops the return value and the removal
of the file, because it will now always be renamed.

== Tests ==

Adjusts the test case for the old behaviour to just test
the new behaviour.

== Demo and Q/A ==

A run of generate_ppa_htaccess with a new subscriber for a
private PPA should suffice.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py
  lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py

[Robert Collins (lifeless)]

Do we have data on this? Replacing an unaltered file will cause apache to re-read it, which will increase load. That is, I believe, why the cmp is in there in the first place. Have we got data on how long the cmp is taking?

I ask because this is exactly the sort of small tweak that can bite us rather hard.

Perhaps doing this under a feature flag will let it be phase in under examination and see if it has any impact.

review: Needs Fixing

[James Westby (james-w)]

On Wed, 30 May 2012 22:35:20 -0000, Robert Collins <email address hidden> wrote:
> Review: Needs Fixing
>
> Do we have data on this? Replacing an unaltered file will cause apache
> to re-read it, which will increase load. That is, I believe, why the
> cmp is in there in the first place. Have we got data on how long the
> cmp is taking?

Good point. We have no data at this point, this was just something we
thought might help, but just based on considering the write side of
things, and not apache reading the files.

> I ask because this is exactly the sort of small tweak that can bite us rather hard.

Yeah, but so can performance optimisations done without data, such as
checking if the file has changed before overwriting it :-)

> Perhaps doing this under a feature flag will let it be phase in under
> examination and see if it has any impact.

If a feature flag is acceptable for this sort of thing then I agree that
it would be preferable to landing it without a trivial way to rollback
if it has an adverse affect.

Other ways to do that would be adding a config option, or a command line
option. A feature flag would definitely be cheaper if it is an
acceptable use for them.

Thanks,

James

[Robert Collins (lifeless)]

Feature flags are designed for this. I don't know if the cmp was added
based on data or speculation; its speculation that it was added on
speculation :)

Anyhow, lets feature flag this up, and get the load graphs on
germanium working before we test it, and subsequent to that we can
start to see if it is a net win, loss, or noise.

Unmerged revisions

15228. By James Westby on 2012-05-30

Drop the use of filecmp in generate_ppa_htaccess.

The common case in this script is for the file to have changed, as
it carefully looks for changed PPAs/tokens. Therefore confirming
that with filecmp is just extra I/O in the common case, and that
is certainly undesirable currently on germanium.

=== modified file 'lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2012-05-10 21:44:21 +0000
+++ lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py	2012-05-30 17:22:22 +0000
@@ -9,7 +9,6 @@
     datetime,
     timedelta,
     )
-import filecmp
 import os
 import tempfile
 
@@ -104,21 +103,17 @@
         :return: True if the file was replaced.
         """
         if self.options.dryrun:
-            return False
+            os.remove(temp_htpasswd_file)
+            return
 
         # The publisher Config object does not have an
         # interface, so we need to remove the security wrapper.
         pub_config = getPubConfig(ppa)
         htpasswd_filename = os.path.join(pub_config.htaccessroot, ".htpasswd")
 
-        if (not os.path.isfile(htpasswd_filename) or
-            not filecmp.cmp(htpasswd_filename, temp_htpasswd_file)):
-            # Atomically replace the old file or create a new file.
-            os.rename(temp_htpasswd_file, htpasswd_filename)
-            self.logger.debug("Replaced htpasswd for %s" % ppa.displayname)
-            return True
-
-        return False
+        # Atomically replace the old file or create a new file.
+        os.rename(temp_htpasswd_file, htpasswd_filename)
+        self.logger.debug("Replaced htpasswd for %s" % ppa.displayname)
 
     def sendCancellationEmail(self, token):
         """Send an email to the person whose subscription was cancelled."""
@@ -323,8 +318,7 @@
 
             self.ensureHtaccess(ppa)
             temp_htpasswd = self.generateHtpasswd(ppa, valid_tokens)
-            if not self.replaceUpdatedHtpasswd(ppa, temp_htpasswd):
-                os.remove(temp_htpasswd)
+            self.replaceUpdatedHtpasswd(ppa, temp_htpasswd)
 
         if self.options.no_deactivation or self.options.dryrun:
             self.logger.info('Dry run, so not committing transaction.')

=== modified file 'lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py'
--- lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2012-05-10 21:44:21 +0000
+++ lib/lp/archivepublisher/tests/test_generate_ppa_htaccess.py	2012-05-30 17:22:22 +0000
@@ -163,8 +163,7 @@
         os.remove(filename)
 
     def testReplaceUpdatedHtpasswd(self):
-        """Test that the htpasswd file is only replaced if it changes."""
-        FILE_CONTENT = "Kneel before Zod!"
+        """Test that the htpasswd file is replaced."""
         # The publisher Config object does not have an interface, so we
         # need to remove the security wrapper.
         pub_config = getPubConfig(self.ppa)
@@ -174,28 +173,21 @@
         if not os.path.exists(pub_config.htaccessroot):
             os.makedirs(pub_config.htaccessroot)
         file = open(filename, "w")
-        file.write(FILE_CONTENT)
+        file.write("Kneel before Zod!")
         file.close()
 
         # Write the same contents in a temp file.
+        expected_content = "Come to me, son of Jor-El!"
         fd, temp_filename = tempfile.mkstemp(dir=pub_config.htaccessroot)
         file = os.fdopen(fd, "w")
-        file.write(FILE_CONTENT)
+        file.write(expected_content)
         file.close()
 
-        # Replacement should not happen.
         script = self.getScript()
-        self.assertFalse(
-            script.replaceUpdatedHtpasswd(self.ppa, temp_filename))
-
-        # Writing a different .htpasswd should see it get replaced.
-        file = open(filename, "w")
-        file.write("Come to me, son of Jor-El!")
-        file.close()
-
-        self.assertTrue(
-            script.replaceUpdatedHtpasswd(self.ppa, temp_filename))
-
+        script.replaceUpdatedHtpasswd(self.ppa, temp_filename)
+
+        with open(filename, 'r') as f:
+            self.assertEqual(expected_content, f.read())
         os.remove(filename)
 
     def assertDeactivated(self, token):