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

switchroot: Fix regression for separately mounted /var #1668

Closed
wants to merge 3 commits into from

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Jul 3, 2018

I made a logical error in #1617 which resulted in the exact opposite
behaviour we want when /var is a separate mount.

Split this out and lower the number of negations to make it more obvious
that it's correct.

Closes: #1667

I made a logical error in ostreedev#1617 which resulted in the exact *opposite*
behaviour we want when `/var` is a separate mount.

Split this out and lower the number of negations to make it more obvious
that it's correct.

Closes: ostreedev#1667
@jlebon
Copy link
Member Author

jlebon commented Jul 3, 2018

Marking as WIP. I'll sanity check this tomorrow for both the separate /var point case and integrated case.

@cgwalters
Copy link
Member

Test case I was playing with, but I'm officially on vacation today:

diff --git a/tests/installed/destructive-ansible.yml b/tests/installed/destructive-ansible.yml
index c72f6b82..3c467f7b 100644
--- a/tests/installed/destructive-ansible.yml
+++ b/tests/installed/destructive-ansible.yml
@@ -12,4 +12,5 @@
     - import_tasks: tasks/install-git.yml
       when: use_git_build
     - import_tasks: tasks/query-host.yml
+    - import_tasks: destructive/var-mount.yml
     - import_tasks: destructive/staged-deploy.yml
diff --git a/tests/installed/destructive/var-mount.yml b/tests/installed/destructive/var-mount.yml
new file mode 100644
index 00000000..7bb0ae85
--- /dev/null
+++ b/tests/installed/destructive/var-mount.yml
@@ -0,0 +1,13 @@
+# https://github.com/ostreedev/ostree/issues/1667
+- name: Set up /var as a mountpoint
+  shell: |
+    set -xeuo pipefail
+    cp -a /var /sysroot/myvar
+    touch /sysroot/myvar/somenewfile
+    echo '/sysroot/myvar /var none bind 0 0' >> /etc/fstab
+- include_tasks: ../tasks/reboot.yml
+- name: Check that deploy-staged service worked
+  shell: |
+    set -xeuo pipefail
+    systemctl status var.mount
+    test -f /var/somenewfile

Mind trying this out and verifying it fails without your patch too?

@cgwalters
Copy link
Member

r=me if this works!

cgwalters and others added 2 commits July 4, 2018 11:57
One gotcha here is that we don't invalidate the RPMs if we're not
sitting on the same commit anymore. Shouldn't be too hard to fix, though
let's at least make a note of it for now.
@jlebon
Copy link
Member Author

jlebon commented Jul 4, 2018

Sigh, spent a while banging my head before I realized I was rsync'ing the wrong ostree bits. 🔀

I've verified that:

  • this works with no /var mountpoint
  • this works with a /var mountpoint
  • test case fails without this patch
  • test case succeeds with this patch

Thanks for the testcase! One tweak I had to make was:

diff --git a/tests/installed/tasks/install-git.yml b/tests/installed/tasks/install-git.yml
index 8216afeb..3374cce7 100644
--- a/tests/installed/tasks/install-git.yml
+++ b/tests/installed/tasks/install-git.yml
@@ -8,6 +8,9 @@
   synchronize: src=build/x86_64/ dest=/root/x86_64/ archive=yes
 - name: Install RPMs
   shell: rpm-ostree override replace /root/x86_64/*.rpm
+# Regenerate to make sure new ostree binaries also make it to the initrd
+- name: Regenerate initramfs
+  shell: rpm-ostree initramfs --enable
 - import_tasks: ../tasks/reboot.yml
 - import_tasks: ../tasks/query-host.yml
 - command: ostree --version

(I also tweaked the task name in the test).

@rh-atomic-bot r=cgwalters 4370722

@rh-atomic-bot
Copy link

⌛ Testing commit 4370722 with merge 2cb2571...

rh-atomic-bot pushed a commit that referenced this pull request Jul 4, 2018
rh-atomic-bot pushed a commit that referenced this pull request Jul 4, 2018
One gotcha here is that we don't invalidate the RPMs if we're not
sitting on the same commit anymore. Shouldn't be too hard to fix, though
let's at least make a note of it for now.

Closes: #1668
Approved by: cgwalters
@jlebon jlebon removed the WIP label Jul 4, 2018
@rh-atomic-bot
Copy link

💡 This pull request was already approved, no need to approve it again.

  • This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

@rh-atomic-bot
Copy link

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 2cb2571 to master...

@jlebon jlebon deleted the pr/fix-regression branch April 24, 2023 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants