Fixes #16063 - content facet builds when not present #6401

lzap · 2016-10-18T12:04:38Z

Katello as well as the facet framework assumes that the only entrypoint for
host objects are our UI and API controllers (host#create). Facet framework
hooks into there and it just works.

Except there is more. Discovered hosts can be provisioned either via edit host
form (which works with facets) but also via autoprovisioning which is an
alternative path which is missed by Facet framework.

Until Shim comes up with a nicer solution, I propose this "dirty" patch that
explicitly builds the facet object when it's not present. This fixes the issue
in Foreman 1.13, 1.12 and Satellite 6.2 and 6.3. The symptoms are that all
auto-provisioned hosts are completely missing all content flags (source, view,
environment, kickstart repo) and fails provisioning.

I also created and tested downstream version against Satellite 6.2 because
there is opened ticket for 6.2 errata (there were several changes in the
codebase already), so the patch looks like this:

      def set_hostgroup_defaults_with_katello_attributes
        if hostgroup.present?
          if content_facet.present?
            self.content_facet.kickstart_repository_id ||= hostgroup.inherited_kickstart_repository_id
          else
            if hostgroup.inherited_content_view_id && hostgroup.inherited_lifecycle_environment_id
              build_content_facet(
                :kickstart_repository_id => hostgroup.inherited_kickstart_repository_id,
                :content_source_id => hostgroup.inherited_content_source_id,
                :content_view_id => hostgroup.inherited_content_view_id,
                :lifecycle_environment_id => hostgroup.inherited_lifecycle_environment_id)
            end
          end
          assign_hostgroup_attributes(%w(content_source_id content_view_id lifecycle_environment_id environment_id))
        end
        set_hostgroup_defaults_without_katello_attributes
      end

References:

Original issue I need to fix in Foreman and Satellite:

http://projects.theforeman.org/issues/16063

Planned change in Facet framework:

http://projects.theforeman.org/issues/16987

BZ for 6.2:

https://bugzilla.redhat.com/show_bug.cgi?id=1364544

mention-bot · 2016-10-18T12:04:39Z

@lzap, thanks for your PR! By analyzing the history of the files in this pull request, we identified @xprazak2, @jlsherrill and @bbuckingham to be potential reviewers.

lzap · 2016-10-18T12:08:52Z

Fixed hound comment. Also CCing @ehelms as I am providing backported version already (it's tested on 6.2.2). Reproducer:

Create a rule "always" with "cpu_count > 1" and associate with any hostgroup that has some Content (View, Environment, Source, Repo) associated. Discover a fake host (*) and AUTO provision it. Check the host if it has all the content flags set. Without the patch, all the fields are empty, thus provisioning would fail.

(*) https://github.com/lzap/bin-public/blob/master/discover-host - script that creates "fake" discovery hosts

jlsherrill · 2016-10-18T12:17:08Z

app/models/katello/concerns/host_managed_extensions.rb

@@ -75,7 +75,18 @@ def content_and_puppet_match?
      end

      def inherited_attributes_with_katello
-        self.content_facet.kickstart_repository_id ||= hostgroup.inherited_kickstart_repository_id if content_facet.present?
+        if hostgroup.present?
+          if content_facet.present?


We should probably check to see if the hostgroup has a content_view and lifecycle_env first. Content facets aren't valid if both of those are not present.

Err, this should really have been tagged at the build_content_facet part below

Sure, plus as per IRC discussion, I should use "inherited_" flags everywhere.

theforeman-bot · 2016-10-18T12:58:12Z

@lzap, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

Move ticket #16063 to the Discovery project
File a new ticket in the Discovery project, update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2016-10-18T12:59:26Z

@lzap, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

Move ticket #16063 to the Discovery project
File a new ticket in the Discovery project, update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

lzap · 2016-10-18T13:01:40Z

Amended the change, also updated the backport code bit in the description.

Please test the version against develop Katello before merging, I was testing with older version.

Moved the Issue to Katello project. Thanks.

lzap · 2016-10-21T12:26:10Z

Ping - see my instructions above how to test this. It's not complex, don't worry :-)

jlsherrill · 2016-10-26T14:05:03Z

app/models/katello/concerns/host_managed_extensions.rb

+            if hostgroup.inherited_content_view_id && hostgroup.inherited_lifecycle_environment_id
+              build_content_facet(
+                :kickstart_repository_id => hostgroup.inherited_kickstart_repository_id,
+                :content_source_id => hostgroup.inherited_content_source_id,


getting an error because of this line:

unknown attribute 'content_source_id' for Katello::Host::ContentFacet.

content_source_id is actually on the host object.

Deleting this line, and removing 'content_view_id' and 'lifecycle_environment_id' from the line below:

inherited_attributes_without_katello.concat(%w(content_source_id content_view_id lifecycle_environment_id))

Seems to make it all work.

I'll work on reviewing it quickly after a turnaround :)

Many thanks, let me fix this.

Allright, did you mean this?

diff --git a/app/models/katello/concerns/host_managed_extensions.rb b/app/models/katello/concerns/host_managed_extensions.rb index 23fc0f1..202ed53 100644 --- a/app/models/katello/concerns/host_managed_extensions.rb +++ b/app/models/katello/concerns/host_managed_extensions.rb @@ -83,13 +83,12 @@ module Katello if hostgroup.inherited_content_view_id && hostgroup.inherited_lifecycle_environment_id build_content_facet( :kickstart_repository_id => hostgroup.inherited_kickstart_repository_id, - :content_source_id => hostgroup.inherited_content_source_id, :content_view_id => hostgroup.inherited_content_view_id, :lifecycle_environment_id => hostgroup.inherited_lifecycle_environment_id) end end end - inherited_attributes_without_katello.concat(%w(content_source_id content_view_id lifecycle_environment_id)) + inherited_attributes_without_katello.concat(%w(content_source_id)) end def import_package_profile(simple_packages)

Pushed! Thanks for help with this one.

Yep exactly, thanks! will re-test

jlsherrill · 2016-10-26T17:43:55Z

APJ

lzap · 2016-10-26T19:13:07Z

Sorry, what's that?

jlsherrill · 2016-10-26T19:16:24Z

ACK Pending Jenkins :)

jlsherrill · 2016-10-31T15:13:16Z

[test]

jlsherrill · 2016-11-01T00:10:25Z

thanks @lzap! merging 🍖

lzap · 2016-11-01T13:24:34Z

Thank you so much @jlsherrill !

Is there any chance of getting this into latest stable Katello? Because provisioning is basically blocked for upstream stable users.

jlsherrill · 2016-11-01T13:28:57Z

@lzap yep it'll be cherrypicked for 3.2 GA

theforeman-bot added Needs testing Not yet reviewed labels Oct 18, 2016

lzap force-pushed the explicit-facet-build-16063 branch from c995a17 to 38a877d Compare October 18, 2016 12:06

jlsherrill reviewed Oct 18, 2016

View reviewed changes

theforeman-bot added the Waiting on contributor label Oct 18, 2016

lzap force-pushed the explicit-facet-build-16063 branch from 38a877d to 35de759 Compare October 18, 2016 12:59

theforeman-bot added Waiting on contributor Needs testing and removed Waiting on contributor labels Oct 18, 2016

jlsherrill self-assigned this Oct 26, 2016

jlsherrill reviewed Oct 26, 2016

View reviewed changes

jlsherrill added Waiting on contributor and removed Needs testing Not yet reviewed labels Oct 26, 2016

Fixes #16063 - content facet builds when not present

bd8fb1c

lzap force-pushed the explicit-facet-build-16063 branch from 35de759 to bd8fb1c Compare October 26, 2016 16:48

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 26, 2016

jlsherrill added Ready for merge and removed Needs re-review Needs testing labels Oct 26, 2016

jlsherrill merged commit 0d1d7c1 into Katello:master Nov 1, 2016

lzap deleted the explicit-facet-build-16063 branch November 1, 2016 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #16063 - content facet builds when not present #6401

Fixes #16063 - content facet builds when not present #6401

lzap commented Oct 18, 2016 •

edited

Loading

mention-bot commented Oct 18, 2016

lzap commented Oct 18, 2016

jlsherrill Oct 18, 2016 •

edited

Loading

lzap Oct 18, 2016

lzap Oct 18, 2016

theforeman-bot commented Oct 18, 2016

theforeman-bot commented Oct 18, 2016

lzap commented Oct 18, 2016

lzap commented Oct 21, 2016

jlsherrill Oct 26, 2016

jlsherrill Oct 26, 2016

jlsherrill Oct 26, 2016

lzap Oct 26, 2016

lzap Oct 26, 2016

lzap Oct 26, 2016

jlsherrill Oct 26, 2016

jlsherrill commented Oct 26, 2016

lzap commented Oct 26, 2016

jlsherrill commented Oct 26, 2016

jlsherrill commented Oct 31, 2016

jlsherrill commented Nov 1, 2016

lzap commented Nov 1, 2016

jlsherrill commented Nov 1, 2016

Fixes #16063 - content facet builds when not present #6401

Fixes #16063 - content facet builds when not present #6401

Conversation

lzap commented Oct 18, 2016 • edited Loading

mention-bot commented Oct 18, 2016

lzap commented Oct 18, 2016

jlsherrill Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Oct 18, 2016

theforeman-bot commented Oct 18, 2016

lzap commented Oct 18, 2016

lzap commented Oct 21, 2016

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

jlsherrill commented Oct 26, 2016

lzap commented Oct 26, 2016

jlsherrill commented Oct 26, 2016

jlsherrill commented Oct 31, 2016

jlsherrill commented Nov 1, 2016

lzap commented Nov 1, 2016

jlsherrill commented Nov 1, 2016

lzap commented Oct 18, 2016 •

edited

Loading

jlsherrill Oct 18, 2016 •

edited

Loading