Skip to content

kola: Support the Debian autopkgtest reboot API#1528

Merged
openshift-merge-robot merged 1 commit intocoreos:masterfrom
cgwalters:autopkgtest-reboot
Jun 17, 2020
Merged

kola: Support the Debian autopkgtest reboot API#1528
openshift-merge-robot merged 1 commit intocoreos:masterfrom
cgwalters:autopkgtest-reboot

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 cgwalters force-pushed the autopkgtest-reboot branch from d8a801d to dc3e2ca Compare June 12, 2020 17:23
@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!


// 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.
@cgwalters cgwalters force-pushed the autopkgtest-reboot branch from dc3e2ca to d571851 Compare June 17, 2020 13:26
@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

Details 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.

5 participants

Comments