Fixes #26393 - fix uptime calculation for btime #6843

ares · 2019-06-14T08:44:18Z

Thet source of uptime information coming from subscription manager does
not mean uptime but boot time, so we need to convert it in uptime
seconds method.

theforeman-bot · 2019-06-14T08:44:36Z

Issues: #26393

ares · 2019-06-14T09:52:20Z

test/models/host_test.rb

    test 'should return nil if no uptime fact is available' do
      host = FactoryBot.create(:host)
      assert_nil host.uptime_seconds
    end

    test 'should return uptime and based on backup facts even if there are multiple other facts' do
      host = FactoryBot.create(:host)
-      ansible_uptime_fact = FactoryBot.create(:fact_name, name: 'ansible_uptime_seconds', :type => 'FactName::Ansible')


note for the reviewer: this is in fact needed since now we instantiate FactName model, in core we don't have these classes defined

timogoebel · 2019-06-14T13:22:39Z

app/models/host/base.rb

@@ -366,7 +366,12 @@ def render_template(template:, **params)
    end

    def uptime_seconds
-      self.uptime_fact&.value&.to_i
+      fact = self.uptime_fact
+      if fact&.fact_name&.name == 'proc_stat::btime'


This is super hacky. Can we please not hardcode subscription manager specific code in core?
Why do we have the fact parser models if we don't use them to parse facts?

Big 👎 from me. Sorry.

timogoebel

Can we refactor this to put the fact parsing logic into a fact parser?

timogoebel · 2019-06-16T10:04:27Z

To explain this a little bit more:

I think we should make the uptime a field on the host model. This then allows us to use scoped search for this.
This field should be updated with every fact import as we do with all other fields during a fact import. Note that we need to clear the field when the host is put in build mode.
Impelemting this feature this way we can add proper tests in the respective plugin's code and not add more technical dept here in core. And the data is still available when the facts expire.

lzap · 2019-06-18T10:59:42Z

Let's add new fact called foreman_uptime or foreman::uptime and modify our importers to set it appropriately.

ekohl · 2019-06-18T12:59:31Z

I like the idea of a derived fact and to store it somewhere, but there's some things to consider. For example, what would be the fact source on that fact?

I must also say that I like the boot time from subscription manager a lot more than the uptime from Facter. It doesn't change as often and to get the current uptime you need to calculate anyway. We can normalize uptime into boot time.

lzap · 2019-06-18T13:20:14Z

Good point on immutability, let's prefer immutable fact and calculate this in the UI instead!

ares · 2019-06-24T15:40:07Z

I agree with Timo, making uptime a first class attribute of host model makes the most sense to me. It's not different to any other information we parse from facts. This will be a bit bigger patch though and this should be fixed in 1.22-stable, so it would be harder and riskier to cherry-pick. Would it be acceptable to take this for 1.22-stable and get the "refactored" PR for develop? Could I base the new work on top of this PR?

I also agree with @ekohl, the new attribute should be boottime instead of uptime to avoid unnecessary updates. But I disagree with derived fact, I belive it should be attribute of host object itself.

timogoebel · 2019-06-24T15:44:21Z

Would it be acceptable to take this for 1.22-stable and get the "refactored" PR for develop?

Fine for me.

But I disagree with derived fact, I belive it should be attribute of host object itself.

... or a facet.

ares · 2019-06-25T05:45:38Z

I was thinking about facet too, but what would that be representing? What other attributes we'd extract? Would it be for facts based values only or generic reported info. Boottime seems as quite generic info, such mac, ip. But the facet may be a way to solve the long standing issue of provisioned vs reported OS.

timogoebel · 2019-06-25T06:59:09Z

I was thinking about facet too, but what would that be representing?

I was thinking about data we store from the hosts that we don't actually need. So data we collect to show it in the UI. Basically, some kind of inventory.

What other attributes we'd extract?

I'm not sure I would actually extract something but use it to collect more interesting data in the future.

ares · 2019-06-30T11:00:56Z

I added a second commit that adds a new facet. I'm working on other fact parsers patches. I suppose I'll need to add tests here but I'd appreciate if @timogoebel could confirm this is what you had in mind. Also @ShimShtein this seems to be the first facet in core, any comments from that perspective? Since it's just one attribute, I didn't want to add new tab to host detail page.

Btw with this, people can search using boot_time > "1 week ago". If we're adding more reported data in future (e.g. OS) maybe we should prefix search terms accordingly from the beginning

houndci-bot · 2019-06-30T12:47:07Z

test/models/host_test.rb

-      FactoryBot.create(:fact_value, fact_name: fact, host: host, :value => 123)
-      assert_equal 123, host.uptime_seconds
+      boot_time = 1.day.ago
+      reported_data = host.create_reported_data(:boot_time => boot_time)


Lint/UselessAssignment: Useless assignment to variable - reported_data.

timogoebel

@ares: Yeah, that's what I had in mind. Looks good, just some comments.

Note that in the plugin PRs tests are missing.

timogoebel · 2019-06-30T18:39:43Z

app/models/host/base.rb

@@ -1,7 +1,7 @@
 module Host
  class Base < ApplicationRecord
    KERNEL_RELEASE_FACTS = [ 'kernelrelease', 'ansible_kernel', 'kernel::release' ]
-    UPTIME_FACTS = [ 'system_uptime::seconds', 'ansible_uptime_seconds', 'uptime_seconds', 'proc_stat::btime' ]
+    UPTIME_FACTS = [ 'system_uptime::seconds', 'ansible_uptime_seconds', 'uptime_seconds' ]


Is this still used?

These are still uptime facts which can be useful, similar to kernel release fact, now these are really just uptime facts, but if you prefer I can drop.

If we don't use this somewhere, I'd prefer to drop it.

timogoebel · 2019-06-30T18:40:10Z

app/models/host/base.rb

@@ -234,6 +235,14 @@ def set_interfaces(parser)
      self.interfaces.reload
    end

+    def set_reported_data(parser)
+      if parser.boot_time


return unless parser.boot_time

timogoebel · 2019-06-30T18:45:27Z

app/models/host/base.rb

@@ -234,6 +235,14 @@ def set_interfaces(parser)
      self.interfaces.reload
    end

+    def set_reported_data(parser)
+      if parser.boot_time
+        reported_data_facet = self.reported_data || self.build_reported_data


Maybe extract this to a method so you can just call host.reported_data_facet and get either the saved object or a new one?
Then we could just do reported_data_facet.update(boot_time: parser.boot_time)

timogoebel · 2019-06-30T18:46:51Z

test/models/host_test.rb

+      assert_present host.reported_data.boot_time
+      assert_kind_of DateTime, host.reported_data.boot_time
+
+      sleep 1


I hope this is was just for debugging. Note that rails allows you to travel in time.

timogoebel · 2019-06-30T18:53:41Z

Note, that the facet should be destroyed when the host enters build mode. Just extend clear_data_on_build. A test for this would be great as well.

foreman/app/models/host/managed.rb

Lines 299 to 304 in 870c50a

    
           def clear_data_on_build 
        
             return unless respond_to?(:old) && old && build? && !old.build? 
        
             clear_facts 
        
             clear_reports 
        
             self.build_errors = nil 
        
           end

ares · 2019-07-03T13:15:30Z

updated according to comments, let's see what jenkins says before I squash

houndci-bot · 2019-07-16T11:42:12Z

app/models/host/base.rb

@@ -366,7 +375,8 @@ def render_template(template:, **params)
    end

    def uptime_seconds
-      self.uptime_fact&.value&.to_i
+      boot_time = self&.reported_data&.boot_time.to_i
+      boot_time > 0 ? Time.zone.now.to_i - boot_time : nil


Style/TernaryParentheses: Use parentheses for ternary expressions with complex conditions.

what about:

boot_time = self&.reported_data&.boot_time Time.zone.now.to_i - boot_time.to_i unless boot_time.nil? # or if boot_time

How do we know what timezone the boot time is in? I wonder if this can lead to things like -3 hours uptime in the UI.

what about:

I reverted to .nil? and moved to_i part lower, I think it's more readable than statement modified at the end of the method

How do we know what timezone the boot time is in? I wonder if this can lead to things like -3 hours uptime in the UI.

the bootime in /proc/stat is I believe in system time zone, the parsing uses Time.at which assumes boot time in UTC (# of seconds since unix epoch does not have time zone info). When we display it to user, current timezone is used to convert the time in Time.zone.now, the same for boot_time thanks to active record. When I was testing this, it seemed to work fine.

I believe that the to_i removes any information about timezone, what makes maybe the Time.zone misleading. Time.now.to_i should give the same result.

ekohl · 2019-07-16T12:58:43Z

app/services/puppet_fact_parser.rb

@@ -132,6 +132,10 @@ def support_interfaces_parsing?
    true
  end

+  def boot_time
+    Time.zone.now.to_i - facts[:uptime_seconds].to_i


The (non-deprecated) Facter 3 fact is system_uptime:

$ facter --json system_uptime { "system_uptime": { "days": 4, "hours": 100, "seconds": 361191, "uptime": "4 days" } }

That means you should first look for facts[:system_uptime][:seconds] and then fall back. I don't know if the nesting properly converts keys to symbols so probably good to check.

ezr-ondrej

Looks pretty good already., just two more comments.
I haven't tested facter yet, if anyone would like to help out :P

ezr-ondrej · 2019-07-16T14:28:29Z

app/helpers/hosts_helper.rb

@@ -276,7 +276,7 @@ def overview_fields(host)
    fields += [[_("Operating System"), link_to(host.operatingsystem.to_label, hosts_path(:search => %{os_title = "#{host.operatingsystem.title}"}))]] if host.operatingsystem.present?
    fields += [[_("PXE Loader"), host.pxe_loader]] if host.operatingsystem.present? && !host.image_build?
    fields += [[_("Host group"), link_to(host.hostgroup, hosts_path(:search => %{hostgroup_title = "#{host.hostgroup}"}))]] if host.hostgroup.present?
-    fields += [[_("Uptime"), time_ago_in_words(host.uptime_seconds.seconds.from_now)]] if host.uptime_seconds.present?
+    fields += [[_("Boot time"), date_time_relative(host&.reported_data&.boot_time) || _('Not reported')]]


Not reported is never going to show up as date_time_relative will handle nil value in the react component as 'N/A'.
Could you eighter remove the fallback, or handle it somewhat like (uglier code, but nicer value):

Suggested change

fields += [[_("Boot time"), date_time_relative(host&.reported_data&.boot_time) || _('Not reported')]]

fields += [[_("Boot time"), (boot_time = host&.reported_data&.boot_time) ? date_time_relative(boot_time) : _('Not reported')]]

Or add the option/quark default_value to the date_time_relative method and propagate it through mount_date_component to the react - the best solution 😛

I took your suggestion, also squashed all back to just 2 commits since I feel we're getting closer, they should be merged separately at the end

ezr-ondrej · 2019-07-16T14:35:30Z

app/models/host/base.rb

@@ -366,7 +375,8 @@ def render_template(template:, **params)
    end

    def uptime_seconds
-      self.uptime_fact&.value&.to_i
+      boot_time = self&.reported_data&.boot_time
+      boot_time.nil? ? Time.zone.now.to_i - boot_time.to_i : nil


This is reversed, test failures are most likely related to that:

Suggested change

boot_time.nil? ? Time.zone.now.to_i - boot_time.to_i : nil

boot_time.present? ? Time.zone.now.to_i - boot_time.to_i : nil

# OR

boot_time && Time.zone.now.to_i - boot_time.to_i

yeah, I figured while fixing tests :-)

ezr-ondrej

Looks good, will retest with puppet after the jenkins ack and I believe we are set 👍

ares · 2019-07-16T15:57:20Z

[test foreman] (broken pipe)

ezr-ondrej

Tests are failing, but it's just because of the tests.

ezr-ondrej · 2019-07-16T21:12:39Z

test/models/host_test.rb

+      boot_time = 1.day.ago
+      host.create_reported_data(:boot_time => boot_time)
+      refute_nil host.uptime_seconds
+      host.clear_data_on_build


I believe, we need to reload after this, the record is destroyed, but still loaded.

in fact it needed more tweaking so that clear_data_on_build would actually work (setting old and build)

ezr-ondrej · 2019-07-16T21:27:06Z

test/models/host_test.rb

+      assert host.import_facts(raw['facts'])
+      first_boot_time = host.reported_data.boot_time
+      refute_nil host.reported_data.boot_time
+      assert_kind_of DateTime, host.reported_data.boot_time


This fails, but I don't like asserting exact type match in dynamicaly typed language anyway.
Cant we predict the value and assert the values are equal? The equality should be handled more predictably.
Time is bit tricky, but something like this would maybe do?

Suggested change

assert_kind_of DateTime, host.reported_data.boot_time

assert (Time.now - host.reported_data.boot_time - raw['facts']['uptime_seconds'].to_i.seconds) < 2.seconds

I tried to do something smarter with freeze_time

Needs fix tests

houndci-bot · 2019-07-17T13:36:47Z

test/models/host_test.rb

+      freeze_time do
+        host = Host.import_host(raw['name'], 'puppet')
+        assert host.import_facts(raw['facts'])
+        first_boot_time = host.reported_data.boot_time


Lint/UselessAssignment: Useless assignment to variable - first_boot_time.

oh dear, fixed cop now

houndci-bot · 2019-07-17T14:56:46Z

test/models/host_test.rb

+    test 'should import facts boot time to report data facet' do
+      refute Host.find_by_name('sinn1636.lan')
+      raw = read_json_fixture('facts/facts_with_certname.json')
+      first_boot_time = nil 


Layout/TrailingWhitespace: Trailing whitespace detected.

ezr-ondrej · 2019-07-18T09:25:49Z

test/models/host_test.rb

+      raw = read_json_fixture('facts/facts_with_certname.json')
+      first_boot_time = nil
+      freeze_time do
+        host = Host.import_host(raw['name'], 'puppet')


you should define the host variable outside of the block. It is undefined in the other one.

yeah I did the same for first_boot_time already, importing needs to happen while time is frozen... feel free to wait with the review until tests are green, I'm monitoring jenkins output here :-)

A new Reported Data facet is introduced. This should be used for storing reported information from various fact parsers. Fact Parser now defines a new optional to implement method, to gather boot time in seconds. Some fact sources will need to construct this based on uptime. Hosts can be now search by boot_time search attribute.

ares · 2019-07-19T09:00:39Z

I believe that failures are now unrelated and this beast can be merged :-) (please keep 2 separate commits)

ezr-ondrej

Code LGTM, works fine.
Thanks @ares 👍 and thanks @timogoebel for most of the review, to @ekohl and @lzap for valuable notes 😊 Merging!

theforeman-bot added Needs testing Not yet reviewed labels Jun 14, 2019

ares force-pushed the fix/26393 branch from a4bfd21 to 3feffe4 Compare June 14, 2019 09:51

ares commented Jun 14, 2019

View reviewed changes

timogoebel reviewed Jun 14, 2019

View reviewed changes

timogoebel requested changes Jun 14, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Jun 14, 2019

lzap mentioned this pull request Jun 19, 2019

Fixes #26984 - maximum subtree fact filter #6850

Merged

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jun 30, 2019

ares mentioned this pull request Jun 30, 2019

Refs #26393 - import boot time information Katello/katello#8187

Merged

ares force-pushed the fix/26393 branch from a7c0dd4 to 4627d6d Compare June 30, 2019 11:07

This was referenced Jun 30, 2019

Refs #26393 - import boot time fact theforeman/foreman_chef#91

Merged

Refs #26393 - parse boot time fact theforeman/foreman_salt#104

Closed

Fixes #28073 - parse boot time fact theforeman/foreman_ansible#279

Merged

houndci-bot reviewed Jun 30, 2019

View reviewed changes

timogoebel reviewed Jun 30, 2019

View reviewed changes

houndci-bot reviewed Jul 16, 2019

View reviewed changes

ekohl reviewed Jul 16, 2019

View reviewed changes

ezr-ondrej self-assigned this Jul 16, 2019

ezr-ondrej removed the Needs re-review label Jul 16, 2019

ezr-ondrej requested changes Jul 16, 2019

View reviewed changes

theforeman-bot added the Waiting on contributor label Jul 16, 2019

ares force-pushed the fix/26393 branch from 8c347b8 to 733f2af Compare July 16, 2019 14:43

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jul 16, 2019

ares force-pushed the fix/26393 branch from 733f2af to 360c197 Compare July 16, 2019 14:48

ezr-ondrej reviewed Jul 16, 2019

View reviewed changes

ezr-ondrej self-requested a review July 16, 2019 15:03

ezr-ondrej previously approved these changes Jul 16, 2019

View reviewed changes

theforeman-bot removed the Needs re-review label Jul 16, 2019

ezr-ondrej reviewed Jul 16, 2019

View reviewed changes

ares force-pushed the fix/26393 branch from 360c197 to 8cfb6ea Compare July 17, 2019 13:36

houndci-bot reviewed Jul 17, 2019

View reviewed changes

ares force-pushed the fix/26393 branch from 8cfb6ea to 94751b6 Compare July 17, 2019 14:56

houndci-bot reviewed Jul 17, 2019

View reviewed changes

ares force-pushed the fix/26393 branch from 94751b6 to 7955df7 Compare July 17, 2019 15:50

ezr-ondrej reviewed Jul 18, 2019

View reviewed changes

ares force-pushed the fix/26393 branch from 7955df7 to a804fd0 Compare July 18, 2019 10:34

ezr-ondrej approved these changes Jul 19, 2019

View reviewed changes

ezr-ondrej merged commit 42feb26 into theforeman:develop Jul 19, 2019

lzap mentioned this pull request Oct 22, 2019

Refs #27995 - keep set_reported_data in Host::Base #7116

Merged

ezr-ondrej mentioned this pull request Jun 15, 2020

Fixes #30126 - use facets fact parsing #7748

Merged

	fields += [[_("Boot time"), date_time_relative(host&.reported_data&.boot_time) \|\| _('Not reported')]]
	fields += [[_("Boot time"), (boot_time = host&.reported_data&.boot_time) ? date_time_relative(boot_time) : _('Not reported')]]

-      boot_time.nil? ? Time.zone.now.to_i - boot_time.to_i : nil
+      boot_time.present? ? Time.zone.now.to_i - boot_time.to_i : nil
+      # OR
+      boot_time && Time.zone.now.to_i - boot_time.to_i

	assert_kind_of DateTime, host.reported_data.boot_time
	assert (Time.now - host.reported_data.boot_time - raw['facts']['uptime_seconds'].to_i.seconds) < 2.seconds

Fixes #26393 - fix uptime calculation for btime #6843

Fixes #26393 - fix uptime calculation for btime #6843

Conversation

ares commented Jun 14, 2019

theforeman-bot commented Jun 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timogoebel left a comment

Choose a reason for hiding this comment

timogoebel commented Jun 16, 2019

lzap commented Jun 18, 2019

ekohl commented Jun 18, 2019

lzap commented Jun 18, 2019

ares commented Jun 24, 2019

timogoebel commented Jun 24, 2019

ares commented Jun 25, 2019

timogoebel commented Jun 25, 2019

ares commented Jun 30, 2019

Choose a reason for hiding this comment

timogoebel left a 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

timogoebel commented Jun 30, 2019

ares commented Jul 3, 2019

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

ekohl Jul 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej left a 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

ezr-ondrej left a comment

Choose a reason for hiding this comment

ares commented Jul 16, 2019

ezr-ondrej left a 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

ares commented Jul 19, 2019

ezr-ondrej left a comment

Choose a reason for hiding this comment

ekohl Jul 16, 2019 •

edited