Skip to content

Added CLI and fixed a bug in make-microshift-app-images-rpm#642

Closed
nerdalert wants to merge 1 commit intoopenshift:mainfrom
nerdalert:brent-app-image-rpm
Closed

Added CLI and fixed a bug in make-microshift-app-images-rpm#642
nerdalert wants to merge 1 commit intoopenshift:mainfrom
nerdalert:brent-app-image-rpm

Conversation

@nerdalert
Copy link
Member

  • added a CLI to the script
  • fixed a bug where multiple copies of the image would get created
    due to storage.conf getting a storage path appended every time
    the app is run.
  • check for dependecies prior to attempting to build an image

Signed-off-by: Brent Salisbury bsalisbu@redhat.com

@openshift-ci openshift-ci bot requested review from oglok and rootfs March 30, 2022 07:35
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 30, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign copejon after the PR has been reviewed.
You can assign the PR to them by writing /assign @copejon in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

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

@nerdalert nerdalert force-pushed the brent-app-image-rpm branch from 5b64815 to 53e5ef3 Compare March 30, 2022 07:40
@nerdalert
Copy link
Member Author

Caught a bug while adding a CLI to this. Noticed copies of the image were getting incremented every time the app was run. Turned out to be lines getting appended every run under additionalimagestores in /etc/containers/storage.conf. Just added a check if it's already there to not append another line.

additionalimagestores = [
"/var/lib/containers/storage/overlay-images",
"/var/lib/containers/storage/overlay-images",
"/var/lib/containers/storage/overlay-images",
"/var/lib/containers/storage/overlay-images",
"/var/lib/containers/storage/overlay-images",
]

Ty!!

Copy link
Contributor

@copejon copejon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the UX enhancement! Some of my comments don't pertain directly to code changes, but as it's a UX pr, I feel they are relevant.

Comment on lines 116 to 120
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an FYI, this block fails consistently, both on main branch and here, with:

+ '[' -d /tmp/ushift-rpm-package//BUILDROOT/microshift-app-images-1-1.x86_64/home/jcope/.local/share/containers/storage/overlay-images ']'
+ sudo rm -rf /tmp/ushift-rpm-package//BUILDROOT/microshift-app-images-1-1.x86_64/home/jcope/.local/share/containers/storage/overlay-images
rm: cannot remove '/tmp/ushift-rpm-package//BUILDROOT/microshift-app-images-1-1.x86_64/home/jcope/.local/share/containers/storage/overlay-images/overlay': Device or resource busy
error: Bad exit status from /var/tmp/rpm-tmp.Y9aEP7 (%prep)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.Y9aEP7 (%prep)

I'll open an issue for this. cc @mangelajo

Copy link
Member Author

@nerdalert nerdalert Apr 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @copejon!

I dug around on this. Reason I missed it is because it doesn't show up on Fedora35 but does on RHEL8.5. I'm guessing its the rpmbuild versions (Fedora RPM version 4.17.0) vs. (RHEL RPM version 4.14.3) but I'm having to knock off some rust on RPM building so that's a guess.

  • Here is the buildroot exit:
Binary file /home/brent/rpmbuild/BUILDROOT/microshift-app-images-1-1.x86_64/var/lib/containers/storage/overlay-images/libpod/bolt_state.db matches
Found '/home/brent/rpmbuild/BUILDROOT/microshift-app-images-1-1.x86_64' in installed files; aborting
error: Bad exit status from /var/tmp/rpm-tmp.S0yRBl (%install)

In the meantime, an option is to skip the postinstall buildroot check with %define __arch_install_post %{nil} to get things working in master until we figure out the buildroot issue or if it isn't urgent maybe someone else might have an idea. I'm also chasing down an issue with building some different images that aren't resolving dependancies that I haven't nailed down yet.

Here is output from the build and install if you want to diff it with your output.
Ty!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an FYI, this block fails consistently, both on main branch and here, with:

+ '[' -d /tmp/ushift-rpm-package//BUILDROOT/microshift-app-images-1-1.x86_64/home/jcope/.local/share/containers/storage/overlay-images ']'
+ sudo rm -rf /tmp/ushift-rpm-package//BUILDROOT/microshift-app-images-1-1.x86_64/home/jcope/.local/share/containers/storage/overlay-images
rm: cannot remove '/tmp/ushift-rpm-package//BUILDROOT/microshift-app-images-1-1.x86_64/home/jcope/.local/share/containers/storage/overlay-images/overlay': Device or resource busy
error: Bad exit status from /var/tmp/rpm-tmp.Y9aEP7 (%prep)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.Y9aEP7 (%prep)

I'll open an issue for this. cc @mangelajo

If you run as sudo I think that error should get resolved.

@nerdalert nerdalert force-pushed the brent-app-image-rpm branch 2 times, most recently from 5e23d86 to 532ff00 Compare April 1, 2022 07:41
Copy link
Member Author

@nerdalert nerdalert Apr 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This skips post install checks that is currently breaking the rpm build on RHEL 8.5

Copy link
Contributor

@mangelajo mangelajo Apr 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other spec has a more elaborated version, it has a couple of details we could include here:

%post


# only on install (1), not on upgrades (2)
if [ $1 -eq 1 ]; then
   sed -i '/^additionalimagestores =*/a "%{imageStore}",' /etc/containers/storage.conf
   # if crio was already started, restart it so it read from new imagestore
   systemctl is-active --quiet crio && systemctl restart --quiet crio
fi

%postun

# only on uninstall (0), not on upgrades(1)
if [ $1 -eq 0 ];
  sed -i '/"${imageStoreSed}",/d" /etc/containers/storage.conf
  systemctl is-active --quiet crio && systemctl restart --quiet crio

fi

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the removal sed part is broken on the example I posted.

Copy link
Member Author

@nerdalert nerdalert Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mangelajo got the delete sed sorted and added a noop on the crio check in case it was inactive to avoid scriplet failure errors. /etc/containers/storage inserts the filepath on install [1] and deletes it on [0] for uninstall.

Below is the output of the build/install/delete. Ty!

sudo ./make-rpm.sh   --image-list ./images.txt   --container-dir /var/lib/containers/storage/overlay-images   --rpmbuild-dir /home/brent/rpmbuild
[truncated]
Wrote: /home/brent/rpmbuild/RPMS/x86_64/microshift-app-images-1-1.x86_64.rpm
Executing(%clean): /bin/sh -e /var/tmp/rpm-tmp.TsDj1b
+ umask 022
+ cd /home/brent/rpmbuild/BUILD
+ /usr/bin/rm -rf /home/brent/rpmbuild/BUILDROOT/microshift-app-images-1-1.x86_64
+ exit 0
 
brent@rhel8-80:~/rpm-builds$ sudo rpm -ivh /home/brent/rpmbuild/RPMS/x86_64/microshift-app-images-1-1.x86_64.rpm
Verifying...                          ################################# [100%]
Preparing...                          ################################# [100%]
Updating / installing...
   1:microshift-app-images-1-1        ################################# [100%]

brent@rhel8-80:~/rpm-builds$ sudo podman image ls
REPOSITORY                    TAG         IMAGE ID      CREATED     SIZE        R/O
mirror.gcr.io/library/alpine  latest      0ac33e5f5afa  8 days ago  5.86 MB     true

brent@rhel8-80:~/rpm-builds$ sudo rpm -ev microshift-app-images
Preparing packages...
microshift-app-images-1-1.x86_64

brent@rhel8-80:~/rpm-builds$ sudo podman image ls
REPOSITORY  TAG         IMAGE ID    CREATED     SIZE

@copejon
Copy link
Contributor

copejon commented Apr 1, 2022

/retest

- added a CLI to the script
- fixed a bug where multiple copies of the image would get created
  due to storage.conf getting a storage path appended every time
  the app is run.
- check for dependecies prior to attempting to build an image

Signed-off-by: Brent Salisbury <bsalisbu@redhat.com>
@nerdalert nerdalert force-pushed the brent-app-image-rpm branch from 532ff00 to 01b83b1 Compare April 13, 2022 07:52
@mangelajo
Copy link
Contributor

Hey brent, sorry for stepping feet here, I have been working on #645 to fix some of the issues here, and also some additional ones I found (like broken permissions everywhere because of how rpm packages)

I added you on that one as co-author, first to reflect what you were also working on improving this, and also because I took some bits from here (like the sed with different delimiter which I didn't know that could be done).

I hope that other one works for you and if it doesn't let me know, and let's improve it.

@mangelajo mangelajo closed this Apr 13, 2022
@nerdalert
Copy link
Member Author

@mangelajo awesome! Will give 645 a spin.

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.

3 participants