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

Rewrite maintenance.sh #243

Closed
wants to merge 3 commits into from
Closed

Rewrite maintenance.sh #243

wants to merge 3 commits into from

Conversation

agross
Copy link
Contributor

@agross agross commented Feb 13, 2022

Ich hab nach unserer Diskussion in #233 das maintenance.sh-Skript etwas umgeschrieben.

Dazu ein paar Anmerkungen:

  • POSIX [ und Bash [[ zu [[ vereinheitlicht
  • Quoting aller Variablen
  • Bash Strict Mode aktiviert, alle return codes != 0 müssen explizit mit if behandelt werden oder das Skript bricht ab
  • Shellcheck-Warnings behoben (hauptsächlich read ohne -r betreffend)
  • maint und m existiert bei mir nicht, alle möglichen Aufrufe in der Hilfe durch BASH_SOURCE ersetzt
  • Command Line Parsing verbessert, kein reverse notwendig
  • diverse Duplikationen entfernt, in Funktionen oder Variablen ausgelagert
  • if-Nesting verringert
  • pkill und pgrep wie in #233 besprochen mit Timeout versehen

Jetzt gibt es bei mir tatsächlich ein Problem mit pgrep nach pkill:

  • Ich nutze den adb-Adapter
  • Dieser startet eine adb Executable als iobroker-User
  • adb reagiert nicht auf SIGTERM
  • Bei mir rennt das ioBroker-Beenden deshalb immer in den Timeout

Mögliche Lösungen:

  1. Special Case für adb schaffen und Instanzen dieses Adapaters beim Aktivieren der Maintenance beenden (vielleicht geht das per iob.
  2. Nach dem Timeout noch ein SIGKILL nachschieben.

Vielleicht hat @Apollon77 hierfür eine gute Idee.

Da es gestern ein Update des js-controllers gab, so sieht der Output aus (adb hatte ich vorher beendet). Nach Stopping ioBroker... gibt's eine Zeile mit ein paar Punkten, dort wurde gewartet dass alle Prozesse beendet wurden. Pro Punkt 500ms == 8 Sekunden, also über dem aktuell hartcodierten Timeout von 5 Sekunden.

root@iobroker:/opt/scripts# ./maintenance.sh upgrade
You are now going to upgrade your js-controller.
As this will change data in /opt/iobroker, make sure you have a backup!
During the upgrade process, the container will automatically switch into maintenance mode and stop ioBroker.
Depending on the restart policy, your container will be stopped or restarted automatically after the upgrade.
Do you want to continue [yes/no]? yes
You are now going to stop ioBroker and activate maintenance mode for this container.
This command was already confirmed by the -y or --yes option.
Activating maintenance mode...
Stopping ioBroker...
................
Upgrading js-controller...
^NUsed repository: beta
Adapter    "accuweather"  : 1.2.4    , installed 1.2.4
Adapter    "adb"          : 0.0.5    , installed 0.0.5
...
Update js-controller from @4.0.7 to @4.0.8
Stopped Objects DB
Stopped States DB
NPM version: 6.14.16
Installing iobroker.js-controller@4.0.8... (System call)

> iobroker.js-controller@4.0.8 preinstall /opt/iobroker/node_modules/iobroker.js-controller
> node lib/preinstallCheck.js

NPM version: 6.14.16

> iobroker.js-controller@4.0.8 install /opt/iobroker/node_modules/iobroker.js-controller
> node iobroker.js setup first

Successfully migrated 8459 objects to Redis Sets
object _design/system updated

{
  "defaultPrivate": "-----BEGIN RSA PRIVATE KEY-----\r\nMIIEpQIBAAKCAQEA6Ir5IFyKBHYDIGrfxtZ9JfZt9SFrW9SVMTTKrCMVhdr7F0XO\r\nINtAJnRwzZpxzSSSj7abfZwlg9kiK3aY",
  "defaultPublic": "-----BEGIN CERTIFICATE-----\r\nMIIDfjCCAmagAwIBAgIJD8rZcscFrXvcMA0GCSqGSIb3DQEBCwUAMD4xETAPBgNV\r\nBAMTCGlvYnJva2VyMRYwFAYDV"
}
Update certificate defaultPrivate
The object "system.certificates" was updated successfully.
Update certificate defaultPublic
The object "system.certificates" was updated successfully.
+ iobroker.js-controller@4.0.8
added 1 package from 2 contributors, removed 34 packages and updated 25 packages in 95.976s
^[[B
119 packages are looking for funding
  run `npm fund` for details

Done.
Container will be stopped or restarted...
Terminated
root@iobroker:/opt/scripts# %

pi@raspberrypi ~
$

@buanet
Copy link
Owner

buanet commented Feb 14, 2022

Danke für die Mühe die du dir gemacht hast! Das muss ich mir zusammen mit Google alles mal genauer ansehen. :) Sind ein paar Sachen drin die ich so (noch) nicht kenne, vermutlich aber Sinn ergeben.
Mein Anspruch ist, dass ich den Code im Repo auch zu 100% verstehe. Daher muss ich vorher erstmal meine bash Skills upgraden :)

Was die Optionen zum Beenden angeht denke ich dass wir das nicht allzu sehr ausreizen sollten. Ich würde daher nicht damit anfangen irgendwelche Sonderlocken für einzelne Adapter zu machen.
Ich denke ein SIGTERM mit 10 Sekunden und dann ein SIGKILL sind völlig ausreichend. Anders macht es der Docker Daemon auch nicht. (Ja ich weiß, das Timeout kann man in Docker anpassen. Aber sind wir mal ehrlich, das wird kein standard User von sich aus tun)

MfG,
André

@agross
Copy link
Contributor Author

agross commented Feb 14, 2022

Danke für die Mühe die du dir gemacht hast!

Kein Problem. Danke, dass du dir die Arbeit mit dem Image gemacht hast ;-)

Daher muss ich vorher erstmal meine bash Skills upgraden :)

Guter Ansatz. Lass mich wissen wenn du Fragen hast. Ansonsten kann ich den Artikel von #233 immer noch empfehlen ;-)

Ich denke ein SIGTERM mit 10 Sekunden und dann ein SIGKILL sind völlig ausreichend.

Gut, ich bau das mit dem SIGKILL bei Timeout noch ein.

Happy path: TERM -> alles beendet sich innerhalb 15 Sekunden
Unhappy path: TERM -> jemand lebt nach 15 Sekunden noch -> KILL -> ohne warten fortsetzen

Einverstanden?

@agross
Copy link
Contributor Author

agross commented Feb 17, 2022

Die Aktualisierung mit SIGKILL nach 15 Sekunden ist drin.

Gerade probiert mit dem Überbleibsel vom adb-Adapter:

...
Depending on the restart policy, your container will be stopped or restarted automatically after the upgrade.
Do you want to continue [yes/no]? yes
You are now going to stop ioBroker and activate maintenance mode for this container.
This command was already confirmed by the -y or --yes option.
Activating maintenance mode...
Stopping ioBroker...
...............................timed out, killing remaining processes:
1989577 adb -L tcp:5037 fork-server server --reply-fd 4
Upgrading js-controller...
Used repository: beta
Adapter    "accuweather"  : 1.2.4    , installed 1.2.4
Adapter    "adb"          : 0.0.5    , installed 0.0.5
...

@buanet
Copy link
Owner

buanet commented Jun 13, 2022

Moin,
bin jetzt endlich dazu gekommen mir mal deine ganzen Anpassungen und Optimierungen zu erarbeiten. Habe dabei wie erwartet eine Menge gelernt. :) Danke also nochmals für den Hirnschmalz den du da reingesteckt hast.

Mit dem letzten commit (b8c67b7) habe ich dann viele deiner Anregungen übernommen und teils sogar erweitert/ optimiert. Kannst ja mal einen Blick riskieren.
Die Timeout Schleife habe ich dann auch gleich noch im startup script für den graceful shutdown übernommen.

MfG,
André

@agross
Copy link
Contributor Author

agross commented Jun 15, 2022

Dann schauen wir mal wie das Feedback dazu in den vielen Produktionsinstanzen ausfällt ;-)

@agross agross closed this Jun 15, 2022
@agross agross deleted the maintance-script branch June 15, 2022 07:51
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.

2 participants