Test-1: Tests for PR # 175 by shishir-a412ed · Pull Request #177 · projectatomic/container-storage-setup

shishir-a412ed · 2017-01-04T17:19:55Z

Signed-off-by: Shishir Mahajan shishir.mahajan@redhat.com

rhvgoyal · 2017-01-06T19:42:00Z

@shishir-a412ed can we put some description in commit, explaining what this test is testing.

rhvgoyal · 2017-01-06T19:45:29Z

+      test_status=0
+   fi
+
+  $DSSBIN --reset >> $LOGS 2>&1


I think docker storage reset should be a separate test. First test should just test if DOCKER_ROOT_VOLUME works fine or not.

I think we already have test for devmapper and overlay cleanup. We probably should write one for docker-root-volume cleanup as well.

Yeah I saw those tests (devmapper and overlay cleanup) while developing this one.
After running this test, we would still need to unmount and remove DOCKER_ROOT_VOLUME.
I felt if I use docker-storage-setup --reset, it would test the reset functionality and do the cleanup at the same time. Otherwise it would be duplicating the effort.

rhvgoyal · 2017-01-06T19:46:19Z

+DEVS="$devs"
+VG=$vg_name
+DOCKER_ROOT_VOLUME=yes
+DOCKER_ROOT_VOLUME_SIZE=40%FREE


we should not have to specify size. Let it be default. Want this test to take default path where users will simply specify DOCKER_ROOT_VOLUME=yes.

Initially I did not specify DOCKER_ROOT_VOLUME_SIZE=40%FREE in the test. I was expecting docker-storage-setup to pick up the default value from /usr/lib/docker-storage-setup/docker-storage-seutp. However the CI started failing. I believe it was because CI version of that file did not have these new storage options DOCKER_ROOT_VOLUME and DOCKER_ROOT_VOLUME_SIZE.

rhvgoyal · 2017-01-06T19:49:33Z

+  # Make sure $DOCKER_ROOT_VOLUME {docker-root-lv} got created
+  # successfully.
+  if lv_exists "$docker_root_lv_name"; then
+    test_status=0


Don't set test_status=0 yet. As test will be successful only when you have done mountpoint check as well. You can instead check if lv exists or not and if not, then do the cleanup and return.

rhvgoyal · 2017-01-06T19:55:50Z

+  local lv lv_name="$1"
+
+  for lv in $(lvs --noheadings -o lv_name); do
+    if [ "$lv" == "$lv_name" ]; then


Do we really have to search for lv name in output. Can we just pass the expected lv to lvs and that should tell us if it exists or not?

lvs docker-vg/docker-root-lv ?

I used existing logic and structure of vg_exists() while developing lv_exists(). I wanted to keep lv_exists() as close as possible to existing code style. Does this work for you ? or should we change both (vg_exists, lv_exists) to incorporate this logic ?

shishir, now you have code in extra_volume_exists(), in docker-storage-setup.sh. It should be much simpler to use that.

shishir-a412ed · 2017-01-10T16:22:41Z

@rhvgoyal PTAL.

rhvgoyal · 2017-01-20T16:52:08Z

+   local mnt
+   mnt=$(findmnt -n -o TARGET --first-only --source /dev/${vg_name}/${docker_root_lv_name})
+   if [ "$mnt" == "/var/lib/docker" ];then
+      test_status=0


I think test_status should be set to 0 only once when we have done all the checks.

rhvgoyal · 2017-01-20T16:54:32Z

+  $DSSBIN --reset >> $LOGS 2>&1
+  # Test failed.
+  if [ $? -eq 0 ]; then
+     if [ ! -e /etc/sysconfig/docker-storage ]; then


I don't think we need to check for presence of this file. here. We should write a separate test which tests the reset functionality. And then we should make sure logical volume went away, var-lib-docker.mount file went away and
/etc/sysconfig/docker-storage went away.

rhvgoyal · 2017-01-20T16:58:43Z

+  # Test failed.
+  if [ $? -eq 0 ]; then
+     if [ ! -e /etc/sysconfig/docker-storage ]; then
+          test_status=0


Ok, how about following. Write two tests.

First tests to test volume creation funcitonality

second test to test reset funcitonality.

In first test you can still use --reset to cleanup but don't use its return code to determine test status. This will be something like cleanup $vg_name. It can fail too but we don't determine success/failure of tests based on cleanup.

So first test will ignore the --reset results. It will PASS/FAIL based on whether volume creation was successful or not.

Second test will fail if reset did not work properly.

Based on our discussion on irc, I am not using --reset for volume creation tests. I am writing a manual methods for doing cleanups.

shishir-a412ed · 2017-01-20T21:11:06Z

@rhvgoyal PTAL.

rhvgoyal · 2017-01-24T19:14:51Z

  wipe_signatures "$devs"
 }

+cleanup_container_root_volume() {


This function feels very specific. It probably is better in the test itself and not part of library.

Or, break this down into smaller pieces. That is have a helper function to cleanup mount unit file in libtest.sh and logic to remove lvm keep it in test itself.

shishir-a412ed · 2017-01-24T19:41:27Z

@rhvgoyal PTAL.

rhvgoyal · 2017-01-24T22:05:50Z

@shishir-a412ed On my local machine, once in a while test is failing. It says mkfs failed as device or resource is busy.

mkfs.xfs: cannot open /dev/dss-test-foo/docker-root-lv: Device or resource busy
ERROR: Failed to create filesystem on /dev/dss-test-foo/docker-root-lv

rhvgoyal · 2017-01-24T22:17:20Z

I introduced "udevadm settle" after lvcreate and before mkfs and that seems to have fixed the issue. So looks like udev is keeping device busy. I will open another PR to fix it.

rhvgoyal · 2017-01-24T22:20:38Z

+  lvchange -an $vg_name/${lv_name} >> $LOGS 2>&1
+  lvremove $vg_name/${lv_name} >> $LOGS 2>&1
+
+  cleanup_mount_file $mountpath


This should be $mount_path and not $mountpath. As a result, right now /etc/systemd/system/var-lib-docker.mount is not being cleaned up on the system after the tests.

rhvgoyal · 2017-01-24T22:41:39Z

I used udevadm settle and problem went away. Now I removed it and still I can't see the problem. I think this is a race which does not happen all the time. For now, fix the mount_path thing and I will merge it. If problem happens, will create separate PR to add udevadm settle.

This test tests docker-storage-setup options DOCKER_ROOT_VOLUME and DOCKER_ROOT_VOLUME_SIZE. This test will check if DOCKER_ROOT_VOLUME option is set to `yes`, docker-storage-setup would create a separate logical volume named `docker-root-lv` and mount it to default docker root location (/var/lib/docker). Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com>

shishir-a412ed · 2017-01-25T15:42:59Z

@rhvgoyal PTAL.

rhvgoyal · 2017-01-25T15:59:30Z

LGTM

rhvgoyal · 2017-01-25T15:59:39Z

@rh-atomic-bot r+

rh-atomic-bot · 2017-01-25T15:59:40Z

📌 Commit 4f2ce20 has been approved by rhvgoyal

rhvgoyal · 2017-01-25T16:01:06Z

@rh-atomic-bot r+ 4f2ce20

rh-atomic-bot · 2017-01-25T16:01:11Z

⌛ Testing commit 4f2ce20 with merge 184ea40...

rh-atomic-bot · 2017-01-25T16:02:32Z

☀️ Test successful - status-redhatci
Approved by: rhvgoyal
Pushing 184ea40 to master...

shishir-a412ed force-pushed the tests_docker_root_volume branch 2 times, most recently from f130c29 to 2b59f02 Compare January 4, 2017 18:29

shishir-a412ed mentioned this pull request Jan 4, 2017

Test-2: Tests for PR # 175 #178

Open

rhvgoyal reviewed Jan 6, 2017

View reviewed changes

shishir-a412ed force-pushed the tests_docker_root_volume branch 2 times, most recently from 486390f to 9df560b Compare January 9, 2017 15:37

rhvgoyal reviewed Jan 20, 2017

View reviewed changes

shishir-a412ed force-pushed the tests_docker_root_volume branch from 9df560b to 077640b Compare January 20, 2017 21:10

shishir-a412ed force-pushed the tests_docker_root_volume branch from 077640b to e5f3588 Compare January 24, 2017 16:41

rhvgoyal reviewed Jan 24, 2017

View reviewed changes

shishir-a412ed force-pushed the tests_docker_root_volume branch 2 times, most recently from ab842a2 to ecae40d Compare January 24, 2017 19:40

rhvgoyal reviewed Jan 24, 2017

View reviewed changes

shishir-a412ed force-pushed the tests_docker_root_volume branch from ecae40d to 4f2ce20 Compare January 25, 2017 15:41

rh-atomic-bot closed this in 184ea40 Jan 25, 2017

Conversation

shishir-a412ed commented Jan 4, 2017

Uh oh!

rhvgoyal commented Jan 6, 2017

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!

shishir-a412ed commented Jan 10, 2017

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!

shishir-a412ed commented Jan 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishir-a412ed commented Jan 24, 2017

Uh oh!

rhvgoyal commented Jan 24, 2017

Uh oh!

rhvgoyal commented Jan 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhvgoyal commented Jan 24, 2017

Uh oh!

shishir-a412ed commented Jan 25, 2017

Uh oh!

rhvgoyal commented Jan 25, 2017

Uh oh!

rhvgoyal commented Jan 25, 2017

Uh oh!

rh-atomic-bot commented Jan 25, 2017

Uh oh!

rhvgoyal commented Jan 25, 2017

Uh oh!

rh-atomic-bot commented Jan 25, 2017

Uh oh!

rh-atomic-bot commented Jan 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants