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: split storage ci jobs for root and non-root testing #276

Merged
merged 5 commits into from
Jul 15, 2022

Conversation

aesteve-rh
Copy link
Member

@aesteve-rh aesteve-rh commented Jul 5, 2022

Split storage CI jobs between unprivileged vdsm user tests (mark non-root) and root tests.

Comparison

With the numbers below we end up executing 50 more tests that were skipped before and safe more than 2 minutes in execution time.

Numbers are not averaged, only the last run is considered.

Before

All storage tests
collected 2702 items / 101 deselected / 2601 selected
Required test coverage of 68% reached. Total coverage: 68.87%
= 2472 passed, 123 skipped, 101 deselected, 5 xfailed, 1 xpassed, 1 warning in 371.31s (0:06:11) =

After

Non-root tests
collected 2700 items / 235 deselected / 2465 selected
Required test coverage of 61% reached. Total coverage: 61.87%
= 2402 passed, 59 skipped, 235 deselected, 4 xfailed, 1 warning in 164.30s (0:02:44) =

Root tests
collected 2700 items / 2566 deselected / 134 selected
Required test coverage of 47% reached. Total coverage: 47.76%
= 120 passed, 12 skipped, 2566 deselected, 1 xfailed, 1 xpassed, 1 warning in 211.60s (0:03:31) =

Total
= 2522 passed, 71 skipped, 5 xfailed, 1 xpassed, in max time 211.60s (0:03:31) =

Fixes: #128
Signed-off-by: Albert Esteve [email protected]

@aesteve-rh aesteve-rh self-assigned this Jul 5, 2022
@aesteve-rh aesteve-rh force-pushed the update-ci-non-root-testing branch from f1e9391 to 85cf193 Compare July 5, 2022 15:32
tests/storage/nbd_test.py Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@aesteve-rh aesteve-rh force-pushed the update-ci-non-root-testing branch from 85cf193 to a0cda98 Compare July 6, 2022 08:43
@aesteve-rh aesteve-rh requested a review from nirs July 6, 2022 08:51
@aesteve-rh aesteve-rh force-pushed the update-ci-non-root-testing branch 2 times, most recently from e43c03a to 4f6e74a Compare July 6, 2022 16:18
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
automation/tests-storage.sh Outdated Show resolved Hide resolved
automation/tests-storage.sh Outdated Show resolved Hide resolved
automation/tests-storage.sh Outdated Show resolved Hide resolved
automation/tests-storage.sh Show resolved Hide resolved
automation/tests-storage.sh Outdated Show resolved Hide resolved
automation/tests-storage.sh Outdated Show resolved Hide resolved
@aesteve-rh aesteve-rh force-pushed the update-ci-non-root-testing branch from 4f6e74a to 3ea9edf Compare July 7, 2022 07:18
@michalskrivanek
Copy link
Member

I still do not follow why do you need any differentiation for CI. Do we need to run twice, once as vdsm and second time as root?

For local you should use the same way as CI, I do not see any value in non-containerized runs. Anyone can run a container today

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@aesteve-rh aesteve-rh force-pushed the update-ci-non-root-testing branch 3 times, most recently from 6a47a06 to 4bee7a1 Compare July 8, 2022 08:46
@aesteve-rh aesteve-rh requested a review from nirs July 8, 2022 08:58
Mark all storage tests that require root with
the root label so that they get correctly picked
or excluded up by the CI jobs.

Signed-off-by: Albert Esteve <[email protected]>
@aesteve-rh aesteve-rh force-pushed the update-ci-non-root-testing branch from 4bee7a1 to 1b937a6 Compare July 14, 2022 07:11
Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

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

Nice!

tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
Makefile.am Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
automation/tests-storage.sh Outdated Show resolved Hide resolved
automation/tests-storage.sh Outdated Show resolved Hide resolved
Add tox environments for storage user and root
tests (i.e., storage-user and storage-root, respectively).
These environments are a subset of the storage
environment, that is split considering the root mark.

This way, we can easily run tests marked as
as a root user, and tests marked as non-root,
separately.

The new environments have their own different
target coverage. The 'storage-root' environment
sets a different coverage file and folder to avoid
overwriting other environment's coverage reports
with root ownership.

Signed-off-by: Albert Esteve <[email protected]>
@aesteve-rh aesteve-rh force-pushed the update-ci-non-root-testing branch 2 times, most recently from 82cef1a to f4ffdd1 Compare July 15, 2022 09:33
automation/tests-storage.sh Outdated Show resolved Hide resolved
Makefile.am Show resolved Hide resolved
Add targets for running tox in the storage-user
and storage-root environments.

Signed-off-by: Albert Esteve <[email protected]>
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Most of vdsm code run as vdsm:kvm, but vdsm tests run
as root by default in the CI, causing several problems, e.g.:
- Different behaviour, when run locally and the CI
- Some tests are skipped as root

We can separate the tests in two different jobs:
- storage-tests: run test marked as "not root".
  It still requires the container to be privileged
  for setting up the storage.
- storage-tests-root: run tests marked as "root".

Fixes: oVirt#128
Signed-off-by: Albert Esteve <[email protected]>
Remove tox storage environment as it is not
used in the CIs anymore. Consequently, remove
also the associated make target 'tests-storage'.

Signed-off-by: Albert Esteve <[email protected]>
@aesteve-rh aesteve-rh force-pushed the update-ci-non-root-testing branch from f4ffdd1 to 9656966 Compare July 15, 2022 12:44
@nirs nirs merged commit 395a3d6 into oVirt:master Jul 15, 2022
@aesteve-rh aesteve-rh added this to the ovirt-4.5.2 milestone Jul 15, 2022
@aesteve-rh aesteve-rh deleted the update-ci-non-root-testing branch September 12, 2022 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run the tests as unprivileged user
3 participants