Skip to content

Conversation

@cevich
Copy link
Member

@cevich cevich commented May 9, 2019

Since CI automation is now executing all tests as a regular user, there
is no need for root-based testing to run special rootless tests. Remove
them.

However, the root-based rootless tests did include one test for exercising
the '--rootfs' option which is needed. Add a new general, and more through
test to replace it - meaning it will be executed as root and non-root.

Signed-off-by: Chris Evich cevich@redhat.com

@cevich
Copy link
Member Author

cevich commented May 9, 2019

@mheon @giuseppe as we discussed 😄

Assuming this works, may I also strip out the python exec debugging-horror or do we still need that for some other purpose?

@TomSweeneyRedHat
Copy link
Member

Do we need to run any tests as root? Not my call, but I like the thought of running the tests as both root and rootless.
Beyond that, tests aren't hip atm.

@cevich
Copy link
Member Author

cevich commented May 9, 2019

@TomSweeneyRedHat An interesting thought, thx for sharing. I know the number of tests we need to run as root is larger than 1 😄 But I think you're correct that most can be run rootless, and better still, most can probably be run rootless and remotely. I'm sure there are at least a few medium-large testing efficiencies to be gained by consolidating. OTOH, it's likely costly to convert to that scheme in terms of engineer-time on the needed changes ☹️

@baude WDYT about ^^^ and relating to the remote/local testing split you're working on? Maybe Tom's idea could simplify that? i.e. skip (as-is) rootless testing and only do remote + rootless. Or does that miss too many tests?

@rhatdan
Copy link
Member

rhatdan commented May 10, 2019

I would like as many tests as possible run in all three modes.

Root, Rootless, Remote

@cevich cevich force-pushed the root_rootless_must_die branch from 1f03528 to acd5b68 Compare May 13, 2019 15:47
@cevich
Copy link
Member Author

cevich commented May 15, 2019

@rhatdan Backstory: The tests removed by this PR are duplicates of rootless tests we otherwise run as a regular user. The removed tests depend on some convoluted, complicated, hard to debug exec wizardry, to start off as root, but then execute podman as a regular user.

In other words, these removed tests predate the current CI setup, where we actually run all/most of the integration tests as a regular user on the system, no root involvement at all. The only exception is the case of --rootfs, which (I'm told) cannot be tested as a non-root user. So this PR adds a dedicated test to cover that.

@cevich cevich force-pushed the root_rootless_must_die branch from acd5b68 to 4cad3a6 Compare May 15, 2019 19:00
@rhatdan
Copy link
Member

rhatdan commented May 15, 2019

@cevich Ok understood.

@mheon
Copy link
Member

mheon commented May 15, 2019

/approve
@cevich Hit this with a rebase, and I'll merge away. I think this will get master back into a less buggy state.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, mheon

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2019
Since CI automation is now executing all tests as a regular user, there
is no need for root-based testing to run special rootless tests.  Remove
them.

However, the root-based rootless tests did include one test for exercising
the '--rootfs' option which is needed.  Add a new general, and more through
test to replace it - meaning it will be executed as root and non-root.

Signed-off-by: Chris Evich <cevich@redhat.com>
@cevich cevich force-pushed the root_rootless_must_die branch from 4cad3a6 to ae64e4e Compare May 16, 2019 13:44
@cevich
Copy link
Member Author

cevich commented May 16, 2019

@mheon okay, tests all green (woot!)

@rhatdan
Copy link
Member

rhatdan commented May 16, 2019

LGTM

@mheon
Copy link
Member

mheon commented May 16, 2019

I'm going to hold this until #3140 lands to keep things clean until the new version lands

@mheon
Copy link
Member

mheon commented May 17, 2019

Alright, it's landed, let's get this in
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2019
@openshift-merge-robot openshift-merge-robot merged commit ee1383a into containers:master May 17, 2019
edsantiago added a commit to edsantiago/libpod that referenced this pull request Feb 26, 2020
9f69c4e (part of the f31 pr, containers#3091) semi-broke the kill test,
there's now an ugly warning:

    setup(): removing stray images quay.io/libpod/fedora-minimal:latest 7bb5a60e8a78

The comments also didn't actually explain the problem
being addressed, and included a misleading reference
to busybox.

Here we switch to using fedora-minimal only with podman-remote,
clean it up (rmi) when finished, and include an explanation in
the comments about why this is needed; making it clear that
this workaround can be removed once we get rid of podman-remote.
We also reformat back to 80 columns.

Signed-off-by: Ed Santiago <santiago@redhat.com>
snj33v pushed a commit to snj33v/libpod that referenced this pull request May 31, 2020
9f69c4e (part of the f31 pr, containers#3091) semi-broke the kill test,
there's now an ugly warning:

    setup(): removing stray images quay.io/libpod/fedora-minimal:latest 7bb5a60e8a78

The comments also didn't actually explain the problem
being addressed, and included a misleading reference
to busybox.

Here we switch to using fedora-minimal only with podman-remote,
clean it up (rmi) when finished, and include an explanation in
the comments about why this is needed; making it clear that
this workaround can be removed once we get rid of podman-remote.
We also reformat back to 80 columns.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@cevich cevich deleted the root_rootless_must_die branch June 30, 2021 18:03
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants