Add rpm-ostree tests by mike-nguyen · Pull Request #164 · projectatomic/atomic-host-tests

mike-nguyen · 2017-05-31T22:00:51Z

Add a basic test suite for rpm-ostree. There are still a few subcommands
that still need to be added to the test suite. These tests assume
that the atomic host is provisioned with the latest version as it will
move between HEAD and HEAD-1.

Tests:

Use rpm-ostree to deploy by version then rollback and delete rollback
Use rpm-ostree to deploy by checksum then rollback and delete rollback
Use rpm-ostree to deploy HEAD-1 then use cleanup pending deployments
Simulate rpm-ostree upgrade then rebase to HEAD and delete rollback
Simulate rpm-ostree upgrade with package install, then rebase to HEAD
with an uninstall of the same package, then delete rollback
Enable and validate client side initramfs generation then disable
initramfs and cleanup deployments

Add a basic test suite for rpm-ostree. There are still a few subcommands that still need to be added to the test suite. These tests assume that the atomic host is provisioned with the latest version as it will move between HEAD and HEAD-1. Tests: - Use rpm-ostree to deploy by version then rollback and delete rollback - Use rpm-ostree to deploy by checksum then rollback and delete rollback - Use rpm-ostree to deploy HEAD-1 then use cleanup pending deployments - Simulate rpm-ostree upgrade then rebase to HEAD and delete rollback - Simulate rpm-ostree upgrade with package install, then rebase to HEAD with an uninstall of the same package, then delete rollback - Enable and validate client side initramfs generation then disable initramfs and cleanup deployments

mike-nguyen · 2017-06-01T01:01:00Z

I tested on CAHC, RHELAH 7.3.5, and Fedora 26. The rpm-ostree version installed must support the --install and --uninstall flags so CentOS AH may not have it yet.

jlebon

Looks really nice overall!

There's definitely some overlap with the rpm-ostree vmcheck testsuite, which reminds me of the thoughts in #74. Though this is clearly a net add in absence of that. (Maybe this can be a faster rpm-ostree sanity check and vmcheck can be the more comprehensive, lower down the pipe test that takes much longer).

jlebon · 2017-06-01T13:35:07Z

+
+Covered In Another Test
+  - rpm-ostree install
+  - rpm-ostree uninstall


Wait, so should install and uninstall be removed from the list just before then?

The package layering feature has a separate test suite already in atomic-host-tests. The install/uninstall below is the package layering as options to deploy, upgrade, and rebase

Just read this again. I will remove it form the readme.

jlebon · 2017-06-01T13:38:26Z

+      delay: 5
+
+    - name: Get HEAD-1 version
+      shell: ostree log {{ refspec.stdout }} | grep Version | tail -1 | cut -d' ' -f2


You can get it straight from ostree without parsing!

ostree show $revision --print-metadata-key version

jlebon · 2017-06-01T13:42:13Z

+         head_version: "{{ ross_json['deployments'][0]['version'] if ross_json['deployments'][0]['booted'] else ross_json['deployments'][1]['version'] }}"
+
+    - name: Pull last two commits
+      command: ostree pull --subpath /usr/share/rpm --depth=1 {{ refspec.stdout }}


Any reason for puling /usr/share/rpm? Can't find where we're querying the rpmdb lower down. Seems like we can just use --commit-metadata-only?

We switched to this for CAHC and FAHC. I was away when this change happened so I don't know the specifics. It has something to do with commit history on the continuous streams. @miabbott do you know offhand why we moved to this instead of commit-metadata-only?

Ah, we started doing this in our deployment playbooks/ jobs:

commit 7857fe3 Author: Micah Abbott <miabbott@redhat.com> Date: Wed Oct 12 11:01:25 2016 -0400 ans_atomic_info: fetch commit and file/RPM data This changes the playbook to pull the file/RPM data for use in determining the the package differences between HEAD^ and HEAD of the stream being tested. We had avoided doing this in the past because an `ostree pull --commit-metatdata-only` of the CAHC stream would fetch the deltas being generated between the two commits. Thus, the `rpm-ostree db diff` would return the desired results. ...

I think in this instance we are safe just pulling the commit metadata

Great! I made the changes to commit metadata

jlebon · 2017-06-01T13:43:57Z

+
+  tasks:
+    - name: Get current refspec
+      shell: cat $(ostree admin --print-current-dir).origin | grep refspec | cut -d= -f2


You can get that from the json too, though right? It's the origin key.

jlebon · 2017-06-01T13:50:50Z

+      command: ostree refs --create local-branch {{ head_csum }}
+
+    - name: Update origin file
+      command:  sed -i 's/^\(.*refspec\)=.*$/\1=local-branch/g' {{ current_dir.stdout }}.origin


Might be safer to do an rpm-ostree reload here after this just for good practice.

I'll add the rpm-ostree reload then verify the rpm-ostree status output which is a test for rpm-ostree reload.

Did you add the reload test, or do you mean to add it in a separate PR?

@jlebon I ran into an error running rpm-ostree reload yesterday. I was going to ask you about it today since you were in training but now I can't reproduce it. I've added the reload after modifying the origin file.

jlebon · 2017-06-01T13:57:49Z

+
+    - name: Convert to json
+      set_fact:
+         ross_json: "{{ ros_status.stdout | from_json }}"


Would be nice if we could factor these two tasks into e.g. an include. IIRC, we should still be able to access ross_json afterwards, I think?

Yes, it works. There was already the rpm_ostree_status role that converts to json then sets ross_json and also sets booted and unbooted to the proper deployment.

jlebon · 2017-06-01T14:04:02Z

+#
+#####################################################################################
+
+- name: rpm-ostree - upgrade and rebase + install and uninstall


Looks like this needs to be changed? :)

jlebon · 2017-06-01T14:09:40Z

+    - name: Fail if contents of initramfs is incorrect
+      fail:
+        msg: "Expected /etc/rpmostree-initramfs-testing-file in initramfs"
+      when: "'/etc/rpmostree-initramfs-testing-file' not in lsinitrd.stdout"


There's a subtle issue with this. The output of lsinitrd also prints out the arguments that were used to create the initramfs, which I suspect why this is matching. (The actual file listings don't have a leading / in their filenames).

Maybe a better check here is lsinitrd $initrd -f /my/file and checking that e.g. This is the contents of my file is in the output?

@jlebon when I run lsinitrd $initrd -f /my/file it produces no file. Am I running the command right?

[root@fedora ~]# ll total 16 -rw-------. 1 root root 5538 Apr 19 13:13 anaconda-ks.cfg -rw-------. 1 root root 5196 Apr 19 13:13 original-ks.cfg [root@fedora ~]# lsinitrd /boot/ostree/fedora-atomic-58da926b4642c6974a8b357b074f3ad3cf168163fb28c42cf3a010eda0ab4b84/initramfs-4.11.0-0.rc6.git0.1.fc26.x86_64.img -f ./myfile.txt [root@fedora ~]# ll total 16 -rw-------. 1 root root 5538 Apr 19 13:13 anaconda-ks.cfg -rw-------. 1 root root 5196 Apr 19 13:13 original-ks.cfg [root@fedora ~]# lsinitrd /boot/ostree/fedora-atomic-58da926b4642c6974a8b357b074f3ad3cf168163fb28c42cf3a010eda0ab4b84/initramfs-4.11.0-0.rc6.git0.1.fc26.x86_64.img Image: /boot/ostree/fedora-atomic-58da926b4642c6974a8b357b074f3ad3cf168163fb28c42cf3a010eda0ab4b84/initramfs-4.11.0-0.rc6.git0.1.fc26.x86_64.img: 49M ======================================================================== Early CPIO image ======================================================================== drwxr-xr-x 3 root root 0 Jan 1 1970 . -rw-r--r-- 1 root root 2 Jan 1 1970 early_cpio drwxr-xr-x 3 root root 0 Jan 1 1970 kernel drwxr-xr-x 3 root root 0 Jan 1 1970 kernel/x86 drwxr-xr-x 2 root root 0 Jan 1 1970 kernel/x86/microcode -rw-r--r-- 1 root root 24070 Jan 1 1970 kernel/x86/microcode/AuthenticAMD.bin ======================================================================== Version: dracut-044-178.fc26 Arguments: --reproducible --gzip -v --add 'ostree' --tmpdir '/tmp' -f --no-hostonly --add 'iscsi' --kver '4.11.0-0.rc6.git0.1.fc26.x86_64' --reproducible --gzip -v --add 'ostree' --tmpdir '/tmp' -f -I '/etc/rpmostree-initramfs-testing-file' --kver '4.11.0-0.rc6.git0.1.fc26.x86_64' ..snip...

Heh, sorry, I should have written it as -f $path_to_my_file. I.e. the same path you included earlier. E.g.:

# echo "barbaz" > /etc/foo.conf # rpm-ostree initramfs --enable --arg="-I" --arg="/etc/foo.conf" # lsinitrd /boot/ostree/fedora-atomic-73577e678e475e49881041c3b7c747c9665b95a645cc2aef85e6678c62a5ce35/initramfs-4.11.3-200.fc25.x86_64.img -f /etc/foo.conf barbaz

miabbott · 2017-06-02T21:42:01Z

+
+  tasks:
+    - name: Get current refspec
+      shell: cat $(ostree admin --print-current-dir).origin | grep refspec | cut -d= -f2


Can we get this from the JSON output of rpm-ostree status? It looks like it may be the origin value:

# rpm-ostree status --json | python -m json.tool { "deployments": [ { "booted": true, "checksum": "0ccf9138962e5c2c3794969a228e751d13bb780f5b0a1f15f4a9649df06ba80a", "gpg-enabled": false, "id": "rhel-atomic-host-0ccf9138962e5c2c3794969a228e751d13bb780f5b0a1f15f4a9649df06ba80a.0", "origin": "rhel-atomic-host:rhel-atomic-host/7/x86_64/standard", "osname": "rhel-atomic-host", "packages": [], "regenerate-initramfs": false, "requested-local-packages": [], "requested-packages": [], "serial": 0, "timestamp": 1495490444, "unlocked": "none", "version": "7.3.5" } ], "transaction": null }

@jlebon Can you confirm this?

Yup, that's correct! I made the same suggestion a bit lower down on another instance of this.

miabbott · 2017-06-02T21:48:54Z

+      delay: 5
+
+    - name: Get HEAD-1 checksum
+      shell: ostree log {{ refspec.stdout }} | grep ^commit | tail -1 | cut -d' ' -f2


You can get this with ostree rev-parse $refspec^. Like this:

# ostree rev-parse rhel-atomic-host/7/x86_64/standard^ d6c7a5639cdeb6c21cf40d80259d516d047176e35411c8684cae40a93eedbed0

miabbott · 2017-06-02T21:54:41Z

+#     remote_name (required) name for remote
+#     remote_url (required) remote url
+#     refspec (required) remote refspec
+


Do you want to add a fail statement here if the parameters aren't defined?

miabbott · 2017-06-02T21:56:40Z

This is a good set of tests. I'd echo what has been said already that if we could make a role that basically generates a variable containing the JSON of rpm-ostree status, it would cleanup the code duplication.

mike-nguyen · 2017-06-06T15:56:49Z

Pushed fixup with the changes suggested.

miabbott · 2017-06-06T20:21:26Z

+      shell: ostree show $(ostree rev-parse {{ refspec }}^) --print-metadata-key version | tr -d \'
+      register: hmo_version
+
+    - name: Deploy version {{ hmo_version.stdout }}


nit - I don't think variable substitution happens for the name: statements

Seems like it does although I have seen instances when it doesn't (possibly in loops?). Seems like it only substitutes for the first instance if running against two hosts so I'll just remove it and add a generic message.

TASK [Deploy version 7.2017.383] *********************************************** changed: [fedora] => { "attempts": 1, "changed": true, "cmd": [ "rpm-ostree", "deploy", "26.55" ], "delta": "0:00:32.515279", "end": "2017-06-06 14:46:03.710649", "rc": 0, "start": "2017-06-06 14:45:31.195370", "warnings": [] }

miabbott · 2017-06-06T20:22:57Z

+    - include: ../../common/ans_reboot.yml
+
+    # verify version in deployment 0
+    - include: roles/rpm_ostree_status_verify/tasks/main.yml


Is there a reason to mix include vs. include_role in this test?

I was kind of all over the place with the include vs include_role. I was trying to use include_role in as many places as possible because it takes out the relative path. The particular instances I did not use include_role was:

ans_reboot.yml isn't in a roles directory so it needs to be included

variables not being passed properly using include_role. I think cevich mentioned something along the lines of this.

I think it would be cleaner if you could stick to one, but I won't hold up merging

I think I will change it to all includes for consistency

miabbott · 2017-06-06T20:25:00Z

+    msg: "deployment {{ deployment }} does not exist"
+  when: ros_json['deployments'][deployment] is undefined
+
+- name:  Fail if deployment {{ deployment }} values are incorrect


I was a bit confused why you used a dict here, but seeing it used in practice in the main test file made me understand.

Great idea and very flexible!

My original implementation had dynamically named variables based on the rpm-ostree status json keys. The problem was that certain properties are displayed based on some rpm-ostree actions. If an action is undone, sometimes that property would disappear from the output but the variable would persist and cause some confusion validating property values.

miabbott · 2017-06-06T20:26:33Z

+  tasks:
+    - include: roles/rpm_ostree_status/tasks/main.yml
+
+    - name: Set current version and refspec


Might be helpful to stick a comment here explaining that ros_booted comes from the rpm_ostree_status role that was included above.

Added comment

miabbott · 2017-06-06T20:30:30Z

+    - name: Create local branch
+      command: ostree refs --create local-branch {{ head_csum }}
+
+    - name: Update origin file


Maybe drop a comment here explaining why you are modifying the origin file

Added comment.

miabbott · 2017-06-06T20:32:02Z

+
+    - name: Cleanup
+      command: rpm-ostree cleanup -rpmb
+


Should there be rpm_ostree_status_verify here as in previous sections?

miabbott · 2017-06-06T20:48:17Z

+
+    - name: Cleanup
+      command: rpm-ostree cleanup -rpmb
+


Another instance to verify that the cleanup was successful with rpm_ostree_status_verify?

jlebon · 2017-06-07T13:25:52Z

+      command: ostree refs --create local-branch {{ head_csum }}
+
+    - name: Update origin file
+      command:  sed -i 's/^\(.*refspec\)=.*$/\1=local-branch/g' {{ current_dir.stdout }}.origin


Did you add the reload test, or do you mean to add it in a separate PR?

jlebon · 2017-06-07T13:29:17Z

+      command: test -n {{ initrd.stdout }}
+
+    - name: Get contents of initrd
+      shell: lsinitrd {{ initrd.stdout }} > lsinitrd.txt


I'd still change this to use lsinitrd -f to make it an even stronger test.

I'm not sure if I'm doing something wrong but -f does not seem to do anything.

mike-nguyen · 2017-06-07T20:56:39Z

Much fixups. I believe I have all the requested changes in now this this last fixup!

jlebon · 2017-06-07T20:59:45Z

Looks good to me! 👍

miabbott · 2017-06-08T01:28:50Z

Let's do this!

jlebon reviewed Jun 1, 2017

View reviewed changes

mike-nguyen mentioned this pull request Jun 2, 2017

modify upgrade/rollback tests/roles to actually verify that they were successful #155

Open

miabbott reviewed Jun 2, 2017

View reviewed changes

fixup! Add rpm-ostree tests

36e7d15

fixup! Add rpm-ostree tests

1557608

miabbott reviewed Jun 6, 2017

View reviewed changes

jlebon reviewed Jun 7, 2017

View reviewed changes

mike-nguyen added 3 commits June 7, 2017 10:31

fixup! Add rpm-ostree tests

1e5e5fd

fixup! Add rpm-ostree tests

9ebcba5

fixup! Add rpm-ostree tests

dc8201b

miabbott merged commit 470968c into projectatomic:master Jun 8, 2017

mike-nguyen deleted the ros branch March 28, 2018 13:45

Conversation

mike-nguyen commented May 31, 2017

Uh oh!

mike-nguyen commented Jun 1, 2017

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miabbott commented Jun 2, 2017

Uh oh!

mike-nguyen commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-nguyen Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-nguyen Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-nguyen Jun 6, 2017 •

edited

Loading

mike-nguyen Jun 6, 2017 •

edited

Loading