/foreman

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #20929 - Added fact name filtering on import #5021

Merged
merged 1 commit into from Dec 30, 2017

Conversation

Reviewers

@ares ares

@lzap lzap

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
4 participants
@ShimShtein @theforeman-bot @lzap @ares
@ShimShtein
Member

ShimShtein commented Nov 22, 2017

Added a setting that will filter out fact names, so they won't be recorded into fact_names and fact_values tables at all.
@theforeman-bot

This comment has been minimized.

Show comment
Hide comment
@theforeman-bot

theforeman-bot Nov 22, 2017

Member

Issues: #20929
Member

theforeman-bot commented Nov 22, 2017

Issues: #20929

@theforeman-bot theforeman-bot added Needs testing Not yet reviewed labels Nov 22, 2017

@lzap

lzap requested changes Dec 7, 2017
View changes

We need this patch so hard, thank you so much :-)

Show outdated Hide outdated app/models/setting/provisioning.rb
Show outdated Hide outdated app/services/fact_importer.rb
Show outdated Hide outdated db/migrate/20171122092336_add_ignored_fact_names_setting.rb

@theforeman-bot theforeman-bot added Waiting on contributor Needs re-review Needs testing and removed Not yet reviewed Waiting on contributor labels Dec 7, 2017

@ShimShtein

This comment has been minimized.

Show comment
Hide comment
@ShimShtein

ShimShtein Dec 13, 2017

Member

Rebased, renamed, refactored and extended already existing facts:clean rake task
Member

ShimShtein commented Dec 13, 2017

Rebased, renamed, refactored and extended already existing facts:clean rake task
@lzap

This comment has been minimized.

Show comment
Hide comment
@lzap

lzap Dec 14, 2017

Member

Marek pointed out on one important issue - fact names are different from interface names.

interfaces => enp30s0,lo,redhat0,virbr0,virbr1,virbr2,virbr0_nic,virbr1_nic,virbr2_nic
ipaddress6_enp30s0 => fd5a:f687:b576:0:xxxx:f6ff:fe3e:aa52
ipaddress_enp30s0 => 192.168.x.13
macaddress_enp30s0 => 88:d7:f6:xx:xx:52
mtu_enp30s0 => 1500
netmask_enp30s0 => 255.255.255.0
network_enp30s0 => 192.168.1.0

I think we should be more explicit to prevent filtering out unwanted entries, therefore I suggest to create separate starting list. How about this:

 IGNORED_FACTS =  IGNORED_INTERFACES.map{|i| "_#{i}"}

This will at least prefix all entries with underscore, which will match what we want.
Member

lzap commented Dec 14, 2017

Marek pointed out on one important issue - fact names are different from interface names.

interfaces => enp30s0,lo,redhat0,virbr0,virbr1,virbr2,virbr0_nic,virbr1_nic,virbr2_nic
ipaddress6_enp30s0 => fd5a:f687:b576:0:xxxx:f6ff:fe3e:aa52
ipaddress_enp30s0 => 192.168.x.13
macaddress_enp30s0 => 88:d7:f6:xx:xx:52
mtu_enp30s0 => 1500
netmask_enp30s0 => 255.255.255.0
network_enp30s0 => 192.168.1.0

I think we should be more explicit to prevent filtering out unwanted entries, therefore I suggest to create separate starting list. How about this:

 IGNORED_FACTS =  IGNORED_INTERFACES.map{|i| "_#{i}"}

This will at least prefix all entries with underscore, which will match what we want.
@lzap

lzap requested changes Dec 14, 2017
View changes

Still have some comments.

Show outdated Hide outdated test/unit/fact_cleaner_test.rb
Show outdated Hide outdated app/models/setting/provisioning.rb
Show outdated Hide outdated app/services/fact_cleaner.rb
Show outdated Hide outdated app/services/fact_cleaner.rb
Show outdated Hide outdated app/services/fact_cleaner.rb
Show outdated Hide outdated db/migrate/20171122092336_add_ignored_fact_names_setting.rb

@theforeman-bot theforeman-bot added Waiting on contributor Needs re-review Needs testing and removed Needs re-review Waiting on contributor labels Dec 14, 2017

@lzap

lzap reviewed Dec 19, 2017
View changes

I have two big concerns. First, the deletion must not happen during fact upload, but via an external rake task (cron). This way, we can even make this more simple and simply iterate over all fact names via Ruby loop and filter them out using single regexp, you are trying to work in two "modes" (SQL and regexp) which makes the code too complex.

Second, you are not testing with real sample data. I think the code either does not match real world samples, or filters out what's not required. I think the pattern should be with underscore to match macaddress_em1 (pattern *_em1 or dhcp-server::em1 (pattern ::em1).

Show outdated Hide outdated app/services/fact_cleaner.rb
Show outdated Hide outdated app/models/setting/provisioning.rb
Show outdated Hide outdated test/unit/fact_cleaner_test.rb
Show outdated Hide outdated db/migrate/20171122092336_add_ignored_fact_names_setting.rb

@theforeman-bot theforeman-bot added Waiting on contributor Needs re-review Needs testing and removed Needs re-review Waiting on contributor labels Dec 19, 2017

@lzap lzap dismissed their stale review Dec 20, 2017

New version

@lzap

lzap requested changes Dec 20, 2017
View changes

Overall no blockers but I don't want to rush this. Can you improve tests, I am still missing one and I have a proposal to share testing data across both two workflows to prevent regressions.

Show outdated Hide outdated app/models/setting.rb
Show outdated Hide outdated app/services/fact_cleaner.rb
Show outdated Hide outdated app/services/fact_cleaner.rb
Show outdated Hide outdated app/services/fact_cleaner.rb
test/unit/fact_cleaner_test.rb
ignored_fact_name = FactoryBot.create(:fact_name, :name => 'ignored01')
bad_fact_name = FactoryBot.create(:fact_name, :name => 'empty_bad')
FactoryBot.create(:fact_value, :fact_name => good_fact_name)
FactoryBot.create(:fact_value, :fact_name => ignored_fact_name)

This comment has been minimized.

Show comment
Hide comment
@lzap

lzap Dec 20, 2017

Member

I still do not see testing of "macaddress_em1" kind of fact. Frankly, I was hoping for real-world interface names rather than these artificial ones :-)
Copy link
@lzap

lzap Dec 20, 2017

Member

I still do not see testing of "macaddress_em1" kind of fact. Frankly, I was hoping for real-world interface names rather than these artificial ones :-)
Show outdated Hide outdated test/unit/fact_importer_test.rb
Show outdated Hide outdated test/unit/fact_importer_test.rb
Show outdated Hide outdated test/unit/fact_importer_test.rb
Show outdated Hide outdated test/unit/structured_fact_importer_test.rb

@theforeman-bot theforeman-bot added Waiting on contributor Needs re-review Needs testing and removed Needs re-review Waiting on contributor labels Dec 20, 2017

@ShimShtein

This comment has been minimized.

Show comment
Hide comment
@ShimShtein

ShimShtein Dec 24, 2017

Member

Finally! The tests are green. Had to give up cleaner's DB code, there is no way to escape LIKE query so it will work on all databases.
Member

ShimShtein commented Dec 24, 2017

Finally! The tests are green. Had to give up cleaner's DB code, there is no way to escape LIKE query so it will work on all databases.
@lzap

lzap requested changes Dec 26, 2017
View changes

One ugly line and few nitpicks.

Show outdated Hide outdated app/services/fact_cleaner.rb
app/services/fact_cleaner.rb
logger.debug "Removing excluded fact names..."
deleted_names = FactName.unscoped.where(:id => fact_names).delete_all
logger.debug "Removed #{deleted_names} fact name records."
deleted_names

This comment has been minimized.

Show comment
Hide comment
@lzap

lzap Dec 26, 2017

Member

I am usually against debug messages like "I am doing something..." the "Removed" lines are enough to tell you it has been processed.
Copy link
@lzap

lzap Dec 26, 2017

Member

I am usually against debug messages like "I am doing something..." the "Removed" lines are enough to tell you it has been processed.

This comment has been minimized.

Show comment
Hide comment
@ShimShtein

ShimShtein Dec 27, 2017

Member

It really helps to identify slow queries - you will see the "starting.." message, but not the "finished" one.
Copy link
@ShimShtein

ShimShtein Dec 27, 2017

Member

It really helps to identify slow queries - you will see the "starting.." message, but not the "finished" one.

This comment has been minimized.

Show comment
Hide comment
@lzap

lzap Dec 29, 2017

Member

Fair enough, although AR time is reported for each request by Rails framework and then there is SQL server tracking, but I buy that. We miss TRACE level so much.
Copy link
@lzap

lzap Dec 29, 2017

Member

Fair enough, although AR time is reported for each request by Rails framework and then there is SQL server tracking, but I buy that. We miss TRACE level so much.
app/services/fact_importer.rb
:suffix => '(\Z|::.*)'
}
)
end

This comment has been minimized.

Show comment
Hide comment
@lzap

lzap Dec 26, 2017

Member

Okay, this can be moved to settings, why not? The other line with send really hurts :-)
Copy link
@lzap

lzap Dec 26, 2017

Member

Okay, this can be moved to settings, why not? The other line with send really hurts :-)

This comment has been minimized.

Show comment
Hide comment
@ShimShtein

ShimShtein Dec 27, 2017

Member

Since we don't have a special settings class, I prefer keeping it with the importer. Refactored it to a class method though.
Copy link
@ShimShtein

ShimShtein Dec 27, 2017

Member

Since we don't have a special settings class, I prefer keeping it with the importer. Refactored it to a class method though.

This comment has been minimized.

Show comment
Hide comment
@lzap

lzap Dec 29, 2017

Member

As long as the send method is gone, okay :-)
Copy link
@lzap

lzap Dec 29, 2017

Member

As long as the send method is gone, okay :-)
test/fact_importer_test_helper.rb
'something::filter::something_else' => 'will_not_show'
}
end
end

This comment has been minimized.

Show comment
Hide comment
@lzap

lzap Dec 26, 2017

Member

One could say this belongs to Katello, but I really think it should be in core. Thanks.
Copy link
@lzap

lzap Dec 26, 2017

Member

One could say this belongs to Katello, but I really think it should be in core. Thanks.

This comment has been minimized.

Show comment
Hide comment
@ShimShtein

ShimShtein Dec 27, 2017

Member

Katello is only an example of this kind of facts. Since this notation is supported by core, I think it should be tested in core.
Copy link
@ShimShtein

ShimShtein Dec 27, 2017

Member

Katello is only an example of this kind of facts. Since this notation is supported by core, I think it should be tested in core.
app/models/setting/provisioning.rb
def self.default_excluded_facts(ignored_interfaces = IGNORED_INTERFACES)
ignored_interfaces
end

This comment has been minimized.

Show comment
Hide comment
@lzap

lzap Dec 26, 2017

Member

Can you optionally add couple of tests to test IGNORED_INTERFACES constant actually? We are not sure we ship with default value that just works. A single test with different names "Ipaddress_vnet1" should do it "Ipaddress:vnet1".
Copy link
@lzap

lzap Dec 26, 2017

Member

Can you optionally add couple of tests to test IGNORED_INTERFACES constant actually? We are not sure we ship with default value that just works. A single test with different names "Ipaddress_vnet1" should do it "Ipaddress:vnet1".
@ShimShtein
Fixes #20929 - Added fact name filtering on import 
Added a setting that will filter out fact names, so they won't be
recorded into `fact_names` and `fact_values` tables at all.
Loading status checks…
517f03a
@lzap

lzap approved these changes Dec 30, 2017
View changes

Thanks, merging.

@lzap

This comment has been minimized.

Show comment
Hide comment
@lzap

lzap Dec 30, 2017

Member

Katello test failures seem irrelevant.
Member

lzap commented Dec 30, 2017

Katello test failures seem irrelevant.

@lzap lzap merged commit 61b35ac into theforeman:develop Dec 30, 2017
5 of 6 checks passed

5 of 6 checks passed

katello Build finished.
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
foreman Build finished.
Details
hound No violations found. Woof!
upgrade Build finished.
Details

@ShimShtein ShimShtein referenced this pull request Mar 14, 2018

Merged

Fixes #22894 - Modified RHSM fact importer for name filtering. #7240

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment