Create zchunked updateinfo

[jdieter]

This patch will create both the regular and the zchunked updateinfo.xml if createrepo_c supports zchunk files.
For the tests to pass, you need librepo-1.9.3 and createrepo_c-0.12.0 with zchunk enabled and the pull request at rpm-software-management/createrepo_c#115 committed.

[bowlofeggs]

Cool, looks reasonable to me. We will need to get the coverage to 100% so that the diff cover tests also pass.

Since the if statement that isn't covered depends on having a createrepo with the new feature, I suggest adding one more test that uses mock to fake createrepo having that feature, and then asserting that the right thing happens.

Alternatively, we can wait until a version of createrepo with that feature is in all supported versions of Fedora and this patch will then pass diff cover on its own (and we can drop the if statement and just say we require that newer version of createrepo).

How would you prefer to proceed?

[bowlofeggs]

Actually, it looks like the rawhide tests are failing. For example:

https://ci.centos.org/job/bodhi-pipeline/job/PR-2904/8/testReport/junit/bodhi.tests.server.test_util/TestSanityCheckRepodata/test_comps_invalid_nonsense/

Error Message

createrepo_c.CreaterepoCError: Error while closing /tmp/tmpm_3tvr7tbodhi/repodata/updateinfo.xml: Unable to end final chunk:

Stacktrace

self = <bodhi.tests.server.test_util.TestSanityCheckRepodata testMethod=test_comps_invalid_nonsense>

    def test_comps_invalid_nonsense(self):
        """RepodataException should be raised if comps is invalid."""
        comps = os.path.join(self.tempdir, 'comps.xml')
        with open(comps, 'w') as uinfo:
            uinfo.write('<whatever />')
>       base.mkmetadatadir(self.tempdir, comps=comps)

bodhi/tests/server/test_util.py:615: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
bodhi/tests/server/base.py:392: in mkmetadatadir
    'xml', os.path.join(path, 'updateinfo.xml'))
bodhi/server/metadata.py:65: in insert_in_repo
    rec_comp = rec.compress_and_fill(cr.SHA256, ct)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <createrepo_c.RepomdRecord updateinfo_zck object>, hashtype = 5
compresstype = 6

    def compress_and_fill(self, hashtype, compresstype):
        rec = RepomdRecord(self.type + "_gz", None)
        _createrepo_c.RepomdRecord.compress_and_fill(self,
                                                     rec,
                                                     hashtype,
>                                                    compresstype)
E       createrepo_c.CreaterepoCError: Error while closing /tmp/tmpm_3tvr7tbodhi/repodata/updateinfo.xml: Unable to end final chunk:

/usr/lib64/python3.7/site-packages/createrepo_c/__init__.py:191: CreaterepoCError

Standard Error

INFO:bodhi.server.metadata:Inserting updateinfo.xml into /tmp/tmpm_3tvr7tbodhi/repodata

Does rawhide have the createrepo with this feature?

[jdieter]

Rawhide's createrepo_c is currently missing rpm-software-management/createrepo_c#115. When I did my testing, I did it on Rawhide against createrepo_c with that patch pulled in. Would it help if I pushed out a working version of createrepo_c at https://copr.fedorainfracloud.org/coprs/jdieter/dnf-zchunk/?

[jdieter]

As for the test coverage, I'd probably go with using mock to fake out createrepo_c, though I'm not sure how that would work. The problem with waiting is that createrepo_c will probably not be zchunk-enabled on <= F29.

[jdieter]

...and I've just looked up mock and realized you're not talking about the Fedora packaging tool, but the python tool. I'll take a look at it and see if I can something together. /me walks away feel slightly stupid. ;)

[jdieter]

So I've put something together that is designed to literally just go through the different code-paths. I've even had to wrap it in exception handling because the test won't actually write the file, but it's brought the coverage to 100% in bodhi/server/metadata.py.

I've also pushed a working copy of createrepo_c to https://copr.fedorainfracloud.org/coprs/jdieter/dnf-zchunk/. I'm not sure if you want to go ahead and enable this copr (for Rawhide only), or if you'd rather wait until the patches make it into Rawhide's createrepo_c and librepo.

[bowlofeggs]

On Thu, 2019-01-10 at 22:39 +0000, Jonathan Dieter wrote: So I've put something together that is designed to literally just go through the different code-paths. I've even had to wrap it in exception handling because the test won't actually write the file, but it's brought the coverage to 100% in bodhi/server/metadata.py.

Cool, I haven't had time to look at the code yet, but what you wrote here sounds reasonable to me. Since Python is a dynamic language, coverage is more important because without it things like typos can cause problems. I do have one suggestion on how we can do even better: in addition to the mocked test, we could add a test that really uses createrepo_c if it has the needed functionality, and then use a @skipif decorator to skip that test when the system createrepo_c doesn't have it. This way the first test ensures coverage, and the second test will ensure the feature actually works for distros that have it.

[jdieter]

On Thu, 2019-01-10 at 22:39 +0000, Jonathan Dieter wrote: So I've put something together that is designed to literally just go through the different code-paths. I've even had to wrap it in exception handling because the test won't actually write the file, but it's brought the coverage to 100% in bodhi/server/metadata.py.
Cool, I haven't had time to look at the code yet, but what you wrote here sounds reasonable to me. Since Python is a dynamic language, coverage is more important because without it things like typos can cause problems. I do have one suggestion on how we can do even better: in addition to the mocked test, we could add a test that really uses createrepo_c if it has the needed functionality, and then use a @skipif decorator to skip that test when the system createrepo_c doesn't have it. This way the first test ensures coverage, and the second test will ensure the feature actually works for distros that have it.

If I understand you correctly, I think I've already done this. The test I changed in bodhi/tests/server/test_metadata.py, lines 278-290 originally just checked whether the stored updateinfo.xml.xz (or whatever the normal compression is) had a checksum that matched the checksum in repomd. I extended it to test both updateinfo.xml.xz and updateinfo.xml.zck if createrepo_c supports zchunk, and the test should fail if only updateinfo.xml.xz is created.

[bowlofeggs]

We don't need to assert that mock works:

Suggested change

assert(bodhi_metadata.cr.ZCK_COMPRESSION == 99)

[jdieter]

Ack. Fixed in next commit

[bowlofeggs]

What would raise this TypeError, and the other two below?

[jdieter]

Without this, I get
E TypeError: write() argument must be str, not MagicMock
at the line: repomd_file.write(repomd.xml_dump()) in insert_in_repo() in bodhi/server/metadata.py

[bowlofeggs]

It seems to me that this probably happens because we need to do more mocking to get it to work - i.e., we need to make our mock work such that it hands out a str to that write() call.

[bowlofeggs]

We should add assertions that the right calls were made when the zchunk compression was available, otherwise all we know is that it didn't blow up.

[jdieter]

insert_in_repo() is called by UpdateInfoMetadata(), which is tested by _test_extended_metadata(), and I piggybacked the zchunk tests there.

UpdateInfoMetadata() creates both the regular and zchunked metadata, _verify_updateinfos() validates that we have two updateinfos if zchunk is enabled, and _test_extended_metadata() validates that both updateinfo files match what they're supposed to be.

So far, test_zchunk_metadata is a bit of a misnomer as all it does is make sure that we reach 100% code coverage, and doesn't really test anything.

Is there other testing we should be doing to verify that the zchunk compression is working?

[bowlofeggs]

It's true that mock testing isn't nearly as valuable as testing that actually uses the code/resources you are mocking, but there is still something being tested, which is that the code is at least valid Python. Since Python is not a static language, mocking is a way to make sure that the code we have doesn't have problems like typos or things of that nature.

[bowlofeggs]

We don't need to assert that del works.

[jdieter]

Fixed in next commit.

[bowlofeggs]

I think this code path is adequately tested by other tests. What do you think?

[jdieter]

If we don't have this path and zchunk is enabled, we have no way to test what happens if zchunk isn't enabled, which will leave us with a hole in the coverage.

[bowlofeggs]

Ah ok, that makes sense to me.

[jdieter]

Randy, thanks for the review! I've answered your individual questions, and I hope I cleared up where insert_into_repo is actually being tested. I've pushed a fix for the unneeded assertions, but I think everything else is needed. If I'm missing something, please let me know.

[bowlofeggs]

I would like to see that one mock test be a bit more thorough. If you don't have time/desire to do it, I can do it for you (though probably not this week) - just let me know!

[bowlofeggs]

I thought about this some more, and I can think of two more reasonable options:

0. We can just say Bodhi depends on the version of createrepo that has this feature. I don't mind depending on things that aren't in stable Fedora versions. For the Fedora deployment, I have a way I can just pull the new createrepo into a repository for deployment, and for CI we can just pull it in in the various dockerfiles. This way we don't need a conditional, and we can simplify your testing too.
1. We can just mark the code that handles createrepos that don't have this feature as not needing coverage. It will get coverage on Rawhide anyway, so we can rely on that to handle the tests. I can also make myself a todo to drop that code in about a year when Fedora 30 is the oldest supported stable release.

I think I'm fine with either of these if you prefer one of them over the previously suggested approach. What do you think?

[jdieter]

I don't mind making the mock test more thorough, but it might be a little while before I'm able to get to it. On the other hand, if bodhi's just going to lose the code anyway when all createrepo_c's support zchunk, maybe it's easier to just test that codepath. I don't feel strongly, so what would you prefer?

[bowlofeggs]

@jdieter yeah the more I think about it, it seems better to just say we require the newer createrepo_c. Do you know what version has the needed feature? We can encode that in our requirements. We should add that to the release notes too.

[jdieter]

@bowlofeggs, yeah, it's 0.12.0, but only on F30+ (if we're talking Fedora). 0.12.0 on <F30 is built with zchunk disabled. I suspect that might cause a problem...

[bowlofeggs]

On Thu, 2019-02-07 at 13:54 -0800, Jonathan Dieter wrote: @bowlofeggs, yeah, it's 0.12.0, but only on F30+ (if we're talking Fedora). 0.12.0 on <F30 is built with zchunk disabled. I suspect that might cause a problem...

Ah, so we can't simply require a version of it - it also needs a build flag set. Hmm. In that case, I think I do like the code you have here. I'll see if I can find some time to get the tests more complete. Thanks!

[jdieter]

Ok, I've rebased to the latest develop head and have successfully managed to get rid of the try-except blocks. Please let me know if there's anything else you'd like me to do.

[bowlofeggs]

I'm going to rebase this pull request, and I think I will add some more assertions to the mock test. I see that a folder with several RPMs was added - are those needed for this patch? I don't see a place where they get used.

[bowlofeggs]

Hey @jdieter!

I finally got some time to sit down and look at this (sorry - things are always on $fire for me…). Looks pretty good!

I added two commits on top of yours. The first removes the RPMs - I assume those were not intentionally added? The second breaks the mock test into three tests, since there are three conditions we want to test, and it also adds a lot of assertions to check that the mocks were used in an expected manner.

Please review the patches I wrote and give me a +1/-1 - I want to make sure you agree too before merging. Once we both agree, I will squash all the commits into one.

For the record (since the commits will be lost once we sqash), the test patch I wrote is here:

commit a9efe2c0e9eb9afb4ee940a5c9c8991b14ab2836
Author: Randy Barlow <randy@electronsweatshop.com>
Date:   Thu Mar 7 12:35:32 2019 -0500

    Expand the mock test assertions and split into three tests.
    
    Signed-off-by: Randy Barlow <randy@electronsweatshop.com>

diff --git a/bodhi/tests/server/test_metadata.py b/bodhi/tests/server/test_metadata.py
index e77d9bd9..c8d6b527 100644
--- a/bodhi/tests/server/test_metadata.py
+++ b/bodhi/tests/server/test_metadata.py
@@ -386,21 +386,124 @@ class TestUpdateInfoMetadata(UpdateInfoMetadataTestCase):
             self.assertEqual(pkg.arch, 'src')
             self.assertEqual(pkg.filename, 'TurboGears-1.0.2.2-2.fc17.src.rpm')
 
-    # Make sure we reach 100% coverage, even though cr.ZCK_COMPRESSION is only defined
-    # when createrepo_c supports zchunk.
-    #
-    # This function is designed to *only* make sure we reach 100% coverage and isn't meant
-    # to test whether zchunk is working correctly.  _test_extended_metadata will take care
-    # of testing both the regular and zchunked updateinfo if zchunk is enabled
     @mock.patch('bodhi.server.metadata.cr')
-    def test_zchunk_metadata_coverage(self, mock_cr):
+    def test_zchunk_metadata_coverage_xz_compression(self, mock_cr):
+        """
+        Let's test that we skip zchunk files, because we don't want to zchunk zchunk files.
+
+        This test makes sure we reach 100% coverage by mocking createrepo.
+
+        cr.ZCK_COMPRESSION is only defined when createrepo_c supports zchunk, but createrepo_c's
+        zchunk support is only available in createrepo_c >= 0.12.0, and it is also a build flag,
+        so we can't be sure that the createrepo_c we work with has that feature.
+
+        This function is designed to *only* make sure we reach 100% coverage and isn't meant
+        to test whether zchunk is working correctly.  _test_extended_metadata will take care
+        of testing both the regular and zchunked updateinfo if zchunk is enabled
+        """
         mock_cr.ZCK_COMPRESSION = 99
         mock_repomd = mock.MagicMock()
-        mock_repomd.xml_dump = mock.MagicMock(return_value="")
+        mock_repomd.xml_dump = mock.MagicMock(return_value="test data")
         mock_cr.Repomd = mock.MagicMock(return_value=mock_repomd)
-        bodhi_metadata.insert_in_repo(99, self.tempcompdir, 'garbage', 'zck', '/dev/null')
+
         bodhi_metadata.insert_in_repo(bodhi_metadata.cr.XZ_COMPRESSION, self.tempcompdir,
                                       'garbage', 'zck', '/dev/null')
-        del bodhi_metadata.cr.ZCK_COMPRESSION
+
+        mock_cr.Repomd.assert_called_once_with(os.path.join(self.tempcompdir, 'repomd.xml'))
+        self.assertEqual(
+            mock_cr.RepomdRecord.mock_calls,
+            [mock.call('garbage', os.path.join(self.tempcompdir, 'garbage.zck')),
+             mock.call().compress_and_fill(mock_cr.SHA256, mock_cr.XZ_COMPRESSION),
+             mock.call().compress_and_fill().rename_file(),
+             mock.call('garbage_zck', os.path.join(self.tempcompdir, 'garbage.zck')),
+             mock.call().compress_and_fill(mock_cr.SHA256, mock_cr.ZCK_COMPRESSION),
+             mock.call().compress_and_fill().rename_file()])
+        rec = mock_cr.RepomdRecord.return_value
+        rec_comp = rec.compress_and_fill.return_value
+        self.assertEqual(rec_comp.type, 'garbage')
+        self.assertEqual(
+            mock_cr.Repomd.return_value.set_record.mock_calls,
+            [mock.call(rec_comp), mock.call(rec_comp)])
+
+        with open(os.path.join(self.tempcompdir, 'repomd.xml')) as repomd_file:
+            repomd_contents = repomd_file.read()
+
+        self.assertEqual(repomd_contents, 'test data')
+        self.assertFalse(os.path.exists(os.path.join(self.tempcompdir, 'garbage.zck')))
+
+    @mock.patch('bodhi.server.metadata.cr')
+    def test_zchunk_metadata_coverage_zchunk_skipped(self, mock_cr):
+        """
+        Let's test that we skip zchunk files, because we don't want to zchunk zchunk files.
+
+        This test makes sure we reach 100% coverage by mocking createrepo.
+
+        cr.ZCK_COMPRESSION is only defined when createrepo_c supports zchunk, but createrepo_c's
+        zchunk support is only available in createrepo_c >= 0.12.0, and it is also a build flag,
+        so we can't be sure that the createrepo_c we work with has that feature.
+
+        This function is designed to *only* make sure we reach 100% coverage and isn't meant
+        to test whether zchunk is working correctly.  _test_extended_metadata will take care
+        of testing both the regular and zchunked updateinfo if zchunk is enabled
+        """
+        mock_cr.ZCK_COMPRESSION = 99
+        mock_repomd = mock.MagicMock()
+        mock_repomd.xml_dump = mock.MagicMock(return_value="test data")
+        mock_cr.Repomd = mock.MagicMock(return_value=mock_repomd)
+
+        bodhi_metadata.insert_in_repo(99, self.tempcompdir, 'garbage', 'zck', '/dev/null')
+
+        mock_cr.Repomd.assert_called_once_with(os.path.join(self.tempcompdir, 'repomd.xml'))
+        mock_cr.RepomdRecord.assert_called_once_with('garbage',
+                                                     os.path.join(self.tempcompdir, 'garbage.zck'))
+        rec = mock_cr.RepomdRecord.return_value
+        rec.compress_and_fill.assert_called_once_with(mock_cr.SHA256, mock_cr.ZCK_COMPRESSION)
+        rec_comp = rec.compress_and_fill.return_value
+        rec_comp.rename_file.assert_called_once_with()
+        self.assertEqual(rec_comp.type, 'garbage')
+        mock_cr.Repomd.return_value.set_record.assert_called_once_with(rec_comp)
+
+        with open(os.path.join(self.tempcompdir, 'repomd.xml')) as repomd_file:
+            repomd_contents = repomd_file.read()
+
+        self.assertEqual(repomd_contents, 'test data')
+        self.assertFalse(os.path.exists(os.path.join(self.tempcompdir, 'garbage.zck')))
+
+    @mock.patch('bodhi.server.metadata.cr')
+    def test_zchunk_metadata_coverage_zchunk_unsupported(self, mock_cr):
+        """
+        Let's test that we skip zchunk compression when it is unsupported by createrepo_c.
+
+        This test makes sure we reach 100% coverage by mocking createrepo.
+
+        cr.ZCK_COMPRESSION is only defined when createrepo_c supports zchunk, but createrepo_c's
+        zchunk support is only available in createrepo_c >= 0.12.0, and it is also a build flag,
+        so we can't be sure that the createrepo_c we work with has that feature.
+
+        This function is designed to *only* make sure we reach 100% coverage and isn't meant
+        to test whether zchunk is working correctly.  _test_extended_metadata will take care
+        of testing both the regular and zchunked updateinfo if zchunk is enabled
+        """
+        del mock_cr.ZCK_COMPRESSION
+        mock_repomd = mock.MagicMock()
+        mock_repomd.xml_dump = mock.MagicMock(return_value="test data")
+        mock_cr.Repomd = mock.MagicMock(return_value=mock_repomd)
+
         bodhi_metadata.insert_in_repo(bodhi_metadata.cr.XZ_COMPRESSION, self.tempcompdir,
                                       'garbage', 'xz', '/dev/null')
+
+        mock_cr.Repomd.assert_called_once_with(os.path.join(self.tempcompdir, 'repomd.xml'))
+        mock_cr.RepomdRecord.assert_called_once_with('garbage',
+                                                     os.path.join(self.tempcompdir, 'garbage.xz'))
+        rec = mock_cr.RepomdRecord.return_value
+        rec.compress_and_fill.assert_called_once_with(mock_cr.SHA256, mock_cr.XZ_COMPRESSION)
+        rec_comp = rec.compress_and_fill.return_value
+        rec_comp.rename_file.assert_called_once_with()
+        self.assertEqual(rec_comp.type, 'garbage')
+        mock_cr.Repomd.return_value.set_record.assert_called_once_with(rec_comp)
+
+        with open(os.path.join(self.tempcompdir, 'repomd.xml')) as repomd_file:
+            repomd_contents = repomd_file.read()
+
+        self.assertEqual(repomd_contents, 'test data')
+        self.assertFalse(os.path.exists(os.path.join(self.tempcompdir, 'garbage.zck')))

[bowlofeggs]

Also, note that I've force pushed to your branch after rebasing.

[bowlofeggs]

We can ignore the rawhide-build failure. It's a problem in Rawhide and not in this pull request.

[jdieter]

This looks great! Yes, the RPMS were for testing, and should have never made it into the commit. So, a +1 from me. (I'm assuming the F28-python3-integration is also a known bug.)

[bowlofeggs]

Yeah the f28-python3-integration test seems to be a timeout in starting up one of the test containers. I think we can safely assume it's not due to this patch, especially since the tests did pass on the other releases.

[bowlofeggs]

Thanks!

[jdieter]

Thank you! I'm glad we managed to get it in!

[bowlofeggs]

quay.io seems to be down right now, which is why the integration tests failed.

[bowlofeggs]

This patch is planned for inclusion in the upcoming 4.0.0 release: #3221

[bowlofeggs]

Bodhi 4.0.0 beta is built and deployed to staging:

https://copr.fedorainfracloud.org/coprs/bowlofeggs/bodhi-pre-release
https://bodhi.stg.fedoraproject.org/