Merge lp:~brian-murray/ubuntu-release-upgrader/better-uri-testing into lp:ubuntu-release-upgrader

[Brian Murray]

As a part of improving the release upgrade process for people with PPAs enabled a check was added to see if the release to which we are upgrading is provided by the archive (LP: #1807043). However, this check is done for every entry and its uri in a sources.list file which could result in a situation, if a network was flakey, where main was enabled and universe was not (LP: #1822886).

If we instead have a dictionary of the entry.uris and whether or not they were accessible all the sources.list entries with that uri will be disabled. In the case of the Ubuntu archive this will result in 'ubuntu-minimal' not being downloadable and the upgrade process will abort. This is better than the upgrade proceeding and some components for an archive being disabled. Additionally, the multiple calls to _sourcesListEntryDownloadable were unnecessary.

[lp:~brian-murray/ubuntu-release-upgrader/better-uri-testing]

3227. By Brian Murray on 2019-04-03

add in a test where getting the Release file fails, if entry.uri is rewritten add new entry.uri to entry_uri_test_results

[Steve Langasek (vorlon)]

review: Needs Fixing

[lp:~brian-murray/ubuntu-release-upgrader/better-uri-testing]

3228. By Brian Murray on 2019-04-04

merge Steve's code refactor to reduce code duplication

3229. By Brian Murray on 2019-04-04

remove sources.list file on tear down e.g. when a test fails

3230. By Brian Murray on 2019-04-04

give new test a unique name

3231. By Brian Murray on 2019-04-05

add in a changelog entry

=== modified file 'DistUpgrade/DistUpgradeController.py'
--- DistUpgrade/DistUpgradeController.py	2019-03-08 22:41:57 +0000
+++ DistUpgrade/DistUpgradeController.py	2019-04-05 00:10:53 +0000
@@ -590,7 +590,10 @@
         # collect information on what components (main,universe) are enabled for what distro (sub)version
         # e.g. found_components = { 'hardy':set("main","restricted"), 'hardy-updates':set("main") }
         self.found_components = {}
+        entry_uri_test_results = {}
         for entry in self.sources.list[:]:
+            if entry.uri not in entry_uri_test_results:
+                entry_uri_test_results[entry.uri] = 'unknown'
 
             # ignore invalid records or disabled ones
             if entry.invalid or entry.disabled:
@@ -661,6 +664,8 @@
                     if self._sourcesListEntryDownloadable(test_entry):
                         logging.info("transition from old-release.u.c to %s" % uri)
                         entry.uri = uri
+                        if entry.uri not in entry_uri_test_results:
+                            entry_uri_test_results[entry.uri] = 'unknown'
                         break
 
             logging.debug("examining: '%s'" %  get_string_with_no_auth_from_source_entry(entry))
@@ -686,11 +691,16 @@
                     logging.debug("entry '%s' is already set to new dist" % get_string_with_no_auth_from_source_entry(entry))
                     foundToDist |= validTo
                 elif entry.dist in fromDists:
-                    foundToDist |= validTo
-                    # check to see whether the archive provides the new dist
-                    test_entry = copy.copy(entry)
-                    test_entry.dist = self.toDist
-                    if not self._sourcesListEntryDownloadable(test_entry):
+                    if entry_uri_test_results[entry.uri] == 'unknown':
+                        foundToDist |= validTo
+                        # check to see whether the archive provides the new dist
+                        test_entry = copy.copy(entry)
+                        test_entry.dist = self.toDist
+                        if not self._sourcesListEntryDownloadable(test_entry):
+                            entry_uri_test_results[entry.uri] = 'failed'
+                        else:
+                            entry_uri_test_results[entry.uri] = 'passed'
+                    if entry_uri_test_results[entry.uri] == 'failed':
                         entry.disabled = True
                         self.sources_disabled = True
                         logging.debug("entry '%s' was disabled (no Release file)" % get_string_with_no_auth_from_source_entry(entry))

=== modified file 'debian/changelog'
--- debian/changelog	2019-03-14 18:02:37 +0000
+++ debian/changelog	2019-04-05 00:10:53 +0000
@@ -1,3 +1,12 @@
+ubuntu-release-upgrader (1:19.04.14) UNRELEASED; urgency=medium
+
+  * DistUpgradeController.py: Instead of checking every sources.list entry to
+    see if they are accessible keep track of the uris and only test once per
+    uri thereby preventing situations where entries for the same server are
+    enabled and disabled. (LP: #1822886)
+
+ -- Brian Murray <[e-mail]>  Thu, 04 Apr 2019 17:06:26 -0700
+
 ubuntu-release-upgrader (1:19.04.13) disco; urgency=medium
 
   [ Brian Murray ]

=== modified file 'tests/test_sources_list.py'
--- tests/test_sources_list.py	2018-12-11 00:28:15 +0000
+++ tests/test_sources_list.py	2019-04-05 00:10:53 +0000
@@ -72,6 +72,10 @@
             os.unlink(os.path.join(self.testdir, "sources.list"))
         apt_pkg.config.set("APT::Default-Release", "")
 
+    def tearDown(self):
+        if os.path.exists(os.path.join(self.testdir, "sources.list")):
+            os.unlink(os.path.join(self.testdir, "sources.list"))
+
     def test_sources_list_with_nothing(self):
         """
         test sources.list rewrite with nothing in it
@@ -131,6 +135,39 @@
              apt_pkg.config.find_file("Dir::Etc::sourcelist") + ".distUpgrade"
              ]))
 
+    @mock.patch("DistUpgrade.DistUpgradeController.DistUpgradeController._sourcesListEntryDownloadable")
+    @mock.patch("DistUpgrade.DistUpgradeController.get_distro")
+    def test_sources_list_rewrite_no_network(self, mock_get_distro, mock_sourcesListEntryDownloadable):
+        """
+        test sources.list rewrite no network
+        """
+        shutil.copy(os.path.join(self.testdir, "sources.list.in"),
+                    os.path.join(self.testdir, "sources.list"))
+        apt_pkg.config.set("Dir::Etc::sourcelist", "sources.list")
+        v = DistUpgradeViewNonInteractive()
+        d = DistUpgradeController(v, datadir=self.testdir)
+        d.config.set("Distro", "BaseMetaPkgs", "ubuntu-minimal")
+        mock_get_distro.return_value = UbuntuDistribution("Ubuntu", "feisty",
+                                                          "Ubuntu Feisty Fawn",
+                                                          "7.04")
+        d.openCache(lock=False)
+        mock_sourcesListEntryDownloadable.return_value = False
+        res = d.updateSourcesList()
+        self.assertTrue(mock_get_distro.called)
+        self.assertTrue(mock_sourcesListEntryDownloadable.called)
+        self.assertTrue(res)
+
+        # now test the result
+        #print(open(os.path.join(self.testdir,"sources.list")).read())
+        self._verifySources("""
+# main repo
+# deb http://archive.ubuntu.com/ubuntu/ feisty main restricted multiverse universe
+# deb http://de.archive.ubuntu.com/ubuntu/ feisty main restricted multiverse
+# deb-src http://archive.ubuntu.com/ubuntu/ feisty main restricted multiverse
+# deb http://security.ubuntu.com/ubuntu/ feisty-security main restricted
+# deb http://security.ubuntu.com/ubuntu/ feisty-security universe
+""")
+
     @mock.patch("DistUpgrade.DistUpgradeController.DistUpgradeController.abort")
     @mock.patch("DistUpgrade.DistUpgradeController.get_distro")
     def test_double_check_source_distribution_reject(self, mock_abort, mock_get_distro):
@@ -424,8 +461,6 @@
 
     def testEOL2EOLUpgrades(self):
         " test upgrade from EOL release to EOL release "
-        v = DistUpgradeViewNonInteractive()
-        d = DistUpgradeController(v, datadir=self.testdir)
         shutil.copy(os.path.join(self.testdir, "sources.list.EOL"),
                     os.path.join(self.testdir, "sources.list"))
         apt_pkg.config.set("Dir::Etc::sourceparts",
@@ -454,8 +489,6 @@
         # data center, unlike most mirrors.  This lets this test pass when
         # when run in their Jenkins test environment.
         os.environ["LANG"] = "en_US.UTF-8"
-        v = DistUpgradeViewNonInteractive()
-        d = DistUpgradeController(v, datadir=self.testdir)
         shutil.copy(os.path.join(self.testdir, "sources.list.EOL2Supported"),
                     os.path.join(self.testdir, "sources.list"))
         apt_pkg.config.set("Dir::Etc::sourceparts",
@@ -624,7 +657,7 @@
     @mock.patch("DistUpgrade.DistUpgradeController.DistUpgradeController._sourcesListEntryDownloadable")
     def test_local_mirror(self, mock_sourcesListEntryDownloadable):
         """
-        test that a local mirror with official -backports works (LP:# 1067393)
+        test that a local mirror with official -backports works (LP: #1067393)
         """
         shutil.copy(os.path.join(self.testdir, "sources.list.local"),
                     os.path.join(self.testdir, "sources.list"))