Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI] Add a VM-based Jenkins pipeline #1032

Merged
merged 1 commit into from
May 1, 2023
Merged

Conversation

dimakuv
Copy link
Contributor

@dimakuv dimakuv commented Nov 8, 2022

Description of the changes

New Jenkins pipeline runs a Docker container that creates a minimal VM based on https://github.com/gramineproject/device-testing-tools repo and runs a subset of Gramine tests, in particular, the device IOCTL tests. The pipeline uses Ubuntu 22.04 with modern Linux kernel (to have an upstream SGX driver and support for SGX in KVM) and QEMU/KVM to run the VM.

A LibOS regression test device_ioctl is added to test the CI; currently it is minimal but will be expanded when Flexible IOCTLs functionality is added to Gramine.

The corresonding PR in device-testing-tools repo is gramineproject/device-testing-tools#6 gramineproject/device-testing-tools#7 gramineproject/device-testing-tools#8.

How to test this PR?

New pipeline in Jenkins.


This change is Reviewable

@woju woju added the jenkins-debugging triggers additional pipelines on Jenkins label Nov 8, 2022
@woju
Copy link
Member

woju commented Nov 8, 2022

Jenkins, test linux-sgx-vm-gcc-release please

@woju
Copy link
Member

woju commented Nov 8, 2022

Jenkins, test linux-sgx-vm-gcc-release please

2 similar comments
@woju
Copy link
Member

woju commented Nov 8, 2022

Jenkins, test linux-sgx-vm-gcc-release please

@woju
Copy link
Member

woju commented Nov 8, 2022

Jenkins, test linux-sgx-vm-gcc-release please

@dimakuv dimakuv force-pushed the dimakuv/WIP-add-vm-jenkins branch 4 times, most recently from d6e23f7 to ec0301d Compare November 9, 2022 12:57
@dimakuv
Copy link
Contributor Author

dimakuv commented Nov 9, 2022

Jenkins, retest linux-sgx-vm-gcc-release please

1 similar comment
@dimakuv
Copy link
Contributor Author

dimakuv commented Nov 9, 2022

Jenkins, retest linux-sgx-vm-gcc-release please

@dimakuv dimakuv force-pushed the dimakuv/WIP-add-vm-jenkins branch 13 times, most recently from 45f4269 to 67a6015 Compare November 10, 2022 18:01
@dimakuv
Copy link
Contributor Author

dimakuv commented Nov 17, 2022

Jenkins, retest linux-sgx-vm-gcc-release please

2 similar comments
@dimakuv
Copy link
Contributor Author

dimakuv commented Nov 18, 2022

Jenkins, retest linux-sgx-vm-gcc-release please

@dimakuv
Copy link
Contributor Author

dimakuv commented Nov 18, 2022

Jenkins, retest linux-sgx-vm-gcc-release please

@dimakuv dimakuv force-pushed the dimakuv/WIP-add-vm-jenkins branch 4 times, most recently from 2265d20 to c712baa Compare January 20, 2023 12:48
Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 15 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)


.ci/ubuntu22.04.dockerfile line 98 at r19 (raw file):

    update-alternatives --set g++ /usr/bin/g++-10

# FIXME: below commands for the user `leeroy` are useless, remove them in all Dockerfiles

TODO(myself): Remove all the below during final rebase, after #1131 is merged


.ci/lib/stage-build-sgx-vm.jenkinsfile line 2 at r14 (raw file):
Yeah... I asked @woju if this is really needed, and if we can avoid this. I don't want to touch that generic code, since it is orthogonal and I don't know why it was needed like that in the first place.

UPDATE: reply from @woju:

we need this PATH assignment, because we meson install with non-standard prefix (we need to install inside $WORKSPACE)
and PATH in Jenkins is magic, i.e. you can't just sh '''PATH='''
even env.PATH= is broken
same for ENV PATH= in Dockerfile and any other combination really
yes, this is broken in a variety of ways, and the current situation is only slightly less broken than other options and we're quite fortunate we are still getting away with it

So I'm keeping my code as is.


.ci/lib/stage-build-sgx-vm.jenkinsfile line 18 at r14 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

Why it doesn't have it? How's that possible?

Ok, you're right, I had to do two things:

  • Install apt-get install linux-libc-dev -t bullseye-backports on the host (to get /usr/include/x86_64-linux-gnu/asm/sgx.h)
  • Setup the Docker volume redirection in the main Jenkinsfile: --volume=/usr/include/x86_64-linux-gnu/asm/sgx.h:/usr/include/asm/sgx.h:ro

.ci/lib/stage-build-sgx-vm.jenkinsfile line 52 at r14 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

See my reply above.

Done


libos/test/regression/device_ioctl.manifest.template line 13 at r14 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done, removed. This was set in other LibOS regression tests (and they also don't need it), so I wanted to be uniform. But yes, this is not needed.

FYI: I re-added sgx.nonpie_binary = true again. Apparently this is needed, because we build all tests by default with fno-PIE, see

c_args += '-fno-PIE'

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 15 files reviewed, 8 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)


scripts/gitignore-check-files line 8 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I should move this in a separate PR

Created a PR: #1137

That PR should be merged before this one (and then on rebase of this PR, this change will go away).

Copy link
Contributor

@boryspoplawski boryspoplawski left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r18, 2 of 3 files at r19, 2 of 2 files at r20, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @woju)


.ci/lib/stage-build-sgx-vm.jenkinsfile line 2 at r14 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yeah... I asked @woju if this is really needed, and if we can avoid this. I don't want to touch that generic code, since it is orthogonal and I don't know why it was needed like that in the first place.

UPDATE: reply from @woju:

we need this PATH assignment, because we meson install with non-standard prefix (we need to install inside $WORKSPACE)
and PATH in Jenkins is magic, i.e. you can't just sh '''PATH='''
even env.PATH= is broken
same for ENV PATH= in Dockerfile and any other combination really
yes, this is broken in a variety of ways, and the current situation is only slightly less broken than other options and we're quite fortunate we are still getting away with it

So I'm keeping my code as is.

This is wrong, but I don't have the resources to fight this.
Unblocking, cc @mkow

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow and @woju)


.ci/ubuntu22.04.dockerfile line 98 at r19 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

TODO(myself): Remove all the below during final rebase, after #1131 is merged

Just for info: #1131 was merged, I'll need to rebase and remove this fluff


scripts/gitignore-check-files line 8 at r8 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Created a PR: #1137

That PR should be merged before this one (and then on rebase of this PR, this change will go away).

#1137 was merged, so the rebase of this PR will incorporate those changes. Unblocking.

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 15 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @boryspoplawski, @mkow, and @woju)

a discussion (no related file):
Rebased to latest master.



.ci/ubuntu22.04.dockerfile line 21 at r9 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Will do during final rebase

Done.


.ci/ubuntu22.04.dockerfile line 98 at r19 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Just for info: #1131 was merged, I'll need to rebase and remove this fluff

Done

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 5 of 15 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)


.ci/lib/stage-build-sgx-vm.jenkinsfile line 7 at r23 (raw file):


        git clone https://github.com/gramineproject/device-testing-tools.git
        cd device-testing-tools

The PR gramineproject/device-testing-tools#8 was merged, so I added a fixup commit here that removes git checkout ..., because we can now use the default master branch.

@dimakuv dimakuv removed the jenkins-debugging triggers additional pipelines on Jenkins label Apr 25, 2023
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r4, 1 of 5 files at r12, 2 of 10 files at r13, 3 of 3 files at r14, 2 of 7 files at r15, 1 of 3 files at r18, 1 of 3 files at r19, 1 of 2 files at r20, 8 of 9 files at r22, 2 of 2 files at r23, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @woju)


.ci/linux-sgx-vm-gcc-release.jenkinsfile line 22 at r23 (raw file):

    env.DOCKER_ARGS_COMMON += ' --volume=/boot:/boot:ro'

    // only root and `kvm` group can access /dev/kvm, so add `kvm` GID to the in-Docker user

FYI: We need to be careful with this, it removes the isolation between the Docker interior and the host. It will be possible to accidentally break something on the Jenkins worker if we won't be careful.


.ci/lib/stage-build-sgx-vm.jenkinsfile line 2 at r14 (raw file):

Previously, boryspoplawski (Borys Popławski) wrote…

This is wrong, but I don't have the resources to fight this.
Unblocking, cc @mkow

Can't we pass that non-standard installation prefix using a different variable and just append to PATH here?


libos/test/regression/device_ioctl.c line 14 at r21 (raw file):

#include "gramine_test_dev_ioctl.h" /* currently unused */

#define STRING_READWRITE      "Hello world via read/write\n"

why this weird spacing?

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 15 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)


.ci/linux-sgx-vm-gcc-release.jenkinsfile line 22 at r23 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

FYI: We need to be careful with this, it removes the isolation between the Docker interior and the host. It will be possible to accidentally break something on the Jenkins worker if we won't be careful.

Yes, agreed.


.ci/lib/stage-build-sgx-vm.jenkinsfile line 2 at r14 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Can't we pass that non-standard installation prefix using a different variable and just append to PATH here?

Working on this in a separate PR, let's see if it will work out.


libos/test/regression/device_ioctl.c line 14 at r21 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

why this weird spacing?

Done.

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 15 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)


.ci/lib/stage-build-sgx-vm.jenkinsfile line 2 at r14 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Working on this in a separate PR, let's see if it will work out.

Trying here: #1312

Well, probably won't work because of https://issues.jenkins.io/browse/JENKINS-49076, but who knows.

Copy link
Contributor Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 14 of 15 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @mkow, and @woju)


.ci/lib/stage-build-sgx-vm.jenkinsfile line 2 at r14 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Trying here: #1312

Well, probably won't work because of https://issues.jenkins.io/browse/JENKINS-49076, but who knows.

I give up, please check #1312 for details. Jenkins is cancer, and I don't want to look at it anymore.

I will insist on keeping this hack; I spent enough time on circumventing this bug.

mkow
mkow previously approved these changes Apr 27, 2023
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r24, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)


.ci/lib/stage-build-sgx-vm.jenkinsfile line 2 at r14 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I give up, please check #1312 for details. Jenkins is cancer, and I don't want to look at it anymore.

I will insist on keeping this hack; I spent enough time on circumventing this bug.

Uh, Jenkins strikes again :/ Ok, resolving then.

New Jenkins pipeline runs a Docker container that creates a minimal VM
based on https://github.com/gramineproject/device-testing-tools repo and
runs a subset of Gramine tests, in particular, the device IOCTL tests.
The pipeline uses Ubuntu 22.04 with modern Linux kernel (to have an
upstream SGX driver and support for SGX in KVM) and QEMU/KVM to run the
VM.

A LibOS regression test `device_ioctl` is added to test the CI;
currently it is minimal but will be expanded when ioctl passthrough
functionality is added to Gramine.

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
@dimakuv dimakuv added the jenkins-debugging triggers additional pipelines on Jenkins label Apr 27, 2023
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r25, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @boryspoplawski)

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Dismissed @boryspoplawski from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


.ci/ubuntu22.04.dockerfile line 21 at r9 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Done.

Dismissing Borys, this issue was fixed and he unfortunately won't have time to re-review this PR.

@mkow mkow dismissed boryspoplawski’s stale review April 28, 2023 12:51

Borys won't have time to re-review this PR and was ok with dismissing him from such PRs.

@mkow mkow merged commit e5e06e8 into master May 1, 2023
@mkow mkow deleted the dimakuv/WIP-add-vm-jenkins branch May 1, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jenkins-debugging triggers additional pipelines on Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants