virtcontainers: Set ppc64le maxmem depending on qemu version #417
Conversation
| @@ -64,6 +64,9 @@ const qmpCapErrMsg = "Failed to negoatiate QMP capabilities" | |||
|
|
|||
| const defaultConsole = "console.sock" | |||
|
|
|||
| var qemuMajorVersion int | |||
| var qemuMinorVersion int | |||
There was a problem hiding this comment.
I wonder if we want to store these in one of the types, even though they are only being used for PPC64le atm.
There was a problem hiding this comment.
@jodh-intel : Should we? I am not sure if its gonna get consumed by any other architectures atm.
There was a problem hiding this comment.
I would definitely include those as part of the qemu structure since this is an info about the hypervisor. And I agree with @jodh-intel that even if it's only used by ppc64, this is not a reason to define some global variable there.
There was a problem hiding this comment.
@sboeuf : Okay, I shall add qemuMajorVersion and qemuMinorVersion into the qemu structure. However, I am unsure of what would be the most efficient way of exposing it in func (q *qemuPPC64le) memoryTopology. One way is to get qemu version info in func (q *qemu) init(sandbox *Sandbox) error and pass via newQemuArch resulting in change of signature of newQemuArch everywhere.
There was a problem hiding this comment.
It's a bit fiddly, but we could add them to qemuArchBase like this:
There was a problem hiding this comment.
@nitkon oh yeah my bad, I forgot qemu structure was not the interface related to qemu_ppc64le. As @jodh-intel suggested, qemuArchBase should be the one.
@jodh-intel, I have taken a look at your suggestion but I thought we could simplify this by simply having those fields being called from qemu_ppc64le structure (no need for getters and setters). Isn't it possible since this structure inherits from qemuArchBase ?
There was a problem hiding this comment.
I'm not quite sure what you're saying tbh. I'm hoping there is a simpler way though as I'm not particularly happy with what I came up with :)
There was a problem hiding this comment.
@jodh-intel : So I was going through the implementation but noticed the following.
Printing q.qemuMajorVersion, q.qemuMinorVersion in "func (q qemuArchBase) getQEMUVersion() (major, minor int) {" shows me the following: level=info msg="DEBUG: versions: major: 0, minor: 0" arch=amd64 name=kata-runtime pid=6215 .
There was a problem hiding this comment.
@jodh-intel : I am a bit confused by the reference implementation. We are setting qemuMajorVersion and qemuMinorVersion using one qemu instance and getting it with another qemu instance. (resulting it in 0,0 values).
IIUC, we need to set/get using the same qemu instance
virtcontainers/qemu_ppc64le.go
Outdated
| if hostMemoryMb > defaultMemMaxPPC64le { | ||
| hostMemoryMb = defaultMemMaxPPC64le | ||
| } else { | ||
| if qemuMajorVersion >= 2 && qemuMinorVersion >= 10 { |
There was a problem hiding this comment.
I'd consider adding a couple of logging calls here, one for each scenario so it's clear what's happening from the log.
What we cannot reliably check the guest kernel version so maybe your debug entry for handling qemu 2.10+ should mention the 4.11 guest kernel assumption.
There was a problem hiding this comment.
@jodh-intel : Done. Updated my PR.
|
PSS Measurement: Memory inside container: |
The "Failed to allocate HTAB of requested size, try with smaller maxmem" error in ppc64le occurs when maxmem allocated is very high. This got fixed in qemu 2.10 and kernel 4.11. Hence put a maxmem restriction of 32GB per kata-container if qemu version less than 2.10 Fixes: #415 Signed-off-by: Nitesh Konkar <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #417 +/- ##
==========================================
- Coverage 63.79% 63.76% -0.04%
==========================================
Files 87 87
Lines 8811 8825 +14
==========================================
+ Hits 5621 5627 +6
- Misses 2587 2594 +7
- Partials 603 604 +1
Continue to review full report at Codecov.
|
|
@jodh-intel : I see the following error in the failed CI. Should the CI be restarted? |
|
Jobs restarted. |
|
PSS Measurement: Memory inside container: |
|
@jodh-intel : All |
|
Yes, but we have 1 failure. You'll also notice that you still need 1 more approval :) |
|
On the F27 CI: Known and wholly unrelated @jodh-intel @chavafg ? If so, then we can merge this I think (the button is not green, but is 'pressable' ;-) ) |
|
/cc @sboeuf |
|
@nitkon please backport this PR to stable branches if not present. |
|
Nevermind, it's already part of the stable branches. |
update architecture.md link, since it has moved to within the design/ directory. contributes to: kata-containers#417 Signed-off-by: Gabi Beyer <[email protected]>
Re-vendor the testify package to obtain the newest assert test
functions.
Shortlog:
87b1dfb Remove returns info from assertion docs
bf45a85 Skip vendored packages from tests
2c9035a Changed mock assertions text output to be non-unicode friendly
05aa1d4 Remove outdated info from README
b91bfb9 Move from Godep to dep for dependency management
9ede17e Drop old Go versions from tests
8de2544 Fix HTTP assertions to be consistent with the rest
b57b6d1 Add FileExists and DirExists assertions
b3596e9 Fxinng After(time.Duration) to wait properly
cbf22d8 Support nil arguments to Argument Matchers
42baa3e Nil check in Implements assertion
b3dfaa9 fail: add test name for logged output
6e494f9 put closestCall.Arguments to the expected side of the diff
51464da Consider empty/nil arrays as matching elements
6f306a6 Reuse aLen and bLen variables
76de30e Actually fail tests
ae87ba6 Ran go generate
8bd27dd Compress some newlines
bf57a5d ElementsMatch array/slice assertion ignoring order
0c49dd9 Replace is with in
9fb9de1 Make NotSubset actually fail the test on nil subset
8ccf48a Allow a method call to be optional
aa8279e travis: add go1.9
c0f1d44 indent actual value for better comparison with expected value
249123e Implement delta comparison for map values
88a414d generalize Empty assertion
2aa2c17 Fix unprotected call fields access in MethodCalled()
890a5c3 Issue kata-containers#469 fix
05e8a0e Fix the actuality of InEpsilon
2f1cd6b time.Duraions are numbers too
b1f9894 Fix InDelta expected nan check message
4b92304 Fix actual float conversion error message under calcRelativeError
f6abca5 Added assert.PanicsWithValue + tests
e964b17 add MethodCalled to the mock
46b3c82 Simple validation of Equal/NotEqual assertion arguments
cc18973 Fix gogenerate travis check
e179a18 Travis check go generate has been run
3458981 Run go vet in travis
c7668ea Fixes kata-containers#339 - Add `assertion`f assertions like Errorf and Equalf
c33f336 Fix vet warnings and go generate to update docs
3104bf5 Use Go 1.7 subtests so suites can properly nest
bd79c01 Fix race condition on mock package's Called
edd8e19 Run go generate to syn generated assertions
78be756 Fixed HTTP assertions messages formatting and removed wrong test message
3a59a58 add Subset and NotSubset assertions
dd57c7b Check code is formatted in travis
f712be9 Revert "add mock.MethodCalled(methodName, args...)"
34687eb Revert "diffArguments: remove unnecessary range-for (kata-containers#417)"
2b76a97 Revert "Added goconvey style assertions to the mock package"
b6296e3 fix(docs): correct Error usage examples
d2f3716 Add msgAndArgs pass forward to InDelta from InDeltaSlice
158f9d0 Check that there is a directory before trying to access it.
18cfa68 Added extra unit test for function with mixed variadic arguments.
b1f1bcb fix typo
09f61d7 assert: fix error reporting when error contains escape sequences
5c861cc diffArguments: remove unnecessary range-for (kata-containers#417)
bc11a6e Tighten language by increasing overall consistency in wording in texts and argument names: use 'actual' instead of 'received'
115ab90 Provide argument name `args` in function signature
faf0710 Added goconvey style assertions to the mock package
97c0e43 compare bytes with bytes.Equal instead of reflect.DeepEqual
17a0bd5 add mock.MethodCalled(methodName, args...)
9afdd65 Check number of provided arguments vs mocked
287336f travis: rm broken go releases
32d79c5 travis: check if these platforms are broken
5c9da49 HTTP code status assertions now fail tests
332ae0e Add Equal test comparing nil with non-nil
434d5c1 Fixed minor typo
3928f57 Add comments for Equal and NotEqual to clarify pointer comparison
13b9dd4 Fix typos in comments in _codegen/main.go
6835870 Remove isNumericType check
cbd71e7 When diffing with spew, use a format that doesn't include pointer addresses (which generate false negatives). This updates go-spew to 04cdfd42973bb9c8589fd6a731800cf222fde1a9.
ddb91ee Ensure that assert.Fail properly align its output
5e72f93 Remove timestamp from callback
976c720 Format generated code
4b9bfb8 run go/format on generated code
4ccf54a Clearer messages
bf7a93e Add timestamp
fcedfe2 Add callbacks to run before and after each test
8879a01 Unlock and relock not needed anymore. Addresses kata-containers#167.
4a6e516 Added test to avoid regresions of kata-containers#167.
547cd8e Release lock before .WaitUntil, as it may cause a deadlock when testing parallel objects. Address kata-containers#167: Unable to test concurrent objects
Signed-off-by: James O. D. Hunt <[email protected]>
Add container to sandbox list until it is created. This commit makes sure to add the container to the sandbox until it is created. This helps to avoid other go routines uses the container structure with nil pointers (because is not fully created). Fixes: kata-containers#417 Signed-off-by: Jose Carlos Venegas Munoz <[email protected]>
…nerd-version-2.x docs: Clarifying minimum version of containerd for annotations
The "Failed to allocate HTAB of requested size,
try with smaller maxmem" error in ppc64le occurs
when maxmem allocated is very high. This got fixed
in qemu 2.10 and kernel 4.11. Hence put a maxmem
restriction of 32GB per kata-container if qemu
version less than 2.10
Fixes: #415
Signed-off-by: Nitesh Konkar [email protected]