Ignore negative deltas in cpu times when calculating percentages

[wiggin15]

See discussion in #1210

[giampaolo]

Uhm... scary (because these are very old critical parts). =)
Do you think you can add a test case which simulates this issue?
If it's too much effort I can do it for you.

[wiggin15]

Yes, this is a little scary 😬. A careful review of the logic is needed.
I added a test and checked that it fails before the fix.

[giampaolo]

I don't want to rush this. I will take a deep look at it when I have more free time (may be weeks or months).

[giampaolo]

I modified your unit tests so that it uses mock instead of playing with PROCFS_PATH:

    def test_cpu_steal_decrease(self):
        # Test cumulative cpu stats decrease. We should ignore this.
        # See issue #1210.

        def open_mock(name, *args, **kwargs):
            if name == "/proc/stat":
                return io.BytesIO(textwrap.dedent("""\
                    cpu   0 0 0 0 0 0 0 1 0 0
                    cpu0  0 0 0 0 0 0 0 1 0 0
                    cpu1  0 0 0 0 0 0 0 1 0 0
                    """).encode())
            else:
                return orig_open(name, *args, **kwargs)

        orig_open = open
        patch_point = 'builtins.open' if PY3 else '__builtin__.open'

        with mock.patch(patch_point, create=True, side_effect=open_mock) as m:
            # first call to "percent" functions should read the new stat file
            # and compare to the "real" file read at import time - so the
            # values are meaningless
            psutil.cpu_percent()
            assert m.called
            psutil.cpu_percent(percpu=True)
            psutil.cpu_times_percent()
            psutil.cpu_times_percent(percpu=True)

        def open_mock(name, *args, **kwargs):
            if name == "/proc/stat":
                return io.BytesIO(textwrap.dedent("""\
                    cpu   1 0 0 0 0 0 0 0 0 0
                    cpu0  1 0 0 0 0 0 0 0 0 0
                    cpu1  1 0 0 0 0 0 0 0 0 0
                    """).encode())
            else:
                return orig_open(name, *args, **kwargs)

        with mock.patch(patch_point, create=True, side_effect=open_mock) as m:
            # Increase "user" while steal goes "backwards" to zero.
            cpu_percent = psutil.cpu_percent()
            assert m.called
            cpu_percent_percpu = psutil.cpu_percent(percpu=True)
            cpu_times_percent = psutil.cpu_times_percent()
            cpu_times_percent_percpu = psutil.cpu_times_percent(percpu=True)
            self.assertNotEqual(cpu_percent, 0)
            self.assertNotEqual(sum(cpu_percent_percpu), 0)
            self.assertNotEqual(sum(cpu_times_percent), 0)
            self.assertNotEqual(sum(cpu_times_percent), 100.0)
            self.assertNotEqual(sum(map(sum, cpu_times_percent_percpu)), 0)
            self.assertNotEqual(sum(map(sum, cpu_times_percent_percpu)), 100.0)
            self.assertEqual(cpu_times_percent.steal, 0)
            self.assertNotEqual(cpu_times_percent.user, 0)

[giampaolo]

This I understand and it looks correct. It's also better than the previous version.

[giampaolo]

I'm having troubles understanding this part. Can you add a comment explaining what it does?

[wiggin15]

I just moved the calculation of 100 / all_delta that was inside the loop before to outside of the loop. So (100 * field_delta) / all_delta becomes field_delta * scale where scale is "pre-calculated". The max bit is to avoid ZeroDivisionError in the case of all_delta=0. In this case field_delta will also be 0 so the percentages will zero out anyway. I took this from top.c (https://gitlab.com/procps-ng/procps/blob/v3.3.12/top/top.c#L5083) but I can revert to the old logic if you prefer.

[giampaolo]

It wouldn't be bad to add:

assert t1._fields == t2._fields, (t1, t2)

[wiggin15]

Thanks for the review. I will update the unit tests, add the assert and try to write a comment for the scale variable :)

[giampaolo]

Thanks. This is great BTW. That logic has been "uncertain" for a long time, as you may have seen from my code comments.

[wiggin15]

Updated commit