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

kola: Support the Debian autopkgtest reboot API #1528

Merged
merged 1 commit into from Jun 17, 2020

Conversation

cgwalters
Copy link
Member

As I was working on extending some of ostree's destructive
test suite to do reboots:
ostreedev/ostree#2127

I realized that the Debian autopkgtest API for rebooting is
better, because it allows saving state external to the host.

Rather than having the test count boots as ostree is doing
today, the "mark" allows us to more reliably dispatch.
And further, becase we don't rely on writing anything to disk
on the target, we can add clean support for "forced reboots"
that might kill the OS before we write to persistent storage there.

The "between reboot" state lives in the test runner's memory instead.

We retain support for the previous (two!) reboot APIs here for now.

I tested this with basically the example script
from the Debian autopkgtest specification:

set -xeuo pipefail
case "${AUTOPKGTEST_REBOOT_MARK:-}" in
  "") echo "test beginning"; /tmp/autopkgtest-reboot mark1 ;;
  mark1) echo "test in mark1"; /tmp/autopkgtest-reboot mark2 ;;
  mark2) echo "test in mark2" ;;
  *) echo "unexpected mark: ${AUTOPKGTEST_REBOOT_MARK}"; exit 1;;
esac
echo "ok autopkgtest rebooting"

I think it will make sense actually to implement more of the autopkgtest
API - Debian has a nontrivial number of tests using this, and I
think there's even work upstream in e.g. systemd to bridge its
tests to autopkgtest. Which would mean we gain "run systemd's tests in kola"
for free.

@cgwalters
Copy link
Member Author

OK now extended with some infrastructure to more e2e-validate the test infrastructure and also run that autopkgtest reboot example, until such time as we start using that API for other tests.

@cgwalters
Copy link
Member Author

Any high level feedback here?

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

High-level makes sense to me!

mantle/kola/harness.go Outdated Show resolved Hide resolved
@@ -43,6 +43,23 @@ const (
// kolaRebootStamp should be created by tests that want to reboot
const kolaRebootStamp = "/run/kola-reboot"

// These are defined by https://salsa.debian.org/ci-team/autopkgtest/raw/master/doc/README.package-tests.rst
const (
autopkgTestRebootPath = "/tmp/autopkgtest-reboot"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, why not e.g. /usr/local/bin? And that way we don't have to re-write it on each boot either.

Copy link
Member Author

Choose a reason for hiding this comment

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

That path is part of the autopkgtest specification.

#!/bin/bash
# Copy of the reboot example from https://salsa.debian.org/ci-team/autopkgtest/raw/master/doc/README.package-tests.rst
set -xeuo pipefail
case "${AUTOPKGTEST_REBOOT_MARK:-}" in
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshed: WDYT about just KOLA_REBOOT_MARK? It's good to know that the concept comes from Debian's autopkgtest, though it seems cleaner to have all the env vars exposed consistently start with KOLA_.

Copy link
Member Author

Choose a reason for hiding this comment

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

Similarly, the variable is part of the spec. I think using exactly the API from autopkgtest is valuable because it helps us more easily consume existing test suites.

Copy link
Member

Choose a reason for hiding this comment

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

Ack, sure WFM.

@arithx
Copy link
Contributor

arithx commented Jun 16, 2020

Overall concept WFM

Copy link
Contributor

@arithx arithx left a comment

Choose a reason for hiding this comment

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

LGTM, will let @jlebon do the /command so he can take a pass at responding to comments

As I was working on extending some of ostree's destructive
test suite to do reboots:
ostreedev/ostree#2127

I realized that the Debian autopkgtest API for rebooting is
better, because it allows *saving state external to the host*.

Rather than having the test count boots as ostree is doing
today, the "mark" allows us to more reliably dispatch.
And further, becase we don't rely on writing anything to disk
on the target, we can add clean support for "forced reboots"
that might kill the OS before we write to persistent storage there.

The "between reboot" state lives in the test runner's memory instead.

We retain support for the previous (two!) reboot APIs here for now.

I tested this with basically the example script
from the Debian autopkgtest specification:

```
set -xeuo pipefail
case "${AUTOPKGTEST_REBOOT_MARK:-}" in
  "") echo "test beginning"; /tmp/autopkgtest-reboot mark1 ;;
  mark1) echo "test in mark1"; /tmp/autopkgtest-reboot mark2 ;;
  mark2) echo "test in mark2" ;;
  *) echo "unexpected mark: ${AUTOPKGTEST_REBOOT_MARK}"; exit 1;;
esac
echo "ok autopkgtest rebooting"
```

I think it will make sense actually to implement more of the autopkgtest
API - Debian has a nontrivial number of tests using this, and I
think there's even work upstream in e.g. systemd to bridge its
tests to autopkgtest.  Which would mean we gain "run systemd's tests in kola"
for free.
@jlebon
Copy link
Member

jlebon commented Jun 17, 2020

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arithx, 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 [arithx,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 360e64e into coreos:master Jun 17, 2020
cgwalters added a commit to cgwalters/ostree that referenced this pull request Jun 19, 2020
See coreos/coreos-assembler#1528

I think we can drop the old cosa reboot APIs after this,
though I've already forgotten where else I might have written
tests using it.
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.

None yet

5 participants