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

qemu-virtiofs: Update to qemu 5.0 + virtiofs + dax #1097

Merged
merged 2 commits into from
Aug 14, 2020

Conversation

jcvenegas
Copy link
Member

@jcvenegas jcvenegas commented Jul 20, 2020

Update build scripts for qemu-virtiofs

  • virtiofs-0.3 patches are not needed
  • Sync build on how vanilla qemu is built
  • Apply patches for virtiofsd if any (none today)
  • Apply patches that are used for the qemu vanilla
  • Apply patches in order

Depends-on: github.com/kata-containers/runtime#2840
Depends-on: github.com/kata-containers/tests#2737

Fixes: github.com/kata-containers/runtime#2848

Signed-off-by: Jose Carlos Venegas Munoz [email protected]

@jcvenegas jcvenegas added the do-not-merge PR has problems or depends on another label Jul 20, 2020
jcvenegas added a commit to jcvenegas/runtime that referenced this pull request Jul 20, 2020
Update to qemu 5.0.x with support for virtiofs + dax.

Depends-on: github.com/kata-containers/packaging#1097

Fixes: #1

Signed-off-by: Jose Carlos Venegas Munoz <[email protected]>
ADD qemu/patches/ /root/kata_qemu_patches
# Apply virtiofs specific patches
RUN \
for patch in $(find /root/kata_qemu_patches/virtiofsd/${QEMU_VIRTIOFS_TAG}/ -name '*.patch'); do\
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look reliable - I think we need to sort the patches before applying as on #1005.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with @jodh-intel. We need a more robust approach here.
@jcvenegas Can you explain how you are applying the patches and whats changed.

Copy link
Member

Choose a reason for hiding this comment

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

Also -- I'm not sure if we need to carry any of the virtiofs patches, at least for 5.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jodh-intel done now is using sort to apply as you suggested
@amshinde @egernst the patches are applied in the following order

  1. First look and apply patches specific to the tag for virtiofsd (this is in case we need to fix something before a tag is generated by virtios folks) : Today there is not additional patches
  2. Look for patches specific to the qemu branch is based and apply them

@amshinde
Copy link
Member

amshinde commented Jul 22, 2020

@jcvenegas Since we are moving to 5.0 for the virtiofs kernel, we will also need to disable vmx-rdseed-exit for CI.
kata-containers/tests@a597ab8

Instead of checking for the version, you can add that flag for CI running on x86 architecture.

cc @devimc

jcvenegas added a commit to jcvenegas/runtime that referenced this pull request Jul 23, 2020
Update to qemu 5.0.x with support for virtiofs + dax.

Depends-on: github.com/kata-containers/packaging#1097
Depends-on: github.com/kata-containers/tests#2737

Fixes: kata-containers#2848

Signed-off-by: Jose Carlos Venegas Munoz <[email protected]>
jcvenegas added a commit to jcvenegas/kata-test that referenced this pull request Jul 23, 2020
experimental QEMU will be updated to qemu 5.0, now CI
fails with:

```
failed to set MSR 0x48b to 0x1582e00000000
qemu-virtiofs-system-x86_64:
/root/qemu-virtiofs/target/i386/kvm.c:2695: kvm_buf_set_msrs:
Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.: unknown.
```

Depends-on: github.com/kata-containers/packaging#1097
Depends-on: github.com/kata-containers/runtime#2840

Fixes: kata-containers#2736

Signed-off-by: Jose Carlos Venegas Munoz <[email protected]>
@jcvenegas jcvenegas force-pushed the qemu-virtiofs-5.0-dax branch from 3054d4a to 55d282b Compare July 23, 2020 14:44
@jcvenegas
Copy link
Member Author

/test

jcvenegas added a commit to jcvenegas/kata-test that referenced this pull request Aug 5, 2020
Experimental QEMU will be updated to qemu 5.0,
not needed to check for qemu version and disable in
all the time.

Depends-on: github.com/kata-containers/packaging#1097
Depends-on: github.com/kata-containers/runtime#2840

Fixes: kata-containers#2736

Signed-off-by: Jose Carlos Venegas Munoz <[email protected]>
@jcvenegas jcvenegas force-pushed the qemu-virtiofs-5.0-dax branch from fb5a0d4 to b9f4af0 Compare August 7, 2020 06:18
git apply "$patch"; \
done

RUN PREFIX="${PREFIX}" /root/configure-hypervisor.sh -s kata-qemu | sed -e 's|--disable-seccomp||g' | xargs ./configure \
Copy link
Member

Choose a reason for hiding this comment

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

I think we should just drop this flag from configure-hypervisor itself. It makes sense to have seccomp at this point.

Copy link
Member

Choose a reason for hiding this comment

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

@devimc makes use of Kconfig when building the snap. I opened on issue on this, [1], but we should really make sure we are absolutely consistent between specs/debs, snap and static builds.

[1] - #1105

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks I think that should be fixed in a separete PR for all places we use it.

RUN \
cat VERSION; \
stable_branch=$(cat VERSION | awk 'BEGIN{FS=OFS="."}{print $1 "." $2 ".x"}');\
for patch in $(find /root/kata_qemu_patches/${stable_branch}/ -name '*.patch'); do\
Copy link
Member

Choose a reason for hiding this comment

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

As soon as you start doing a for loop in a Dockerfile, you know you've gone too far man!

I'll see if I can provide a better suggestion in a bit, but this shouldn't live in the Dockerfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

:) yeah does not look nice, I have tried to change to way to do it, please take a look or suggest any other approach

jcvenegas added a commit to jcvenegas/runtime that referenced this pull request Aug 11, 2020
Update to qemu 5.0.x with support for virtiofs + dax.

Depends-on: github.com/kata-containers/packaging#1097
Depends-on: github.com/kata-containers/tests#2737

Fixes: kata-containers#2848

Signed-off-by: Jose Carlos Venegas Munoz <[email protected]>
@jcvenegas jcvenegas force-pushed the qemu-virtiofs-5.0-dax branch 2 times, most recently from b9d6ca8 to 8b22ec1 Compare August 12, 2020 16:39
Update build scripts for qemu-virtiofs.

- virtiofs-0.3 patches are not needed
- Sync build on how vanilla qemu is built
- Apply patches for virtiofsd if any (none today)
- Apply patches that are used for the qemu vanilla
- Apply patches in order

Depends-on: github.com/kata-containers/runtime#2840
Depends-on: github.com/kata-containers/tests#2737

Fixes: github.com/kata-containers/runtime#2848

Signed-off-by: Jose Carlos Venegas Munoz <[email protected]>
@jcvenegas jcvenegas force-pushed the qemu-virtiofs-5.0-dax branch from 8b22ec1 to e1e8577 Compare August 12, 2020 16:42
@jcvenegas
Copy link
Member Author

/test

Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @jcvenegas.

sudo docker build -t "${docker_image}" "${script_dir}"
DOCKER_CLI="docker"

if ! command -v docker && command -v podman; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You might want to add &>/dev/null to both the command calls (here and below) to avoid the paths appearing in build logs?

DOCKER_CLI="docker"

if ! command -v docker && command -v podman; then
DOCKER_CLI="podman"
Copy link
Member

Choose a reason for hiding this comment

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

Why podman..for Fedora?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes :)

Build clh with Podman, allow build the vmm in the Podman CI

Virtiofs qemu has to be build as this is requried by clh.

Signed-off-by: Jose Carlos Venegas Munoz <[email protected]>
@jcvenegas jcvenegas force-pushed the qemu-virtiofs-5.0-dax branch from e1e8577 to 4e1b572 Compare August 12, 2020 23:55
@jcvenegas
Copy link
Member Author

/test

@jcvenegas
Copy link
Member Author

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@likebreath likebreath left a comment

Choose a reason for hiding this comment

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

LGTM. This PR is required as the dependent PR of kata-containers/runtime#2840.

@jcvenegas jcvenegas changed the title dnm: qemu-virtiofs: Update to qemu 5.0 + virtiofs + dax qemu-virtiofs: Update to qemu 5.0 + virtiofs + dax Aug 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants