Opened 7 years ago

Closed 7 years ago

#2297 closed defect (fixed)

Fix isc-sysinfo free memory, uptime, smp output

Reported by: jreed Owned by: jinmei
Priority: low Milestone: Sprint-20121106
Component: sysinfo Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 5.5 Internal?: no

Description

I saw this on n10 with isc-sysinfo:

Memory
 + Total: 1058455552 bytes
 + Free: -156352512 bytes

Negative number.

 + Uptime: 26230139 seconds

Not human friendly.

+ SMP kernel: no

I'd think that any amd64 FreeBSD kernel is SMP capable.

Subtickets

Change History (13)

comment:1 Changed 7 years ago by muks

  • Summary changed from isc-sysinfo free memory, uptime, smp to Fix isc-sysinfo free memory, uptime, smp output

comment:2 Changed 7 years ago by jelte

  • Milestone changed from New Tasks to Sprint-20121023

comment:3 Changed 7 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:4 Changed 7 years ago by jinmei

trac2297 is ready for review.

First note: I made a couple of unrelated changes:

  • introduce run_sysinfo script so we can run it in the build tree
  • removed the call to isc.util.process.rename(). see the commit log.

Regarding the support of SMP in kernel, the fix is a kind of ugly
hack (7d777ce), but I couldn't come up with a cleaner way.

Regarding the negative memory, I don't know the intent of the original
code, but "'physical memory' - 'active VM memory'" shouldn't be
valid calculation anyway. Showing the size of free-list may not be
exactly what we want to show, but it seems to be closer to e.g. what
top(1) reports. I applied the same change to the OpenBSD version.

Proposed changelog:

496.?	[bug]		jinmei
	Fixed several issues in isc-sysinfo:
	- make sure it doesn't report a negative value for free memory
	  size (this happened on FreeBSD, but can possibly occur on other
	  BSD variants)
	- correctly identifies the SMP support in kernel on FreeBSD
	- print more human readable uptime as well as the time in seconds
	(Trac #2297, git TBD)
Last edited 7 years ago by jinmei (previous) (diff)

comment:5 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:6 Changed 7 years ago by muks

  • Owner changed from UnAssigned to muks

Picking

comment:7 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

  • I have pushed some minor cleanup commits. Please check them.
  • make check fails:
    Running test: process_rename_test.py
    F
    ======================================================================
    FAIL: test_calls (__main__.TestRename)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/home/muks/bind10/src/bin/tests/process_rename_test.py", line 62, in test_calls
        self.__scan(d, script, fun)
      File "/home/muks/bind10/src/bin/tests/process_rename_test.py", line 32, in __scan
        "Didn't find a call to isc.util.process.rename in " + prettyname)
    AssertionError: None is not true : Didn't find a call to isc.util.process.rename in src/src/bin/sysinfo/isc-sysinfo
    
    ----------------------------------------------------------------------
    Ran 1 test in 0.566s
    
  • Can you not use str(t) on timedelta object for the pretty printing?

comment:8 in reply to: ↑ 7 Changed 7 years ago by jinmei

Thanks for the review.

Replying to muks:

  • I have pushed some minor cleanup commits. Please check them.

These look good. Thanks.

  • make check fails:

Oops, lazy me. I've fixed it, but just by introducing a filter for
excluding the test and specify isc-sysinfo there. I believe not doing
rename() for sysinfo is correct (mainly for reducing dependency), so
excluding it from this test is actually correct, but if it seems
controversial I'm okay with reverting this change. It's not related
to the subject of this ticket anyway.

  • Can you not use str(t) on timedelta object for the pretty printing?

Ah, good catch. I simply overlooked it, and that should be definitely
better than reinventing the wheel. I've still left some test cases
where the result is predictable, but removed one other case due to
possible margin errors. And, in any case, now that it's a simple
library call I don't think it's not that important to check each
detailed behavior.

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:10 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

I've pushed a minor update to a comment. If it's okay, please go ahead and merge.

comment:11 in reply to: ↑ 10 Changed 7 years ago by jinmei

Replying to muks:

Hi Jinmei

I've pushed a minor update to a comment. If it's okay, please go ahead and merge.

Thanks, your change looks okay, branch merged, and closing.

comment:12 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 5.5

comment:13 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.