-
Notifications
You must be signed in to change notification settings - Fork 11
Conversation
@gorozco1 May I have a do-not-merge tag for this? |
@erick0z done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After Arch meeting the name chosen for the new qemu is qemu-cc
please change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general OK, but we need some changes.
qemu-cc/debian.changelog
Outdated
@@ -0,0 +1,5 @@ | |||
qemu-cc (2.9.0) stable; urgency=medium | |||
|
|||
* Initial version introducing Q35 type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And removal of pc-lite
@@ -0,0 +1 @@ | |||
setBadness('arch-dependent-file-in-usr-share', 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null at the end of the file, can you fix this please?
runtime/update_runtime.sh
Outdated
@@ -19,6 +19,7 @@ short_hashtag="${hash_tag:0:7}" | |||
[ -z "$hash_tag" ] && hash_tag=$VERSION || : | |||
|
|||
OBS_PUSH=${OBS_PUSH:-false} | |||
NON_STAGING=${NON_STAGING:-false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better call the variable STAGING
for clarity. And always set to true
runtime/update_runtime.sh
Outdated
-e '/Requires: linux-container*/d' -i cc-runtime.spec | ||
} | ||
|
||
if [ "$NON_STAGING" == true ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just change a little bit the logic with a variable called STAGING
runtime/update_runtime.sh
Outdated
-e "s/@HASH_TAG@/$short_hashtag/g;" \ | ||
-e "s/@cc_proxy_version@/$proxy_obs_ubuntu_version/" \ | ||
-e "s/@cc_shim_version@/$shim_obs_ubuntu_version/" \ | ||
-e "s/@qemu_lite_version@/$qemu_lite_obs_ubuntu_version/" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will need to remove the dependency of qemu-lite
or change the dependency for qemu-cc
qemu-cc/qemu-cc.spec
Outdated
|
||
%build | ||
export LANG=C | ||
%configure --prefix=/usr --disable-tools --disable-libssh2 --disable-tcmalloc --disable-glusterfs \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks neat, but makes maintenance more difficult: could you split this so each option is put on a separate line:
--option-1 \
--option-2 \
:
qemu-cc/qemu-cc.spec
Outdated
|
||
%files data | ||
%defattr(-,root,root,-) | ||
/usr/share/qemu/acpi-dsdt.aml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong to me - /usr/share/qemu/
will conflict with a distro-packaged version of qemu so you need to add a --libdir=
, --libexecdir=
and --datadir=
as is used in qemu-lite.spec
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we don't want conflicts with the packaged qemu. Fixing...
qemu-cc/qemu-cc.spec
Outdated
Docs for the qemu-cc package. | ||
|
||
%prep | ||
ls -la /home/abuild/rpmbuild/SOURCES/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed or is it some stray debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, that's my bad, thanks!
qemu-cc/qemu-cc.spec
Outdated
|
||
Requires: qemu-cc-bin | ||
Requires: qemu-cc-data | ||
Requires: qemu-cc-doc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessarily wrong, but why are we now packaging the docs when we didn't in qemu-lite.spec
?
@jodh-intel Thanks for the review, I've addressed the comments. |
@erick0z please also update the README.rst to list qemu-cc on |
qemu-cc/qemu-cc.spec
Outdated
%defattr(-,root,root,-) | ||
/usr/bin/qemu-cc-ga | ||
/usr/bin/qemu-cc-system-x86_64 | ||
/usr/bin/virtfs-proxy-helper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still going to conflict with a distro-packaged version of qemu. Please see how this is handled in qemu-lite.spec
in the %install
and %files
sections.
(Aside: this file is quite different to qemu-lite.spec
which makes comparison difficult).
qemu-cc/qemu-cc.spec
Outdated
BuildRequires : m4 | ||
BuildRequires : numactl-devel | ||
BuildRequires : python-devel | ||
BuildRequires : zlib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The qemu-lite.spec
specifies this as:
BuildRequires : zlib-devel
... which seems "more correct" since that will pull in the zlib
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
qemu-cc/qemu-cc.spec
Outdated
BuildRequires : libtool-ltdl-devel | ||
BuildRequires : libtool | ||
BuildRequires : m4 | ||
BuildRequires : numactl-devel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qemu-lite.spec
has a test for SuSE here and adds libnuma-devel
for that distro. Was that code dropped on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
qemu-cc/qemu-cc.spec
Outdated
--datadir=/usr/share/qemu-cc \ | ||
--libdir=/usr/lib64/qemu-cc \ | ||
--libexecdir=/usr/libexec/qemu-cc \ | ||
--disable-tools \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few differences here between qemu-lite.spec
. Please can you check and compare and either fix, or add a comment in the commit message explaining why each feature has been enabled/disabled compared to qemu-lite
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I took the configure flags from the travis setup (https://github.com/clearcontainers/tests/blob/master/.ci/setup_env_ubuntu.sh), so the package build remains the same as from-source build. I don't know the whole story of why some flags are enabled/disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got to be careful here for various reasons...
Here's how qemu-lite
is built in clr:
That is almost identical to qemu-lite.spec
except that that spec file adds two extra options:
$ comm -3 clr-qemu-lite.configure qemu-lite.spec
--disable-docs
--disable-static
It should be safe to add those two to qemu-cc.spec
.
Comparing qemu-lite.spec
and setup_env_ubuntu.sh
, it looks like setup_env_ubuntu.sh
is missing:
--disable-uuid (I think this is valid as we always specify a value to `-uuid`, so qemu doesn't need to generate one).
--enable-kvm (this is admittedly the default)
--enable-vhost-net (needed!)
--disable-qom-cast-debug
Also -- oddly -- setup_ubuntu_env.sh
adds the following...
--disable-cap-ng
--disable-attr
--enable-trace-backend=nop
I had thought that those disabled options were required for 9p. @grahamwhaley - do you have any input on that? I'm also not sure why it is explicitly enabling a trace backend, unless that effectively "disables" tracing which is enabled by default (the name suggests it might).
Finally, setup_ubuntu_env.sh
doesn't specify any CFLAGS
. That's fine for a CI build, but we need them for packaged builds for security reasons.
So, in summary, I think you need to remove the following from qemu-cc.spec
:
--disable-attr
--disable-cap-ng
... add the following to qemu-cc.spec
:
--disable-docs
--disable-qom-cast-debug
--disable-static
--disable-strip
--disable-uuid
--enable-attr
--enable-kvm
--enable-vhost-net
--enable-virtfs
--extra-cflags="-fno-semantic-interposition -O3 -falign-functions=32 -D_FORTIFY_SOURCE=2 -fPIE"
--extra-ldflags="-pie"
For the cflags
options, see:
- Makefile: Add build hardening with -fPIE flag intel/cc-oci-runtime#775
- configure.ac: Add -D_FORTIFY_SOURCE=2 optimization intel/cc-oci-runtime#778
@sboeuf, @mcastelino, @grahamwhaley, @anthonyzxu - please can you review this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've raised clearcontainers/tests#339 for further discussion on the problem of keeping the qemu*
build options in sync "everywhere".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jodh-intel nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some discussion on the public irc channel, we've decided it makes sense to land this first, then at at later date rework all scripts/config files that need qemu options to call the script on clearcontainers/tests#340.
As a mimimum, this PR needs to add --enable-vhost-net
. The other options listed above either should be fine to leave out for now (will not compromise functionality). The issue of the security options probably needs further discussion on clearcontainers/tests#340 as we're not building with those options anywhere afaik.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the attr
and cap-ng
options seem to be related to virtfs-proxy-helper(1).
See: clearcontainers/tests@d4fba34 which is part of clearcontainers/tests#340.
qemu-cc/qemu-cc.spec
Outdated
Requires: qemu-cc-bin | ||
Requires: qemu-cc-data | ||
|
||
BuildRequires : automake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qemu-lite.spec
adds a BuildRequires
on gcc-c++
. Is that needed still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see, those changes were introduced recently for the opensuse build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here #37
qemu-cc/qemu-cc.spec
Outdated
make install DESTDIR=%{buildroot} PREFIX=/usr | ||
mv %{buildroot}/usr/bin/qemu-system-x86_64 %{buildroot}/usr/bin/qemu-cc-system-x86_64 | ||
mv %{buildroot}/usr/bin/qemu-ga %{buildroot}/usr/bin/qemu-cc-ga | ||
mv %{buildroot}/usr/bin/virtfs-proxy-helper %{buildroot}/usr/bin/virtfs-cc-proxy-helper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the explicitness of this, but my worry is that if qemu introduce any new binaries, this package will end up trying to overwrite the distro-packaged version. That was the rational for using a loop in qemu-lite.spec
:
Note that if that technique is "re-adopted", it should also be used in the debian.rules
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the explicitness too :). In the case of a new binary, the build system will notice and complain about it because it has to be listed below the %files directive. Anyway, I'm ok with whatever approach is adopted. @jodh-intel @gorozco1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, if it's guaranteed to complain in that scenario, that's fine with me.
qemu-cc/qemu-cc.spec
Outdated
--datadir=/usr/share/qemu-cc \ | ||
--libdir=/usr/lib64/qemu-cc \ | ||
--libexecdir=/usr/libexec/qemu-cc \ | ||
--disable-tools \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got to be careful here for various reasons...
Here's how qemu-lite
is built in clr:
That is almost identical to qemu-lite.spec
except that that spec file adds two extra options:
$ comm -3 clr-qemu-lite.configure qemu-lite.spec
--disable-docs
--disable-static
It should be safe to add those two to qemu-cc.spec
.
Comparing qemu-lite.spec
and setup_env_ubuntu.sh
, it looks like setup_env_ubuntu.sh
is missing:
--disable-uuid (I think this is valid as we always specify a value to `-uuid`, so qemu doesn't need to generate one).
--enable-kvm (this is admittedly the default)
--enable-vhost-net (needed!)
--disable-qom-cast-debug
Also -- oddly -- setup_ubuntu_env.sh
adds the following...
--disable-cap-ng
--disable-attr
--enable-trace-backend=nop
I had thought that those disabled options were required for 9p. @grahamwhaley - do you have any input on that? I'm also not sure why it is explicitly enabling a trace backend, unless that effectively "disables" tracing which is enabled by default (the name suggests it might).
Finally, setup_ubuntu_env.sh
doesn't specify any CFLAGS
. That's fine for a CI build, but we need them for packaged builds for security reasons.
So, in summary, I think you need to remove the following from qemu-cc.spec
:
--disable-attr
--disable-cap-ng
... add the following to qemu-cc.spec
:
--disable-docs
--disable-qom-cast-debug
--disable-static
--disable-strip
--disable-uuid
--enable-attr
--enable-kvm
--enable-vhost-net
--enable-virtfs
--extra-cflags="-fno-semantic-interposition -O3 -falign-functions=32 -D_FORTIFY_SOURCE=2 -fPIE"
--extra-ldflags="-pie"
For the cflags
options, see:
- Makefile: Add build hardening with -fPIE flag intel/cc-oci-runtime#775
- configure.ac: Add -D_FORTIFY_SOURCE=2 optimization intel/cc-oci-runtime#778
@sboeuf, @mcastelino, @grahamwhaley, @anthonyzxu - please can you review this?
@gorozco1 Can you remove the do-not-merge label? This PR is ready to be merged |
@mcastelino could you mention the firmware that |
Hi @gorozco1 I believe it is 'bios.bin', or very similar. It depends on the machine type being used I think and the config. I ran into this when testing a hand installed q35 machine type with BIOS on a host that had no qemu :-) iirc, if we do not install the binary in our default search dir (such as /usr/share/qemu-lite/qemu or similar), then our qemu will pick up the system qemu installed bios file if it exists (such as from /usr/share/qemu). That will most likely work - but, we really should be installing our own version and to match the version of qemu we are using ;-) |
Currently this is PR is under testing. |
I just confirmed with @chavafg that the package include the firmware and the tests is getting it without any problems 👍 |
There were issues with CRI-O tests when running with |
@gorozco1 we need the following for our use of PC
@anthonyzxu Do we need any other firmware. |
@mcastelino I am almost sure we package those with |
only qemu/pc-bios/kvmvapic.bin are not needed. |
We provide all those that @mcastelino commented:
|
@sboeuf here are the console logs: |
This commit adds the OBS files to build qemu with Q35 type support. It also removes qemu-lite as dependency of the runtime. Fixes #25 Signed-off-by: Erick Cardona <erick.cardona.ruiz@intel.com>
@sboeuf @chavafg Just updated this PR. New packages can be found here: https://download.opensuse.org/repositories/home:/clearcontainers:/clear-containers-3/. I'll let you know when we can retrieve these from clearlinux.org. |
Thanks @erick0z ! |
@chavafg Can we compare the versions of devicemapper on ubuntu and fedora? Maybe there were some fixes pushed. |
@amshinde the Fedora version is newer than the Ubuntu one. Ubuntu:
Fedora:
|
Will try to update ubuntu's version and see if the problem goes away. |
@chavafg I think you mean you will try to downgrade devicemapper version for Ubuntu in order to check if the proble goes away, right ? |
@sboeuf no, ubuntu version is lower than the fedora one (where the tests work well), so I tried by updating devicemapper to the same version that fedora has. But after updating device-mapper to 1.02.136 in Ubuntu, I still have the same errors. |
@fuentess Have you tried this with the latest agent/image. I reran the crio tests locally today on Ubuntu 16.04 and saw that the tests are passing. Can you retest? |
This sounds like the same bug as clearcontainers/runtime#910 Once the virtio-scsi lands, perhaps we can test here. Seems virtio-blk support is buggy when moving to later QEMU versions,. |
related PR for scsi: containers/virtcontainers#584 |
@sboeuf ^^ see issue above for logs regarding the mount failures observed; likely the same which is observed during our crio tests with qemu-cc. |
--datadir=/usr/share/qemu-cc \ | ||
--libdir=/usr/lib64/qemu-cc \ | ||
--libexecdir=/usr/libexec/qemu-cc \ | ||
--disable-tools \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these options should really be coming from https://github.com/clearcontainers/packaging/blob/master/scripts/configure-hypervisor.sh.
@chavafg could you please give this a new try after clearcontainers/runtime#1037 has been merged. My understanding is that we got blocked all this time because we were using |
@chavafg clearcontainers/runtime#1037 has been merged 😄, it's time to check if switching to Qemu 2.9 can be done ! |
2.10 or greater or bust! |
@egernst of course ! 2.10 or greater 😄 |
verifying today... will post back when having the results. |
Thank you @chavafg |
So I have tested using qemu 2.10 and But I also tried using Azure VMs and I am not able to run containers. Any Idea of what might be wrong in the azure machines? |
@chavafg does Qemu 2.10 need specific libraries or a specific kernel config ? Something seems to be missing on the host environment. |
@sboeuf not sure, because I could create containers using qemu 2.10 and |
@chavafg can you go on Azure and verify that you can manually start a VM with Qemu 2.10 ? We need to identify if the issue comes from some misconfiguration of the environment, or if it comes from the way Qemu 2.10 is used by Clear Containers. |
Tracking these patch |
@mcastelino ok but this is a patch for the host kernel, which means this is something we don't really have control on. What is the workaround that we should use in the meantime ? |
Fix qemu build in distros with new glibc versions.
This commit adds the OBS files to build qemu with Q35 type support.
Fixes #25