USHIFT-1312 Test: Rebooting healthy system should result in backing up the data

[pmtk]

Tests functionality of #1874

[pmtk]

This test will probably require setting up current microshift-e2e to ignore ostree tags or provide mechanism to not run tests on non-ostree-based systems

[pmtk]

/cc @dhellmann @jogeo

[pmtk]

/cc @dhellmann @pacevedom @jogeo @dhensel-rh

This PR needs some sort mechanism to not run tests intended for R4E/ostree-based systems on RPM.
Approaches that come to mind:

• doing a runtime check and deciding if test should run or be skipped
• using tags to filter out irrelevant test suites
  any other ideas?

[dhellmann]

This looks OK, but I see the e2e test is failing with an error in this new test suite

Keyword 'Greenboot Health Check Exited' failed after retrying for 5 minutes. The last error was: dead != exited

[dhellmann]

This would be more robust if we looked for a different boot id after the reboot, to avoid any race condition with issuing the reboot command and sshd stopping. It doesn't seem like the most likely race, so let's make that change in another PR.

[dhellmann]

We know the exact path, right? Can we just remove it, instead of using find?

[pmtk]

With current flow (make new backup, delete old backup) there's always a chance that more than one backup will be present, this just makes sure that we're starting clean.
And we don't necessarily know the path, with randomized order of tests, in future, we could have a backup from two boots away, or more.
Also, health.json might already change (post-healthchecks) and point to current boot instead of previous

[dhellmann]

OK, we probably want to do something to limit the depth of the find call, then. Is it enough to say rm -rf ${deployID}* instead of using find at all?

And we probably want to check for deployID being empty, to ensure we don't delete everything.

[pmtk]

find was only looking into /var/lib/microshift-backups/ but I changed it to rm -rf because it's easier to read

[pmtk]

This file is in separate dir so e2e CI job doesn't try to run it against non-ostree system.
When we have e2e for ostree systems we can think if we want to move it back to suites/ and use tags (or other method to filter tests)

[pmtk]

/label tide/merge-method-squash

[pmtk]

/remove-label tide/merge-method-squash

Remembered that tide's squash results in ugly commit messages. I'll squash the commits myself

[dhellmann]

One small comment, but this generally looks good.

[dhellmann]

This looks good to me. I'll leave it for @jogeo to approve since he wanted a chance to review, too.

[jogeo]

Thank you for adding this test, it looks good over all. I've left a few questions and comments.

[jogeo]

Do we expect there to always be only one deployment, so the index of 0 will always be the deployment we should get the "id" from?

[pmtk]

This uses kw Get Booted Deployment Status which runs rpm-ostree status --booted --json
Thanks to --booted it will only output one deployment, but it's still using the same data structure, i.e. deployments in an array. With --booted it will only have 1 element.

Without that switch, 0 could be either booted or staged deployment

[jogeo]

'Evaluate json.loads' was used above in keyword "Get Booted Deployment Status". Should we be consistent in the pattern we use to ingest and parse json?

[jogeo]

As a side note, 'jq' is used by scripts such as microshift_set_healthy.sh, so it's not a bad thing that we are also using it in our test scripts. My initial comment was just a reaction to seeing json parsed in the same PR using two different approaches.

[pmtk]

That's a good point. jq here felt simpler, otherwise I'd have either cat and use json.loads or... I don't know what, I don't think SSHLibrary.Get File supports privilege escalation and health.json is only readable by root.
Also Get File scp's file, I'd rather use some kind of slurp which I haven't seen in that library.

Maybe we could create some helper keywords to slurping and modifying files as root and have flow consistent

[dhellmann]

I like the idea of keywords for grabbing files only readable by root. Save Default MicroShift Config uses cat to grab the contents of the config file, and it could use the same reusable keyword for that step. Then something like Upload String To File could be modified to take arguments for the owner and mode of the output file.

[jogeo]

Should we test path here with 'Should Not Be Empty'? I see that 'Get Booted Deployment Backup Prefix Path' has a check for empty values. However, if the code is changed to use a different key word at some point to get path we would not want to accidentally call 'rm -rf *'

[dhellmann]

For YAML, I found it handy to parse the text and then return a RF DottedDict type to make accessing the data from RF keywords easier. See the YAML library in the resources directory.

[ggiguash]

Let's merge this in the current form. The tests are not going to run because they are in a separate folder.
We now have a metal environment to run this tests, so let's try those out.
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ggiguash, pmtk

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 [ggiguash,pmtk]

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

[pmtk]

/retest

[openshift-ci]

@pmtk: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.