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

vmcheck: Work around read-only /sysroot #2023

Merged

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Mar 18, 2020

We need to adapt some of our tests here which assume that /sysroot is
writable. However, in FCOS this is no longer the case now that we enable
sysroot.readonly.

We only remount rw for the couple of operations that need it so that we
still retain coverage for the ro path everywhere else.

@jlebon
Copy link
Member Author

jlebon commented Mar 18, 2020

There's definitely more bits that need to be adjusted. Will add on to this.

@jlebon jlebon force-pushed the pr/adapt-tests-to-sysroot-ro branch from 51fcdd2 to 6b617ed Compare March 18, 2020 19:59
@cgwalters
Copy link
Member

I'm totally fine with this, but a shortcut would mount it writable at the top of the code in all tests for now except the one or two that explicitly test it.

@jlebon jlebon force-pushed the pr/adapt-tests-to-sysroot-ro branch from 6b617ed to c9ebe98 Compare March 18, 2020 20:33
@jlebon
Copy link
Member Author

jlebon commented Mar 18, 2020

I'm totally fine with this, but a shortcut would mount it writable at the top of the code in all tests for now except the one or two that explicitly test it.

Yeah, I considered that, but I'm worried it'd make it too easy for us to regress on read-only handling in some random paths where we weren't even trying to test it. I think we're at least past the halfway mark now; let's see how it goes!

Classic shell gotcha. We don't want to run `vm_kola_spawn` as part of
the if-statement or otherwise we lose the `set -e` behaviour.
@jlebon jlebon force-pushed the pr/adapt-tests-to-sysroot-ro branch from bbd90bf to e38813c Compare March 19, 2020 13:27
@jlebon jlebon changed the title WIP: vmcheck: work around read-only /sysroot vmcheck: Work around read-only /sysroot Mar 19, 2020
@jlebon jlebon marked this pull request as ready for review March 19, 2020 13:27
@jlebon
Copy link
Member Author

jlebon commented Mar 19, 2020

OK, this is ready to go!

Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

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

👍

tests/common/libvm.sh Show resolved Hide resolved
tests/common/libvm.sh Show resolved Hide resolved
We need to adapt some of our tests here which assume that `/sysroot` is
writable. However, in FCOS this is no longer the case now that we enable
`sysroot.readonly`.

We only remount rw for the couple of operations that need it so that we
still retain coverage for the ro path everywhere else.
@jlebon jlebon force-pushed the pr/adapt-tests-to-sysroot-ro branch from e38813c to 55206db Compare March 19, 2020 13:51
@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [ashcrow,cgwalters,jlebon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit c6e5e80 into coreos:master Mar 19, 2020
@jlebon jlebon deleted the pr/adapt-tests-to-sysroot-ro branch April 23, 2023 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants