Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

kata-env: Fix amd64 VM container capable check #661

Merged
merged 1 commit into from
Aug 30, 2018

Conversation

jodh-intel
Copy link
Contributor

Fix nasty bug which resulted in kata-env showing VMContainerCapable = true even on amd64 systems without virtualisation support (thankfully kata-check still showed the correct results).

Added arch-specific tests to avoid any possibility of regression.

Fixes #660.

Signed-off-by: James O. D. Hunt [email protected]

@egernst egernst added the review label Aug 29, 2018
@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 165135 KB
Proxy: 4212 KB
Shim: 8860 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003620 KB

@ghost
Copy link

ghost commented Aug 29, 2018

The builds are failing....

BTW why not just add support for all architectures at once? That seems easier.

For Z i need "vx" feature to be available. I have aliased features to flag on x86 so so may test the same way you do for x86.

@nitkon What do you require for Power?
@Pennyzct any specific requirements for ARM?

@opendev-zuul
Copy link

opendev-zuul bot commented Aug 29, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 169105 KB
Proxy: 4073 KB
Shim: 8874 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003496 KB

@jodh-intel
Copy link
Contributor Author

Hi @ydjainopensource - the bug was specific to amd64. However, I've now added tests for the other architectures to ensure the correct behaviour.

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 167375 KB
Proxy: 4032 KB
Shim: 8788 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003572 KB

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 167621 KB
Proxy: 4117 KB
Shim: 8872 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003760 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Aug 29, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@opendev-zuul
Copy link

opendev-zuul bot commented Aug 29, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 167834 KB
Proxy: 4079 KB
Shim: 8842 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003440 KB

Fix nasty bug which resulted in `kata-env` showing
`VMContainerCapable = true` even on amd64 systems without virtualisation
support (thankfully `kata-check` still showed the correct results).

Added arch-specific tests to avoid any possibility of regression.

Fixes kata-containers#660.

Signed-off-by: James O. D. Hunt <[email protected]>
@opendev-zuul
Copy link

opendev-zuul bot commented Aug 30, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@jodh-intel
Copy link
Contributor Author

ping @egernst, @bergwolf, @devimc, @grahamwhaley.

This is a real bug folks - please review :)

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm
A brief description of how the bug manifested, and how that one line (afaict) change fixed it would have aided clarity of what was happening here ;-)
kudos for the mega test updates....

@jodh-intel
Copy link
Contributor Author

I'm not sure I dare re-push as don't want to lose another day waiting for the CI to run... ;(

@grahamwhaley
Copy link
Contributor

Sure, np - it was just a note.
My head tends to sit in the kernel land of 'describe what was wrong and what you did to fix it' for commit messages :-)

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 159327 KB
Proxy: 4168 KB
Shim: 8880 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003636 KB

@jodh-intel
Copy link
Contributor Author

Ping @amshinde.

@codecov
Copy link

codecov bot commented Aug 30, 2018

Codecov Report

Merging #661 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #661      +/-   ##
==========================================
+ Coverage   65.35%   65.35%   +<.01%     
==========================================
  Files          85       85              
  Lines        9879     9880       +1     
==========================================
+ Hits         6456     6457       +1     
  Misses       2766     2766              
  Partials      657      657

Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jodh-intel
Copy link
Contributor Author

CI bliss...

@jodh-intel jodh-intel merged commit ede6004 into kata-containers:master Aug 30, 2018
@egernst egernst removed the review label Aug 30, 2018
@sboeuf sboeuf added the bug Incorrect behaviour label Sep 12, 2018
@sboeuf
Copy link

sboeuf commented Sep 12, 2018

@jodh-intel please backport to stable branches.

@jodh-intel
Copy link
Contributor Author

@sboeuf - done - see #726 and #727.

@sboeuf
Copy link

sboeuf commented Sep 13, 2018

Thanks :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Incorrect behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants