(FACT-1784) Correctly initialize lparstat data source #31

caseywilliams · 2017-10-24T01:09:24Z

Outside of AIX, the data in the lparstat source was left uninitialized;
This caused false positives for LPAR/WPAR on power8 machines.

coveralls · 2017-10-24T01:16:28Z

Coverage increased (+0.1%) to 91.146% when pulling 170038c on caseywilliams:lparstat-power8 into f43c12f on puppetlabs:master.

branan · 2017-10-31T21:05:54Z

lib/src/sources/lparstat_source.cc

    {
        if (!data_) {
-            data_.reset(new lparstat_data);
+            data_.reset(new lparstat_data {"", 0, 0, 0});


I think it would be better to just give lparstat_data a proper constructor, so that it is always correctly initialized

coveralls · 2017-10-31T21:43:45Z

Coverage increased (+0.1%) to 91.152% when pulling 2504c83 on caseywilliams:lparstat-power8 into f43c12f on puppetlabs:master.

ekinanp · 2017-11-01T18:19:38Z

lib/tests/sources/lparstat_source.cc

        REQUIRE(lparstat_source.wpar_configured_id() == 1);
    }
+
+    WHEN("An lparstat executable is available but this is not an AIX machine") {


This test looks like it's just making sure that the lparstat data is initialized correctly. I'd change the name to indicate that.

ekinanp

The feature changes LGTM. Only suggestion I have is for the tests. Looking at how the detector tests are structured, it seems like we determine whether something runs inside LPAR/WPAR or outside LPAR/WPAR from just oslevel's output. But the detector functions look at partition_number (for LPAR) and wpar_key (for WPAR) to detect those hypervisors -- something that lparstat displays, not oslevel. So my suggestion would be to make oslevel valid for both those tests, but change the lparstat output to something invalid. Then in the lparstat source tests, I'd maybe add some tests that indicates "If we're on an AIX machine and lparstat is available, we collect its information" and "If we are not on an AIX machine and lparstat is available, we don't collect anything".

caseywilliams · 2017-11-01T18:31:54Z

@ekinanp as I understand it, WPARs were introduced in AIX 6.1; If you try to run lparstat -W, (-W is the flag to include the WPAR information along with the LPAR information) pre-6.1, the command errors, so we need to check the oslevel before trying that. If you check out the lparstat source implementation, it checks and stores the oslevel output first to determine which flags to use, then calls lparstat after that - I think that's valid, but I may be misreading your comment, too. I'll push an update in just a second with updates to make the tests clearer.

ekinanp · 2017-11-01T18:44:06Z

@caseywilliams Yes, but those details (checking oslevel, making sure it is > 6.1 for WPAR) are in the lparstat source, not the detectors. The detectors are only concerned with some parts of what the source returns (partition_number and wpar_key for LPAR and WPAR, respectively) when it comes to hypervisor detection after the source is done processing its information. That is why I suggest adding the oslevel specific tests in the lparstat source tests, and keep the detector tests focused on just lparstat related stuff (think of it as "mocking out" the lparstat source, if that makes sense).

caseywilliams · 2017-11-01T18:46:19Z

@ekinanp oh! I was looking in the wrong place - you're right, that could be clearer - I'll change it

ekinanp · 2017-11-01T18:46:39Z

lib/tests/sources/lparstat_source.cc

+    }
+
+    WHEN("An lparstat executable is available but this is not an AIX machine") {
+        THEN("the source does not attempt to collect any data and empty values are correctly initialized") {


Yes, this is much clearer. Thanks.

coveralls · 2017-11-01T18:48:27Z

Coverage increased (+0.2%) to 91.192% when pulling fe97485 on caseywilliams:lparstat-power8 into f43c12f on puppetlabs:master.

coveralls · 2017-11-01T19:07:06Z

Coverage increased (+0.3%) to 91.282% when pulling 8a9c091 on caseywilliams:lparstat-power8 into f43c12f on puppetlabs:master.

ekinanp · 2017-11-01T20:20:37Z

lib/tests/sources/lparstat_source.cc

+        }
    }

    WHEN("WPAR information is available") {


I'd probably change the structure to something like:

WHEN("LPAR information is available") { WHEN("WPAR information is unavailable") { } WHEN("WPAR information is available") { } }

because you check the lpar information in both cases anyways.

ekinanp · 2017-11-01T20:24:08Z

lib/tests/detectors/lpar_detector.cc

        }
    }
+
+    WHEN("Running outside of an LPAR but `lparstat` is available") {


I'd change these to:

WHEN("Running on a non-AIX machine") { THEN("the result is not valid") { } } WHEN("Running on an AIX machine") { WHEN("lparstat is available") { THEN(...) } WHEN("lparstat is unavailable") { THEN(...) } }

makes it clearer that these detectors only make sense for AIX machines.

ekinanp · 2017-11-01T20:25:25Z

lib/tests/detectors/wpar_detector.cc

        }
    }
+
+    WHEN("Running outside of a WPAR but `lparstat` is available") {


Same idea as my comment above for the lpar detector.

coveralls · 2017-11-01T20:53:38Z

Coverage increased (+0.5%) to 91.488% when pulling f318da5 on caseywilliams:lparstat-power8 into f43c12f on puppetlabs:master.

coveralls · 2017-11-01T22:44:11Z

Coverage increased (+0.4%) to 91.42% when pulling 4f3243d on caseywilliams:lparstat-power8 into f43c12f on puppetlabs:master.

coveralls · 2017-11-02T00:49:01Z

Coverage increased (+0.5%) to 91.457% when pulling 07fd21c on caseywilliams:lparstat-power8 into f43c12f on puppetlabs:master.

ekinanp

Nice! I have only one comment.

ekinanp · 2017-11-02T01:57:40Z

lib/tests/detectors/wpar_detector.cc

-    WHEN("Running inside a WPAR") {
-        lparstat_fixture lparstat_source {"7.1.0.0", "output/lparstat/wpar.txt"};
+    WHEN("Running on AIX") {
+        WHEN("Running inside an LPAR") {


Do you think the LPAR WHEN is necessary? i.e. Does it have to run under LPAR for it to run inside WPAR or can it run inside WPAR independently of LPAR? If it's the latter, it might be helpful to remove the WHEN("Running inside an LPAR") part so that someone looking at this won't think that WPAR has to be in LPAR.

Outside of AIX, the data in the lparstat source was left uninitialized; This caused false positives for LPAR/WPAR on power8 machines.

coveralls · 2017-11-02T16:17:57Z

Coverage increased (+0.4%) to 91.445% when pulling 204db80 on caseywilliams:lparstat-power8 into f43c12f on puppetlabs:master.

geoffnichols requested a review from mcdonaldseanp October 30, 2017 21:53

branan suggested changes Oct 31, 2017

View reviewed changes

caseywilliams force-pushed the lparstat-power8 branch from 170038c to 2504c83 Compare October 31, 2017 21:29

branan approved these changes Oct 31, 2017

View reviewed changes

geoffnichols requested a review from ekinanp November 1, 2017 17:14

ekinanp reviewed Nov 1, 2017

View reviewed changes

ekinanp suggested changes Nov 1, 2017

View reviewed changes

caseywilliams force-pushed the lparstat-power8 branch from 2504c83 to fe97485 Compare November 1, 2017 18:36

ekinanp reviewed Nov 1, 2017

View reviewed changes

caseywilliams force-pushed the lparstat-power8 branch from fe97485 to 8a9c091 Compare November 1, 2017 18:56

ekinanp suggested changes Nov 1, 2017

View reviewed changes

caseywilliams force-pushed the lparstat-power8 branch from 8a9c091 to f318da5 Compare November 1, 2017 20:40

caseywilliams force-pushed the lparstat-power8 branch from f318da5 to 4f3243d Compare November 1, 2017 22:35

caseywilliams force-pushed the lparstat-power8 branch from 4f3243d to 07fd21c Compare November 2, 2017 00:39

ekinanp suggested changes Nov 2, 2017

View reviewed changes

mcdonaldseanp approved these changes Nov 2, 2017

View reviewed changes

(FACT-1784) Correctly initialize lparstat data source

204db80

Outside of AIX, the data in the lparstat source was left uninitialized; This caused false positives for LPAR/WPAR on power8 machines.

caseywilliams force-pushed the lparstat-power8 branch from 07fd21c to 204db80 Compare November 2, 2017 16:11

ekinanp approved these changes Nov 2, 2017

View reviewed changes

ekinanp merged commit 32a9af0 into puppetlabs:master Nov 2, 2017

caseywilliams deleted the lparstat-power8 branch November 7, 2017 12:30

(FACT-1784) Correctly initialize lparstat data source #31

(FACT-1784) Correctly initialize lparstat data source #31

Uh oh!

Conversation

caseywilliams commented Oct 24, 2017

Uh oh!

coveralls commented Oct 24, 2017

Uh oh!

branan Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 31, 2017

Uh oh!

ekinanp Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

ekinanp left a comment

Choose a reason for hiding this comment

Uh oh!

caseywilliams commented Nov 1, 2017

Uh oh!

ekinanp commented Nov 1, 2017

Uh oh!

caseywilliams commented Nov 1, 2017

Uh oh!

ekinanp Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 1, 2017

Uh oh!

coveralls commented Nov 1, 2017

Uh oh!

ekinanp Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

ekinanp Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

ekinanp Nov 1, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 1, 2017

Uh oh!

coveralls commented Nov 1, 2017

Uh oh!

coveralls commented Nov 2, 2017

Uh oh!

ekinanp left a comment

Choose a reason for hiding this comment

Uh oh!

ekinanp Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ekinanp Nov 2, 2017 •

edited

Loading