Offer rollback option for interactive upgrading #11771
Conversation
|
@nicok1 das diff is leider kaum lesbar- warum sind post installs angepasst? Könntest Du den PR so anpassen, dass das echte Delta erkennbar wird? |
|
@andig Im Post Install Skript ist die neue evcc Binary bereits am entsprechenden Pfad vorhanden. Diese Datei wird ja benötigt, um die bestehende Config auf die neue Version zu prüfen. Deshalb wird dort bei einem Fehler der Nutzer gefragt und dann gegebenenfalls die alte Version aus dem /tmp Verzeichnis zurück kopiert und der Post Install schlägt fehl. |
|
Ich würde ein Change an den Line Ending als Ursache für das große diff vermuten |
|
Hallo StefanSchoof, |
|
Wenn man ein revert macht, wird die Versionsnummer bei dpkg und von den evcc binary sich unterscheiden. Ich weiß gerade nicht, ob das nur kosmetisch ist oder irgendwie zu Problemen führen kann. |
|
Das Skript gibt ja Exitcode 1 zurück, falls der Nutzer ein Rollback wünscht. Nach meinem Verständnis würde der Package Manager versuchen, das Upgrade rückabzuwickeln auf die alte Version und ein eigenes Kopieren ins /tmp-Verzeichnis der Binary und zurück wäre eigentlich nicht zwingend erforderlich. Ich fand es aber zugegebenermaßen nicht so leicht zu verstehen und wäre froh darüber, wenn jemand das ebenfalls noch mal durchliest. |
Co-authored-by: andig <cpuidle@gmx.de>
|
Ist das nicht postinstall https://www.debian.org/doc/debian-policy/ch-maintainerscripts#details-of-configuration und somit
Hast du mal ein package gebaut und ausprobiert, wie das verhalten ist? |
…he old version. And small fix.
packaging/scripts/postinstall.sh
Outdated
| fi | ||
|
|
||
| if [ -f /tmp/oldevcc ] && [ $INTERACTIVE -eq 1 ]; then | ||
| checkConfigOutput=$(/usr/bin/evcc checkconfig 2>&1 || true) |
There was a problem hiding this comment.
What's the point of the OR with true here?
There was a problem hiding this comment.
Thank you for your feedback. The modification to this line is intended to prevent the package manager from halting the script execution if the subcommand returns a non-zero exit code. By appending '|| true'.
I have run some tests on the behaviour of the package manager that we can discuss (as soon as I have more time). This may result in a few more changes.
There was a problem hiding this comment.
OK interesting - especially the part about the subcommand / subshell RC influencing postinstall.sh. I would fail to see how postinstall.sh can even know about any RC from the variable substitution as it does not capture it. But I'll take your word for it.
Feel free to mark this as resolved and to ping me again on this subject once you had the time to fiddle around more, and want to bounce around ideas.
packaging/scripts/postinstall.sh
Outdated
| checkConfigOutput=$(/usr/bin/evcc checkconfig 2>&1 || true) | ||
| oldevccversion=$(/tmp/oldevcc -v) | ||
| newevccversion=$(/usr/bin/evcc -v) | ||
| if echo "$checkConfigOutput" | grep -q "config valid"; then |
There was a problem hiding this comment.
if [ "$checkConfigOutput" = "config valid" ]
There was a problem hiding this comment.
Solche Änderungen kannst Du auch direkt inline zum applien vorschlagen ;)
There was a problem hiding this comment.
That makes no sense at this point. If successful, the checkconfig command does not return exactly "config valid", but also dynamic information such as the version number. It should therefore be checked for contains and not for equals.
There was a problem hiding this comment.
Sorry about that. I checked what the command does, but apparently I did not look close enough. My bad.
packaging/scripts/postinstall.sh
Outdated
|
|
||
| if [ "$oldevccversion" = "$newevccversion" ]; then | ||
| echo "--------------------------------------------------------------------------------" | ||
| echo "Old evcc version detected. To apply the new version, please reinstall evcc. (e.g. apt-get reinstall evcc)" |
There was a problem hiding this comment.
This is somewhat confusing.
The check is for old and new version being equal, yet, the log message claims and old version was detected.
What does "old version" in this context mean?
There was a problem hiding this comment.
Yeah, I understand that. To explain it: If the version in /tmp/oldevcc is the same as /usr/bin/evcc, then a rollback was done before and then the apt-get install command was used (again). In order to really get the latest binary after correcting the config, either a newer release must be installed with apt-get install, or the current release must be reinstalled with apt-get reinstall. Perhaps you have an idea for a more suitable feedback?
Here is an example of dpkg -s evcc after a rollback:
klein@NPC:~/evcc$ dpkg -s evcc
Package: evcc
Status: install ok half-configured
Priority: optional
Installed-Size: 78064
Maintainer: andig <cpuidle@gmx.de>
Architecture: amd64
Version: 0.123.9~next ----> But Binary is 0.123.8
Config-Version: 0.123.8
Description: EV Charge Controller
Homepage: https://evcc.ioThere was a problem hiding this comment.
Now I'm even more confused. 😅
Does the above dpkg output indicate, that the rollback does not actually roll back, in the sense that while it did restore the old version (0.123.8), the package manager now thinks 0.123.9 is already installed?
If so, that sounds like a total nightmare/deal breaker. 😢
packaging/scripts/postinstall.sh
Outdated
| else | ||
| if [ "$oldevccversion" = "$newevccversion" ]; then | ||
| echo "--------------------------------------------------------------------------------" | ||
| echo "Old evcc version detected. To apply the new version, please reinstall evcc. (e.g. apt-get reinstall evcc)" |
There was a problem hiding this comment.
Same as above.
Also - any chance to deduplicate?
packaging/scripts/postinstall.sh
Outdated
| echo "$checkConfigOutput" | ||
| echo "--------------------------------------------------------------------------------" | ||
|
|
||
| while true; do |
There was a problem hiding this comment.
Either this while-block is incorrectly indented, or it is accidentally added to the else branch above. (Or Github UI is messing up my view...)
There was a problem hiding this comment.
The indentation issue has been resolved. However, I can confirm that the placement of the loop is correct.
packaging/scripts/postinstall.sh
Outdated
| @@ -99,3 +160,8 @@ if [ "$1" = "configure" ] || [ "$1" = "abort-upgrade" ] || [ "$1" = "abort-decon | |||
| fi | |||
| fi | |||
| fi | |||
|
|
|||
| # Fail installation if checkconfig command failed and the user decided to keep the old version to inform package manager about outcome | |||
There was a problem hiding this comment.
Does it make sense to put this at the end? Assuming the config check failed, the above lines would still try to activate/restart the currently running service instance. 🤔
There was a problem hiding this comment.
The idea is that the service should continue to use the old binary in the event of an invalid config and the user's rollback request, and that the service continues to function as a result. Exit code 1 ensures that the package manager recognises the installation as not completely successful and remembers the last working config/version. Alternatively, the user can choose to use a non-functioning newer version, as was previously the case by default.
packaging/scripts/preinstall.sh
Outdated
| @@ -101,4 +101,9 @@ if [ "$1" = "upgrade" ]; then | |||
| copyDbToUserDir | |||
| fi | |||
|
|
|||
| # copy current binary to /tmp/oldevcc (for possibility to rollback) | |||
| if [ -f /usr/bin/evcc ]; then | |||
There was a problem hiding this comment.
I'd use the "$1" = upgrade check here, just to stay within the overall "standards" this script tries to apply.
Simply moving the cp into the block above below copyDbToUserDir should do the trick.
There was a problem hiding this comment.
That's right. That should work. Thank you. I will apply it.
Ich habe eine Hand voll Tests ausgeführt. So sieht es nach einem Rollback aus: klein@NPC:~/evcc$ dpkg -s evcc
Package: evcc
Status: install ok half-configured
Priority: optional
Installed-Size: 78064
Maintainer: andig <cpuidle@gmx.de>
Architecture: amd64
Version: 0.123.9~next ----> But Binary is 0.123.8
Config-Version: 0.123.8
Description: EV Charge Controller
Homepage: https://evcc.ioDu hast also recht. Man könnte die neuere Binary nach einem Rollback ebenfalls speichern und könnte bei erneutem Aufruf des regulären Install Befehls die neuere Binary zurück kopieren. (Nur, wenn seitdem kein neuer Evcc Release vorliegt). So würde die Config wieder mit der neuen Version geprüft werden und es wäre wieder Erfolg/Rollback/Beibehalten als möglicher Ausgang denkbar, ohne einen anderen Befehl verwenden zu müssen. Das würde aber die Komplexität noch ein Stück weit erhöhen. |
This reverts commit 0643027.
Puh. Je mehr ich mich hier reinfuchse, desto überraschter bin ich. Das hier kein Support für atomare Operationen im dpkg Standard existiert ist echt bemerkenswert. Ich dachte sowas hat man schon vor 25 Jahren gelöst. |
|
Wäre "old-postrm upgrade new-version" ein besserer Platz für den config check? Wenn ich die Doku gerade verstehe sollte zu dem Moment das neue evcc binary am Platz sein und ein error unwind möglich sein, um zur alten Version per dpkg zurück zu kehren. |
|
@StefanSchoof meinst Du konkret aus https://www.debian.org/doc/debian-policy/ch-maintainerscripts.html Absatz 6.6 Punkt Nummer 5 ?
Und dann ggf. die Kaskade für den Failzweig? Das sieht zumindest nach der korrekten Stelle aus. Laut Doku ist man dort auch noch vor dem "Point of no return". |
|
Ja und man ist nach 4. wo das neue binary abgelegt wurde. Schade ist nur das es ein Skript aus dem installieren package und nicht dem neuen ist. Damit dauert es länger bis das bei Nutzern ist und wenn das Skript Probleme macht kann man nicht einfach eine neue Version mit einen Fix veröffentlichen. |
|
Danke euch beiden. Ich habe nachgedacht und verstanden, was ihr vorhabt und wie ihr euch das vorstellt. Die Frage ist, ob überhaupt die alte Datei zurück kopiert werden muss oder ob das nicht jetzt sogar dpkg inkl. richtiger Protokollierung der Version von selbst macht. Dpkg scheint ja laut Doku selbst ein temporäres Backup der alten Dateien anzulegen. Falls ja, werde ich den Code zeitnah nochmal anpassen. |
|
Damit keine Missverständnisse auf kommen: Ich bin nicht Teil des evcc Teams und somit sind die Kommentare von mir nur Hinweise. Das Team kann das anders sehen. |
|
Ich hab von Paketmanagern keine Ahnung und bin für jede Hilfe dankbar- da muss ich mich voll auf Euch verlassen! |
Ich nenne sowas "engagierter Nutzer" 😄 |
Genau. Wenn Du in die Richtung nochmal experimentieren würdest wäre das super. |
|
Vielleicht klappt es auch in |
|
Ich habe noch ein wenig getestet und das Skript angepasst. Es sieht jetzt eigentlich relativ gut aus. Falls der Nutzer ein Rollback möchte, kümmert sich dpkg darum und protokolliert am Ende auch die richtige Version ohne seltsamen Status. Wenn ein Rollback passieren soll, muss der Upgrade-Schritt scheitern sowie der "new-postrm failed-upgrade old-version new-version" Schritt. Dann folgt der "Error Unwind". Das Kopieren der alten Version in tmp ist nicht mehr notwendig. Das ist definitiv sauberer und gefällt mir wesentlich besser. Die beiden Schritte nochmal aufzuteilen (Test und Abfrage) hat bei mir nicht so gut geklappt. Wenn ihr möchtet, könnt ihr ja auch mal das Verhalten testen. Am einfachsten klappt es mit dem reinstall Befehl. (geg. vorher 0.123.8 Binary entsprechend plazieren) |
|
@nicok1 gab es hier nochmal positive Erfahrung? Wenn ja, dann würde ich vorschlagen das wir den PR nicht mehr als draft sehen und einen merge anstreben sollten. |
|
@GrimmiMeloni |
|
Klasse PR, danke für Deine Geduld. Jetzt schauen wir einfach mal, wie gut das funktioniert... |
|
@nicok1 could we just check for exit code instead of actual string match? Problem is that depending on log level the affirmative |
Summary
This pull request introduces an update to the evcc upgrade process (e.g., via apt-get) to include a rollback option in interactive environments. This enhancement ensures a safer upgrade experience by validating the compatibility of existing configurations with the new evcc binary.
Implementation Details
Pre-Install Script: During the pre-install phase, if an evcc binary exists, it is backed up to /tmp/oldevcc.
Post-Install Script: In interactive mode, if /tmp/oldevcc contains a backup, the post-install script employs the new checkconfig command to test the configuration with the updated binary.
User Choice on Failure: If checkconfig fails, indicating an incompatibility, the user is prompted with a choice: proceed with the installation despite the incompatibility or rollback to the old binary.
Rollback Mechanism: If the user opts for rollback, the script restores the old evcc binary from /tmp/oldevcc, effectively undoing the upgrade.
Questions
I would be happy to receive feedback and support.