Add support for suse errata #1088

werwty · 2018-02-27T20:04:08Z

Metadata sync currently works with query_auth_token. Removing
TYPE_ID_ERRATA from QUERY_AUTH_TOKEN_UNSUPPORTED allows pulp to sync
updateinfo.xml from SLES, and generate errata content.

Add relogin_suggested, restart_suggested to errata model since these
fields are available on SUSE metadata.

Add migrations to create relogin_suggested and restart_suggested fields
to pulp's errata models in database

fixes #3377
https://pulp.plan.io/issues/3377

goosemania · 2018-03-08T16:58:17Z

plugins/pulp_rpm/plugins/db/models.py

@@ -1215,7 +1223,7 @@ def update_needed(self, other):
                            format.
        """
        if other.updated == "":
-            return False
+            return other.version > self.version


Is it possible that there is no version in the erratum so one of the operands (or both) canl be None?

hmm I had assumed version was required since it was neither marked as optional, nor commented on here: https://github.com/openSUSE/libzypp/blob/master/zypp/parser/yum/schema/updateinfo.rnc#L11

But I guess I shouldn't assume anything when it comes to errata...

goosemania · 2018-03-08T17:00:04Z

plugins/pulp_rpm/plugins/db/models.py

@@ -1226,6 +1234,10 @@ def update_needed(self, other):
        existing_updated_dt = util.errata_format_to_datetime(self_updated_field,
                                                             msg=existing_err_msg)
        new_updated_dt = util.errata_format_to_datetime(other.updated, msg=other_err_msg)
+
+        if new_updated_dt == existing_updated_dt:
+            return other.version > self.version


Same concern as above. Is it possible that operand(s) can have None value?

goosemania · 2018-03-08T17:06:03Z

plugins/pulp_rpm/plugins/importers/yum/repomd/updateinfo.py

@@ -62,7 +66,10 @@ def process_package_element(element):
        for package in collection['packages']:
            if package.get('reboot_suggested') is not None:
                package_info['reboot_suggested'] = True
-                break
+            if package.get('relogin_suggested') is not None:
+                package_info['relogin_suggested'] = True


Is here an assumption that if tag is present in the package description then it always has a True value? Is it intentional? Can tag be present but has a False value?

I assumed this worked in the same way as reboot_suggested, but it's a good idea to revise this to work with both True and False possible values

goosemania · 2018-03-08T17:11:10Z

plugins/pulp_rpm/plugins/distributors/yum/metadata/updateinfo.py

@@ -122,6 +122,12 @@ def add_unit_metadata(self, item, filtered_pkglist):
        if erratum_unit.reboot_suggested:
            self.xml_generator.completeElement('reboot_suggested', {}, 'True')

+        if erratum_unit.relogin_suggested:


This adds relogin_suggested to the top level of erratum xml. As per SUSE docs, it's only inside of package.

reboot_suggested is added both to the top level and inside the package tag because of how certain versions of yum handles that, iirc. So I suggest to comply with SUSE docs for its tags.

goosemania · 2018-03-08T17:22:20Z

plugins/pulp_rpm/plugins/migrations/0042_add_suse_fields_to_errata.py

+    erratum_collection = get_collection('units_erratum')
+
+    for errata in erratum_collection.find({}):
+        erratum_collection.update({'_id': errata['_id']},


For migration to be idempotent, I suggest to update only those which don't have new fields at all.

goosemania · 2018-03-08T17:25:32Z

plugins/pulp_rpm/plugins/migrations/0042_add_suse_fields_to_errata.py

+
+    erratum_collection = get_collection('units_erratum')
+
+    for errata in erratum_collection.find({}):


Since the only field you need is _id, I suggest to limit returned fields to that.
It's also usually not a bad idea to set some batch_size to the cursor not being timed out in case of slowness with DB writes.
See example here

goosemania · 2018-03-08T17:27:14Z

plugins/pulp_rpm/plugins/migrations/0042_add_suse_fields_to_errata.py

+
+    erratum_collection = get_collection('units_erratum')
+
+    for errata in erratum_collection.find({}):


For users it's more clear what's happening when they see the progress of the migration. Consider adding progress like here

goosemania · 2018-03-09T14:24:56Z

plugins/pulp_rpm/plugins/db/models.py

@@ -1226,6 +1242,11 @@ def update_needed(self, other):
        existing_updated_dt = util.errata_format_to_datetime(self_updated_field,
                                                             msg=existing_err_msg)
        new_updated_dt = util.errata_format_to_datetime(other.updated, msg=other_err_msg)
+
+        # if updated time are equivalent and version exists on both erratum, then check version


Just because it's not easy to keep comments up to date, I suggest to remove this one. The docstring to the method explains logic well and variable names are self-descriptive enough, imo. But it's entirely up to you.

goosemania · 2018-03-12T12:52:35Z

docs/user-guide/release-notes/2.16.x.rst

@@ -10,3 +10,8 @@ New Features

 * A new pulp-manifest tool that can be used to create PULP_MANIFEST for a
  directory that the user plans to use the iso importer with.
+
+* Add support for SUSE Errata format. This includes the addition of two new
+  fields on the Erratum mode: `relogin_suggested` and `restart_suggested`.


s/mode/model/

goosemania · 2018-03-12T12:56:21Z

plugins/pulp_rpm/plugins/db/models.py

+        fields
+
+        Note: SUSE errata lacks the updated field, and rely only on version to determine if it
+        needs an update. Redhat errata only increments version when text in the errata changes;


maybe s/Redhat errata/dnf/yum errata/ ?
or s/Redhat errata/Fedora/RHEL errata/ ?

goosemania · 2018-03-12T13:00:55Z

plugins/pulp_rpm/plugins/importers/yum/repomd/updateinfo.py

@@ -62,13 +64,17 @@ def process_package_element(element):
        for package in collection['packages']:
            if package.get('reboot_suggested') is not None:
                package_info['reboot_suggested'] = True
-                break


Why is it needed to remove this break and inspect all the packages in a collection for reboot_suggested?

Ah my bad, this was when I was checking restart_suggested and relogin_suggested in the loop too, I'll put it back

goosemania · 2018-03-12T15:44:39Z

plugins/pulp_rpm/plugins/distributors/yum/metadata/updateinfo.py

@@ -173,6 +173,11 @@ def add_unit_metadata(self, item, filtered_pkglist):

            if package.get('reboot_suggested'):
                self.xml_generator.completeElement('reboot_suggested', {}, 'True')
+            if package.get('relogin_suggested'):
+                self.xml_generator.completeElement('relogin_suggested', {}, 'True')


Based on this, I guess relogin/restart_suggested can be either True or 1 when enabled.
If it's so, then I think we should preserve the original value, if it was 1, let's generate 1 as well.
But I think here it always will be True. Am I missing something?

goosemania · 2018-03-12T16:26:26Z

plugins/pulp_rpm/plugins/migrations/0042_add_suse_fields_to_errata.py

+    total_erratum_units = erratum_collection.count(
+        {'relogin_suggested': {'$exists': False}, 'restart_suggested': {'$exists': False}})
+
+    with utils.MigrationProgressLog('Erratum', total_erratum_units) as migration_log:


I'm not sure why progress shows N out of N units, when migration_log.progress() is called 2N times. I guess it's a bug in progress tooling.

I get that we need two loops to handle the case when migration was interrupted but I guess then we need 2 separate progress logs as well... and the total number of units can potentially be different in such case.

Maybe something like this:

fields_to_add = ['relogin_suggested', 'restart_suggested'] for field in fields_to_add: total_erratum_units = erratum_collection.count( {field: {'$exists': False}}) with utils.MigrationProgressLog('Erratum (%s field)' % field, total_erratum_units) as migration_log: for errata in erratum_collection.find( {field: {'$exists': False}})... ...

Thoughts?

goosemania · 2018-03-12T16:29:24Z

plugins/test/unit/plugins/migrations/test_0042_add_suse_fields_to_errata.py

+        }
+
+    @mock.patch.object(migration, 'get_collection')
+    def test_calls_correct_functions(self, mock_get_collection):


Can you also add tests for re-run cases?
Both fields are already there, so no db update calls
One field is there and it's not changed after migration, just the missing one is added.

@goosemania I think it's hard making a unit test for "One field is there and it's not changed after migration, just the missing one is added." Since I'm setting mock_get_collection.return_value.find.return_value.batch_size.return_value to test. Do you have any suggestions for this?

goosemania · 2018-03-12T16:34:51Z

@werwty , one more thing. I tested this PR with the repo mentioned in the redmine issue. And it turned out that in the remote repo there are 23 restart_suggested tags and in repo published by Pulp, only 14.
It can be for 2 reasons I think:

Not all packages which contain restart_suggested are present in the repository and thus Pulp excludes them from the list of packages in errata. This case is fine and we can ignore the difference.
Pulp generates incorrect errata with regards to restart_suggested tag.

I didn't check the relogin_suggested tag.

Can you take a look at it and figure out where the problem is?

werwty · 2018-03-12T18:08:29Z

@goosemania When errata is published would you expect pulp to publish only the arch="x86_64" packages? As far as I can see Pulp is not publishing the arch="src" packages from the SUSE errata

goosemania · 2018-03-12T18:57:07Z

@werwty , here, where we create a list of packages in a repo we look at RPMs only. I guess no one thought about SRPMs.

Good find!

goosemania

Thanks a lot, @werwty ! 🎆 🎉 🐙

Metadata sync currently works with query_auth_token. Removing TYPE_ID_ERRATA from QUERY_AUTH_TOKEN_UNSUPPORTED allows pulp to sync updateinfo.xml from SLES, and generate errata content. Add relogin_suggested, restart_suggested to errata model since these fields are available on SUSE metadata. Add migrations to create relogin_suggested and restart_suggested fields to pulp's errata models in database fixes #3377 https://pulp.plan.io/issues/3377

werwty force-pushed the suse branch 5 times, most recently from 4842ce9 to 1bd728d Compare March 7, 2018 19:13

werwty changed the title ~~Allow errata to be synced with query_auth_token.~~ Add support for suse errata Mar 8, 2018

goosemania requested changes Mar 8, 2018

View reviewed changes

werwty force-pushed the suse branch 9 times, most recently from dcfec49 to 82e7b94 Compare March 8, 2018 21:52

goosemania requested changes Mar 12, 2018

View reviewed changes

werwty force-pushed the suse branch 2 times, most recently from daf6a11 to ac0714d Compare March 12, 2018 19:35

goosemania approved these changes Mar 12, 2018

View reviewed changes

werwty force-pushed the suse branch from ac0714d to 424263d Compare March 12, 2018 20:42

werwty force-pushed the suse branch from 424263d to 1bbf8cc Compare March 12, 2018 20:46

goosemania merged commit f0ffdac into pulp:master Mar 12, 2018

Add support for suse errata #1088

Add support for suse errata #1088

werwty commented Feb 27, 2018 •

edited

goosemania Mar 8, 2018

werwty Mar 8, 2018

goosemania Mar 8, 2018

goosemania Mar 8, 2018

werwty Mar 8, 2018

goosemania Mar 8, 2018

goosemania Mar 8, 2018

goosemania Mar 8, 2018

goosemania Mar 8, 2018

goosemania Mar 9, 2018

goosemania Mar 12, 2018

goosemania Mar 12, 2018

goosemania Mar 12, 2018

werwty Mar 12, 2018

goosemania Mar 12, 2018

goosemania Mar 12, 2018

goosemania Mar 12, 2018

werwty Mar 12, 2018

goosemania commented Mar 12, 2018 •

edited

werwty commented Mar 12, 2018 •

edited

goosemania commented Mar 12, 2018

goosemania left a comment


		erratum_collection = get_collection('units_erratum')

		for errata in erratum_collection.find({}):

Add support for suse errata #1088

Add support for suse errata #1088

Conversation

werwty commented Feb 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goosemania commented Mar 12, 2018 • edited

werwty commented Mar 12, 2018 • edited

goosemania commented Mar 12, 2018

goosemania left a comment

Choose a reason for hiding this comment

werwty commented Feb 27, 2018 •

edited

goosemania commented Mar 12, 2018 •

edited

werwty commented Mar 12, 2018 •

edited