virtcontainers: Set test qemu version for unit test

[nitkon]

Set test qemu major/minor version when
running unit test TestQemuPPC64leMemoryTopology

Fixes: #1308

Signed-off-by: Nitesh Konkar niteshkonkar@in.ibm.com

[jodh-intel]

Hi @nitkon - please could you provide some details as to why this is necessary?

[nitkon]

@jodh-intel : When we run the unit test, go test -run TestQemuPPC64leMemoryTopology the qemuMajorVersion, qemuMinorVersion do not get set (since func (q *qemu) waitSandbox(timeout int) error does not get called?)
So we set it explicitly when running unit test.

[grahamwhaley]

@nitkon I think you're going to have to explain this more to us, with some details. I don't see this being done for any of the other architectures for instance, so what is different for ppc64le?

I don't like the coding of 2.11 in the test either, as that will just rot pretty soon over time I suspect. If we really really have to pull in a version, then maybe we can extract it from the versions.yaml (I don't know if we do that for any other tests already for instance for other vars - maybe it isn't trivial).

/me mutters that showing lots of detail in the original report, including the failure logs and why this is very arch specific probably would have avoided us going around the loop ;-)

[jodh-intel]

@nitkon - any update?

[jodh-intel]

Ping @nitkon.

[raravena80]

@nitkon any updates? Thx!

[raravena80]

@nitkon nudge

[jodh-intel]

@nitkon - is this PR still required? I'm still curious as to why this change is required for ppc but not other architectures?

[jodh-intel]

Re-ping @nitkon. Is this still required or can we close?

[nitkon]

Apologies for the late response. For the record, I still see the issue on ppc64le...

=== RUN   TestQemuPPC64leMemoryTopology
--- FAIL: TestQemuPPC64leMemoryTopology (0.00s)
    assertions.go:239: ^M                          ^M   Error Trace:    qemu_ppc64le_test.go:52
        ^M      Error:          Not equal:
        ^M                      expected: qemu.Memory{Size:"120M", Slots:0xa, MaxMem:"2048M", Path:""}
        ^M                      actual: qemu.Memory{Size:"120M", Slots:0xa, MaxMem:"33280M", Path:""}
        ^M
        ^M                      Diff:
        ^M                      --- Expected
        ^M                      +++ Actual
        ^M                      @@ -3,3 +3,3 @@
        ^M                        Slots: (uint8) 10,
        ^M                      - MaxMem: (string) (len=5) "2048M",
        ^M                      + MaxMem: (string) (len=6) "33280M",
        ^M                        Path: (string) ""

[nitkon]

@jodh-intel @grahamwhaley: On ppc64le, max memory maxmem possible, depends on the qemu version. See this issue. Since qemuMajorVersion & qemuMinorVersion for this unit test TestQemuPPC64leMemoryTopology does not get set, hence it always defaults to this. Hence the test fails as expected and actual maxmem never match. That's why this PR sets a qemu version and runs this unit test against qemu version 2.11 & above.

[jodh-intel]

Could the test check the version of qemu and then alter it's behaviour based on that? That approach will be resilient to newer versions of qemu I think.

tal at the test constraints tests as they calculate the distro version and the kernel version, compute new versions based on the actual running versions and then test with them:

• https://github.com/kata-containers/runtime/blob/master/pkg/katatestutils/constraints_test.go#L1192

[nitkon]

/retest

[nitkon]

@jodh-intel : ptal

[jodh-intel]

Hi @nitkon - I'm afraid I'm confused now :) I thought this PR was to set a qemu version for testing? But that's not what this PR is doing....?!?

[nitkon]

@jodh-intel: Earlier qemu version did not get set and hence defaulted to 0,0 always. Now we are querying the installed qemu version and the test case behaves accordingly.

[jodh-intel]

Stepping back a moment, the code here is being modified to suit the tests and I don't think we should be doing that.

Instead, why don't you change qemuPPC64le.memoryTopology() so that it does something like this:

diff --git a/virtcontainers/qemu_ppc64le.go b/virtcontainers/qemu_ppc64le.go
index 6ef4015..29d0a1e 100644
--- a/virtcontainers/qemu_ppc64le.go
+++ b/virtcontainers/qemu_ppc64le.go
@@ -119,8 +119,10 @@ func (q *qemuPPC64le) cpuModel() string {
        return cpuModel
 }
 
-func (q *qemuPPC64le) memoryTopology(memoryMb, hostMemoryMb uint64, slots uint8) govmmQemu.Memory {
+// Defined as a variable to allow tests to modify the behaviour
+var qemuPPC64leMemoryTopology = qemuPPC64leMemoryTopologyImplementation
 
+func qemuPPC64leMemoryTopologyImplementation(q *qemuPPC64le, memoryMb, hostMemoryMb uint64, slots uint8, memoryOffset uint32) govmmQemu.Memory {
        if qemuMajorVersion >= 2 && qemuMinorVersion >= 10 {
                q.Logger().Debug("Aligning maxmem to multiples of 256MB. Assumption: Kernel Version >= 4.11")
                hostMemoryMb -= (hostMemoryMb % 256)
@@ -129,7 +131,11 @@ func (q *qemuPPC64le) memoryTopology(memoryMb, hostMemoryMb uint64, slots uint8)
                hostMemoryMb = defaultMemMaxPPC64le
        }
 
-       return genericMemoryTopology(memoryMb, hostMemoryMb, slots, q.memoryOffset)
+       return genericMemoryTopology(memoryMb, hostMemoryMb, slots, memoryOffset)
+}
+
+func (q *qemuPPC64le) memoryTopology(memoryMb, hostMemoryMb uint64, slots uint8) govmmQemu.Memory {
+       return qemuPPC64leMemoryTopology(q, memoryMb, hostMemoryMb, slots, q.memoryOffset)
 }
 
 func (q *qemuPPC64le) appendImage(devices []govmmQemu.Device, path string) ([]govmmQemu.Device, error) {

That way, your test can simply redefine the qemuPPC64leMemoryTopology variable to do something different: call getQemuVersion(), but only in the test code.

[nitkon]

@jodh-intel: Updated my PR.
Ptal at this approach.
/retest

[jodh-intel]

Ping @kata-containers/runtime - can we get some more eyes on this folks?

@nitkon - I'm afraid I really don't like the idea of launching an external program to check its version using regex's. Yes, some of that is done in kata-env, but that's a utility that is not -- by design -- in the fastpath.

I'll take another look tomorrow with "fresh eyes" but I still think there must be a way to do what you want entirely in test code without potentially impacting container start time purely to make a test pass.

[nitkon]

Thnx @jodh-intel. Moved the code out of the fastpath.
/retest

[jodh-intel]

Thanks @nitkon!

lgtm

[nitkon]

/retest
Fixed: travis-ci failure virtcontainers/qemu_ppc64le_test.go:50:16: cannot use err (variable of type error) as assert.TestingT value in argument to assert.Error: missing method Errorf (typecheck)

[nitkon]

Hi @grahamwhaley , ptal at this PR. Once this gets landed, can we get Power CI attached to this repo? :-) CI checks like make test & make check run clean on my local ppc64le machine

[grahamwhaley]

urgh, heh - fingers crossed that regexp holds up (as the qemu --version output varies somewhat depending on how it was built ime).
lgtm

[grahamwhaley]

@chavafg @GabyCT - can we clone the Power8 proxy job over to the runtime repo as well pls?, and let's see how it flies...

[chavafg]

I've cloned the job, should start testing PRs from now on