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
Issue #148 Check openHAB version and perform an upgrade if different #149
Conversation
…e if the versions are different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You must run the update.sh
But that will download the newest OH, which is already in the image. But if we are fine with this redundant work that makes the changes much simpler but make the update depend upon
I assume you mean /openhab/runtime/bin/update. There is no update.sh file anywhere in the image. |
I mean this |
Ok, now I think I understand. I'll have an update shortly... |
Hmmm. I ran upgrade.sh and it is showing 42 changed files that I commited and 70 objects pushed but it only shows one commit and two files above. Did I mess something up along the way? |
No, I think, I is OK: |
Reviewed 42 of 42 files at r2. Comments from Reviewable |
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending. Comments from Reviewable |
Thanks for this PR @rkoshak! Will it also perform an upgrade when updating the container to a newer version of the same SNAPSHOT version? People may already be using the 2.3.0-SNAPSHOT container now and want to upgrade and test it again just before 2.3.0 stable gets released. @BClark09 how are such SNAPSHOT upgrades handled with the Debian/CentOS packages? |
@wborn, it is based on the build-no field in version.properties. Since that number gets changed for every successful snapshot build it should work just fine to go from one snapshot version to another. that is why I chose that field to test against instead of the others fields in version.properties. I'll note that theoretically, it should work for downgrading as well. It just checks to see if they are different, not whether the build number in the image is bigger than the one in your userdata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some changes were not properly merged since this PR changes several lines back to a previous version.
entrypoint_debian.sh
Outdated
echo "Please start the openHAB container with a pseudo-TTY using the -t option or 'tty: true' with docker compose" | ||
exit 1 | ||
fi | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove these lines that were added in #141?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't intentionally remove any lines. Not sure what caused that. I'll look into it. My changes should only include additions and all in the same place,
entrypoint_debian.sh
Outdated
@@ -52,14 +33,52 @@ case ${OPENHAB_VERSION} in | |||
cp -av "${APPDIR}/configurations.dist/." "${APPDIR}/configurations/" | |||
fi | |||
;; | |||
2.0.0|2.1.0|2.2.0|2.3.0-snapshot) | |||
2.0.0|2.1.0|2.2.0-snapshot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some changes were not properly merged since this changes the line back to a previous version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:-| I literally cloned the repo a few hours ago. This is weird.
entrypoint_debian.sh
Outdated
adduser openhab uucp2 &&\ | ||
adduser openhab dialout2 &&\ | ||
adduser openhab dialout3 &&\ | ||
adduser openhab uucp3 &&\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines were unchained in #142.
…and remade the edits. There were inconsistancies including changes from previous commits that were lost somehow.
I copied the two files from master on this repo rather than my fork and remade the additions and ran update.sh. I think I know what happened. It was a stupid mistake on my part. @wborn, can you confirm that the anomalies you identified are now fixed? |
Yes the anomalies are gone! :-) |
entrypoint_debian.sh
Outdated
@@ -46,20 +46,57 @@ fi | |||
# Copy initial files to host volume | |||
case ${OPENHAB_VERSION} in | |||
1.8.3) | |||
if [ -z "$(ls -A "${APPDIR}/configurations")" ]; then | |||
if [ -z a"$(ls -A "${APPDIR}/configurations")" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add the 'a' option here? It's not added for the alpine image and it doesn't seem to solve warnings when the dir does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure where those a's came from. I did not intentionally ad them. I only made the changes after the comment about the updating. I certainly did not make changes to the 1.8.3 section of the script.
entrypoint_debian.sh
Outdated
# Copy userdata dir for version 1.8.3 | ||
echo "No configuration found... initializing." | ||
cp -av "${APPDIR}/configurations.dist/." "${APPDIR}/configurations/" | ||
fi | ||
;; | ||
2.0.0|2.1.0|2.2.0|2.3.0-snapshot) | ||
# Initialize empty host volumes | ||
if [ -z "$(ls -A "${APPDIR}/userdata")" ]; then | ||
if [ -za"$(ls -A "${APPDIR}/userdata")" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add the 'a' option here? It's not added for the alpine image and it doesn't seem to solve warnings when the dir does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
entrypoint_debian.sh
Outdated
|
||
# Clear the cache and tmp | ||
rm -rf ${APPDIR}/userdata/cache/* | ||
rm -rf ${APPDIR}/userdata/tmp/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With these commands hidden files will not be cleared from the directories which may result in issues some day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openHAB now creates both directories. So you should be okay deleting these directories before openHAB is started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. I'll change it to remove both the folders. However, this would mean that earlier version of OH that don't create those folders will not work, right? What version introduced that behavior?
To hedge the bets I've changed it to remove the folders and then recreate them so the older releases that do not recreate these folders will still work.
entrypoint_debian.sh
Outdated
# Upgrade userdata if versions do not match | ||
curVersion=$(< ${APPDIR}/userdata/etc/version.properties grep build-no | cut -d : -f 2 | tr -d '[:space]') | ||
imgVersion=$(< ${APPDIR}/userdata.dist/etc/version.properties grep build-no | cut -d : -f 2 | tr -d '[:space]') | ||
echo "Current \"${curVersion}\" Image \"${imgVersion}\"" # TODO remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either remove the echo or the TODO. ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see forthcoming update
entrypoint_debian.sh
Outdated
cp ${APPDIR}/userdata.dist/etc/profile.cfg ${APPDIR}/userdata/etc/ | ||
cp ${APPDIR}/userdata.dist/etc/startup.properties ${APPDIR}/userdata/etc | ||
cp ${APPDIR}/userdata.dist/etc/org.apache.karaf* ${APPDIR}/userdata/etc/ | ||
cp ${APPDIR}/userdata.dist/etc/org.ops4j.pax.url.mvn.cfg ${APPDIR}/userdata/etc/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to always use double quotes with files and directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, see forthcoming update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rkoshak Thanks for picking this up!
1.8.3/amd64/alpine/entrypoint.sh
Outdated
cp /tmp/${backupFile} ${APPDIR}/userdata/backup/ | ||
echo "You can find backup of userdata in ${APPDIR}/userdata/backup/${backupFile}" | ||
|
||
# Copy over the updated files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to also be managing this list here? Would it be better to call the backup and update scripts instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update script will download the zip file each time. Would it be better to include the list of files in the distribution for a package manager (and the backup/restore scripts) to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that would be great :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't really but it was a bigger job across two repos to pull them out into a list that can be used in both places. This list doesn't change that frequently so I figured we would kick the can down the road just a bit until we can figure out how to make the list usable for both.
My initial thoughts were perhaps that part of the script could be pulled out into a separate script that both entrypoint and all the installation and upgrade scripts call. Then not only would the list be reusable here but the actual behavior. That would guarantee that Docker would keep up with changes to the upgrade process in the future. I don't know if the apt/yum installers can support that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it would be okay as a first step.
Btw: I think that @BClark09 did not mean it as an improvement on this PR but the openHAB-generic side of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that's right, I agree what you're doing is good for now but when I have the time (may be a few weeks), I'll add the proposed list to openhab-distro
, and make subsequent PRs for openhab-docker
and openhab-linuxpkg
. :)
Upgrading using this PR works very well for me! 😄 👍 I've made a local build of the images and tested upgrading on amd64, with alpine/debian, 2.0.0, 2.1.0, 2.2.0, 2.3.0-snapshot. Perhaps it is better to exclude |
Glad it works for you to. The upcoming update will also have the exclusion of the backups folder from the tar file. Thanks for testing this out. |
- removed extraneous a options from the if statements - removed extraneous echo - use double quotes on all directroies and files - remove the whole cache and tmp folders then recreate them - exclude the backups folder from the tar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements @rkoshak! Please also have a look at my new comments.
entrypoint_alpine.sh
Outdated
|
||
# Make a backup of userdata | ||
backupFile=userdata-$(date +"%FT%H:%M:%S").tar | ||
tar --exclude="${APPDIR}/userdata/backup" cf "/tmp/${backupFile}" "${APPDIR}/userdata" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Alpine TAR creation now fails with:
openhab-upgrade | + tar --exclude=/openhab/userdata/backup cf /tmp/userdata-2018-01-03T21:24:11.tar /openhab/userdata
openhab-upgrade | BusyBox v1.27.2 (2017-12-12 10:41:50 GMT) multi-call binary.
openhab-upgrade |
openhab-upgrade | Usage: tar -[cxtZzJjahmvO] [-X FILE] [-T FILE] [-f TARFILE] [-C DIR] [FILE]...
openhab-upgrade |
openhab-upgrade | Create, extract, or list files from a tar file
openhab-upgrade |
openhab-upgrade | Operation:
openhab-upgrade | c Create
openhab-upgrade | x Extract
openhab-upgrade | t List
openhab-upgrade | f Name of TARFILE ('-' for stdin/out)
openhab-upgrade | C Change to DIR before operation
openhab-upgrade | v Verbose
openhab-upgrade | Z (De)compress using compress
openhab-upgrade | z (De)compress using gzip
openhab-upgrade | J (De)compress using xz
openhab-upgrade | j (De)compress using bzip2
openhab-upgrade | a (De)compress using lzma
openhab-upgrade | O Extract to stdout
openhab-upgrade | h Follow symlinks
openhab-upgrade | m Don't restore mtime
openhab-upgrade | exclude File to exclude
openhab-upgrade | X File with names to exclude
openhab-upgrade | T File with names to include
entrypoint_alpine.sh
Outdated
rm -rf "${APPDIR}/userdata/cache" | ||
rm -rf "${APPDIR}/userdata/tmp" | ||
mkdir "${APPDIR}/userdata/cache" | ||
mkdir "${APPDIR}/userdata/tmp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please correct the indentation
entrypoint_debian.sh
Outdated
|
||
# Make a backup of userdata | ||
backupFile=userdata-$(date +"%FT%H:%M:%S").tar | ||
tar --exclude="${APPDIR}/userdata/backup" cf "/tmp/${backupFile}" "${APPDIR}/userdata" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Debian TAR creation now fails with:
openhab-upgrade | + tar --exclude=/openhab/userdata/backup cf /tmp/userdata-2018-01-03T21:18:25.tar /openhab/userdata
openhab-upgrade | tar: You must specify one of the '-Acdtrux', '--delete' or '--test-label' options
entrypoint_debian.sh
Outdated
rm -rf ${APPDIR}/userdata/cache | ||
rm -rf ${APPDIR}/userdata/tmp | ||
mkdir ${APPDIR}/userdata/cache | ||
mkdir ${APPDIR}/userdata/tmp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add double quotes here too and correct the indentation.
…removed the exclude option from the tar for now pending further research.
OK, this fever is hitting me harder than I thought. Let me go back and figure out what I messed up. I'm pretty sure I ran with the exclude option inside the container to test it but perhaps not. I've removed the excludes option from the tar and fixed the indentation and quotes problems identified. I will need to look into why the tar command doesn't support the standard --exclude option and determine if there is an alternate. If not I'll have to go about excluding the backups some other way. |
IMHO, checking by
Looks like it's better to concatenate |
entrypoint_alpine.sh
Outdated
|
||
# Make a backup of userdata | ||
backupFile=userdata-$(date +"%FT%H:%M:%S").tar | ||
tar -c -f "/tmp/${backupFile}" -X "${APPDIR}/userdata/backup" "${APPDIR}/userdata" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the backup dir does not exist the container fails to start on Alpine with:
openhab-upgrade | + tar -c -f /tmp/userdata-2018-01-04T21:00:58.tar -X /openhab/userdata/backup /openhab/userdata
openhab-upgrade | tar: /openhab/userdata/backup: No such file or directory
So making sure the dir exists will fix it, e.g. add: mkdir -p "${APPDIR}/userdata/backup"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think -X is the problem
… it to before the tar, change the tar to save straight to the backup folder which we can do now that we are excluding the backup folder from the backup and we know it exists. Moved the * outside the double quotes in the cp for the karaf etc files.
Sigh, alpine is a pain in the butt. Seems common sense to me that if you are telling tar to ignore the file, it shouldn't care that the directory doesn't exist in the first place. Moving the test to see if the backup exists and creating it before the tar should fix it. I'll move it on the debian side to keep the two in sync with each other. On the plus side, now that I can guarantee that the backup folder will exist before the tar and we are excluding it, I can skip the step of taring it to tmp and copying it over and just tar is to backups in the first place. I should have thought of that before. It is amazing how much clearer one can think when without a fever. This update should fix the problem with the tar and copy commands (I now know to check Travis) ;-) @andrey-yantsen, you have a good point. I'll submit another update later today or sometime tomorrow when I figure out the best way to use all of the fields in version.properties. Maybe I should just diff the files and see if they are identical and assume any change means a different version. |
:-( I don't understand the travis error. It is failing on a line that isn't part of anything I changed:
As far as I can tell at this point it was an intermittent download error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rkoshak, sorry to be a pain. I missed this the first time round, could you add:
cp "${APPDIR}/userdata.dist/etc/custom.system.properties" "${APPDIR}/userdata/etc"
To that list? This is a file that was created after the first list I suggested and completely forgot about it until now.
Thanks @rkoshak for solving the backup issues! When I upgrade a container from 2.1.0 to 2.2.0 it no longer properly starts. It looks like a new file that was added in 2.2.0 |
Hmmmm. Checking @BClark09's latest upgrade script in the 2.3 snapshot overrides.properties is not one of the files copied over. I do see overrides.properties listed in userdata.dist. Are there any unknown or unexpected side effects if I copy over this file for all versions, or should I only do it for 2.1.0 to 2.2.0 upgrades, or upgrades from 2.1.0 to anything else? @BClark09, does this need to also be addressed in the upgrade script? @BClark09, no problem I'll add that list. Does that file exist for earlier versions of OH too? I'm concerned about needing to add some error checking so it doesn't error out when the file doesn't exist in openhab.dist in earlier versions. I based the original list on the upgrade script for 2.2 snapshot (around build 950) and assumed, falsely that no changes have been made. I should have checked. Of course, I may have just missed it. I'm usually a lot better at this sort of thing. Definitely not showing my best developer skills on the PR. Update forthcoming... |
…ded the missing custom.system.properties and system.properties to the list.
I also discovered system.properties was missing from the list. Added them both. Decided to added overrides.properties as well. Running out of time, I'll pull a 2.1 image tomorrow and check that I haven't broken it because overrides.properties doesn't exist. Drat, upon rereading @wborn's comment now I'm sure I just broke it. Arg. Slow down Rich! |
* Skip copy of non-existing files * Fix excluding backups directory on Alpine * Simplify grep and add missing double quotes * Correct indentation Signed-off-by: Wouter Born <eclipse@maindrain.net>
Oops, it should be!
Afraid not, it's a new file for 2.2 |
I ran into it as well so added a check in my fixes and improvements PR: https://github.com/rkoshak/openhab-docker/pull/1/files#diff-f57fa2e79c10b7b9f67f63d7c84009e3R70 We can add some more elegant checks and logic for adding/removing files in future PRs. The current PR will make working with openHAB containers a lot easier on a daily basis. 👍 |
Just in curiosity: can't we just copy all As for |
Fixes and improvements
…age and container to use cmp which compares the whole file.
Changes have been merged @wborn.
That's a question for @BClark09. I'm strongly of the opinion that we should use the same upgrade approach as the upgrade script in /usr/share/openhab2/bin for an apt/yum installed OH. Since each file is copied over individually in that script I assume there is a good reason why. If there isn't a good reason, then I would prefer to change to the proposed or some other approach until we can extract out the copying of the etc files and clearing of the cache gets pulled out into a single script or some such that apt, yum, the upgrade script and these entrypoint scripts can all call rather than have Docker deviate from the behavior of the existing upgrade script.
That seems a little heavy. I've implemented it using cmp in this latest update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks LGTM now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've changed the way of checking for a new build, but you've forgot about the messages right after this check :)
And you've accidentally committed .entrypoint_debian.sh.swp
entrypoint_alpine.sh
Outdated
@@ -45,6 +45,48 @@ case ${OPENHAB_VERSION} in | |||
cp -av "${APPDIR}/userdata.dist/." "${APPDIR}/userdata/" | |||
fi | |||
|
|||
# Upgrade userdata if versions do not match | |||
if [ ! -z $(cmp "${APPDIR}/userdata/etc/version.properties" "${APPDIR}/userdata.dist/etc/version.properties") ]; then | |||
echo "Image build number \"${imgVersion}\" is different from userdata build number \"${curVersion}\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables imgVersion
and curVersion
are undefined now
entrypoint_debian.sh
Outdated
@@ -60,6 +60,48 @@ case ${OPENHAB_VERSION} in | |||
cp -av "${APPDIR}/userdata.dist/." "${APPDIR}/userdata/" | |||
fi | |||
|
|||
# Upgrade userdata if versions do not match | |||
if [ ! -z $(cmp "${APPDIR}/userdata/etc/version.properties" "${APPDIR}/userdata.dist/etc/version.properties") ]; then | |||
echo "Image build number \"${imgVersion}\" is different from userdata build number \"${curVersion}\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variables imgVersion
and curVersion
are undefined now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly, I'm doing something wrong with git. I made the change to the echo again and checked it in again. Hopefully, it shows up this time.
OK, looks like it matches what I thought the last checkin was supposed to be.
…ow that wasn't part of the last update.
Hmm, my last comment isn't here. I've fixed hopefully the last issue pointed out by @andrey-yantsen. I think it is ready for a final review and whatever the next steps are. |
Review status: all files reviewed at latest revision, 16 unresolved discussions. Comments from Reviewable |
Addresses Issue #148
I used the upgrade script as a template for what to copy over if the versions are different.
I was able to test the entrypoint_debian.sh but not the alpine. Unless grep, cut, tr, or tar do not exist in the alpine image it should not have any problems.
@BClark09, I'd appreciate a review to make sure I didn't miss anything.
Signed off by Richard Koshak rlkoshak@gmail.com
This change is