Seite 1 von 2

Scriptoptimierung aber wie?

Verfasst: Mittwoch 1. Oktober 2008, 08:07
von liberavia
Hallo leute. Habe hier ein Python-Script welches periodisch in einer Datenbank nach jobs schaut
und entsprechende bash scripte startet oder Kommandos direkt absetzt. Das Script
funktioniert, allerdings führt es auf meinem PIII 900MHz als Testmaschine zu einer Systemauslastung von ca. 78% :shock:.

Vermutlich sind meine Aufrufe von os.system Schuld daran. Allerdings hab ich es bisher noch
nicht hinbekommen zu lernnen, wie man es richtig macht. Kann mir mal jemand an diesem Script
demonstrieren, wie man das elegant löst?

Vielen Dank

André

Code: Alles auswählen

#!/usr/bin/python
import os
import MySQLdb
import string
import time
conffile = open("/etc/cbwebgui/cbconnect.conf","r")
for line in conffile:
	confentry = line.split("=")
	if confentry[0] == "dbhost":
		dbhost = confentry[1]
	elif confentry[0] == "dbuser":
		dbuser = confentry[1]
	elif confentry[0] == "dbpasswd":
		dbpasswd = confentry[1]
	elif confentry[0] == "dbname":
		dbname = confentry[1]
conffile.close()
connection = MySQLdb.connect(dbhost[0:-1],dbuser[0:-1],dbpasswd[0:-1],dbname[0:-1])
cursor = connection.cursor()
while True:
	# get the next job
	cursor.execute("SELECT id, command, argument, timestamp FROM jobs_current ORDER BY timestamp LIMIT 1")
	res = cursor.fetchone()
	if res != None:
		# There is a job to do and here are the params
		job_id = res[0]
		job_command = res[1]
		job_argument = res[2]
		job_timestamp = res[3]
		# seperate commands
		if job_command == "restart":
			# seperate restart arguments
			if job_argument == "now":
				os.system("asterisk -rx \"restart now\"")
			elif job_argument == "gracefully":
				os.system("asterisk -rx \"restart gracefully\"")
		elif job_command == "reload":
			# seperate reload arguments
			if job_argument == "all":
				os.system("asterisk -rx reload")
			elif job_argument == "dialplan":
				os.system("asterisk -rx \"dialplan reload\"")
			elif job_argument == "sip":
				os.system("asterisk -rx \"sip reload\"")
		elif job_command == "write":
			# seperate write arguments execute the related script
			if job_argument == "extensions":
				os.system("wextensions %s %s %s" % (dbuser[0:-1],dbpasswd[0:-1],dbname[0:-1]))
			elif job_argument == "sip":
				os.system("wsip %s %s %s" % (dbuser[0:-1],dbpasswd[0:-1],dbname[0:-1]))
			elif job_argument == "queues":
				os.system("wqueues %s %s %s" % (dbuser[0:-1],dbpasswd[0:-1],dbname[0:-1]))
			elif job_argument == "voicemail":
				os.system("wvoicemail %s %s %s" % (dbuser[0:-1],dbpasswd[0:-1],dbname[0:-1]))
			elif job_argument == "agents":
				os.system("wagents %s %s %s" % (dbuser[0:-1],dbpasswd[0:-1],dbname[0:-1]))
			elif job_argument == "meetme":
				os.system("wmeetme %s %s %s" % (dbuser[0:-1],dbpasswd[0:-1],dbname[0:-1]))	
			elif job_argument == "manager":
				os.system("wmanager %s %s %s" % (dbuser[0:-1],dbpasswd[0:-1],dbname[0:-1]))	
		# delete last entry and enter values into jobs_done
		cursor.execute("DELETE FROM jobs_current WHERE id='%s'" % job_id)
		# cursor.execute("INSERT INTO `callbuntu`.`jobs_done` (id, job_id, timestamp, command, argument) VALUES (NULL, %s, %s, %s, %s)" % (job_id, job_timestamp, job_command, job_argument) )
		time.sleep(1)

Re: Scriptoptimierung aber wie?

Verfasst: Mittwoch 1. Oktober 2008, 08:44
von jens
Nur ein paar Anmerkungen:
In deinem Skript sind auffällig viele Teile fast gleich. An einigen Stellen kann man besser ein daten dict nutzten. Besonders bei der job command Geschichte.

Ich würde die DB Daten nicht in Variablen packen, sondern in ein dict:

Code: Alles auswählen

...
db_settings = {}
for line in conffile:
    kind, data = line.split("=",1)
    db_settings[kind] = data.strip()
...
connection = MySQLdb.connect(
    db_settings["dbhost"], db_settings["dbuser"],
    db_settings["dbpasswd"], db_settings["dbname"]
)
...
Das hier:

Code: Alles auswählen

        job_id = res[0]
        job_command = res[1]
        job_argument = res[2]
        job_timestamp = res[3]
müßte auch so gehen:

Code: Alles auswählen

job_id, job_command, job_argument, job_timestamp = res
Vielleicht noch einfacher, du nutzt den MySQL Dict-Cursor:

Code: Alles auswählen

cursor = connection.cursor(MySQLdb.cursors.DictCursor)
Noch was: Ich versuche immer verschachtelungen nicht zu tief werden zu lassen. In deinem Falle würde ich bei if res != None: mit continue arbeiten, also:

Code: Alles auswählen

if res == None:
    continue
Der Code danach ist dann nicht nochmal eingerückt. Noch besser wäre es wohl die ganze Sache ein wenig in Funktionen Aufzuteilen ;)

Statt os.system würde ich subprocess nutzten, siehe: [wiki]Neue Features#Subprocess[/wiki]

All das hat aber wohl nichts mit dem eigentlichen Problem zu tun.

Verfasst: Mittwoch 1. Oktober 2008, 08:58
von liberavia
Vielen Dank für die vielen Anregungen :D
All das hat aber wohl nichts mit dem eigentlichen Problem zu tun.
Was hast du denn im Verdacht, was die Systemauslastung so hoch treibt?

Verfasst: Mittwoch 1. Oktober 2008, 09:00
von jens
Ich denke die System Aufrufe. Messe doch mal die Zeit, während das Programm einen Job abarbeitet.
Du könntest auch mal time.sleep höher setzten und dann die CPU Belastung vergleichen.

Verfasst: Mittwoch 1. Oktober 2008, 09:02
von sma
Dir ist schon klar, dass du FORTLAUFEND in die Datenbank schaust, wenn sie leer ist und nur dann 1 Sekunde wartest, wenn einen Job gefunden, abgearbeitet und gelöscht hast? Du willst bestimmt das sleep() in Zeile 64 einmal ausrücken, damit es immer ausgeführt wird.

Übrigens, den Ansatz ist nicht threadsafe - solltest du jemals zwei deiner Prozesse starten wollen wird es unweigerlich zu Konflikten kommen und zwei Prozesse werden den selben Job ausführen wollen.

Stefan

Verfasst: Mittwoch 1. Oktober 2008, 09:15
von liberavia
Du willst bestimmt das sleep() in Zeile 64 einmal ausrücken, damit es immer ausgeführt wird.
Hoppla :oops: Jetzt weiß ich auch warum die erhöhung des Wertes keine Auswirkung hatte ;)
solltest du jemals zwei deiner Prozesse starten wollen...
Nein, hatte ich nicht vor. Jeder Job soll wie ein stack einzeln abgearbeitet werden.

Aber gegen einen besseren Ansatz habe ich natürlich nix einzuwenden.

Werde die Ansätze so gegen Mittag versuchen umzusetzen, da ich grad noch bei der Weboberfläche bin und diese gegen Falscheingaben absichere...

Werde dann berichten.

Bis denne

André

Verfasst: Mittwoch 1. Oktober 2008, 09:17
von jens
liberavia hat geschrieben:da ich grad noch bei der Weboberfläche bin und diese gegen Falscheingaben absichere...
forms kann da weiterhelfen: http://docs.djangoproject.com/en/dev/topics/forms/

Verfasst: Mittwoch 1. Oktober 2008, 09:38
von liberavia
Die Weboberfläche habe ich mit PHP geschrieben ;)

Trotzdem Danke

Verfasst: Mittwoch 1. Oktober 2008, 09:56
von sma
Um mehr als einen Worker-Prozess zu erlauben, könnte folgendes auch ohne Transaktionen funktionieren:

Füge eine weitere Spalte PID in die Tabelle ein. Diese enthält normalerweise NULL. Bestimme die eigene Prozess-ID. Statt eines SELECT mache jetzt ein "UPDATE jobs_current SET pid=? WHERE pid IS NULL ORDER BY timestamp LIMIT 1" mit der eigenen ID als Argument. Dieses sperrt atomar einen Datensatz gegenüber allen anderen Prozessen. Lade danach den Datensatz mit "SELECT ... WHERE pid=?" falls das Update erfolgreich war. Lösche nach Abarbeitung des Jobs normal den Datensatz.

Nachteil ist, dass so ein Job hängen bleiben kann, wenn dein Worker vor dem Löschen aber nach dem Sperren abstürzt. Das könnten Transaktionen verhindern. Ohne Transaktionen musst du manuell entsperren, etwa indem du periodisch prüfst, ob zu den PIDs in der Datenbank überhaupt Unix-Prozesse existieren. Ein stat() auf /proc/%d sollte reichen, denn so schnell werden die Nummern meines Wissens nicht wiedervergeben.

Stefan

PS: Ich würde das ganze etwas generischer gestalten. Baue dir eine Abbildung von job_command auf das Unix-Kommando und benutzte dort nicht %s sondern %(xyz)s, damit du dann ein dict mit allen möglichen Konfigurationsparametern übergeben kannst und nicht bei jedem Befehl wissen musst, was er wohl braucht.

Code: Alles auswählen

commands = {
  'manager': 'wmanager %(user)s %(passwd)s %(db)s',
  ...
}
config = {
  'user': ...,
  ...
}
def execute(command):
  return os.system(commands[command] % config) == 0 # kann 2x KeyError werfen: Abfangen!

Verfasst: Mittwoch 1. Oktober 2008, 10:10
von BlackJack
@liberavia: Ein wenig Kritik: Zu viel Code auf Modulebene, da sollten möglichst nur Definitionen stehen und das Hauptprogramm sollte in einer Funktion verschwinden.

Für die Konfigurationsdatei würde ich den `ConfigParser` aus der Standardbibliothek verwenden statt selber etwas zu basteln.

So wie's jetzt gelöst ist, wird auch gar nicht überprüft, ob auch alle Optionen gesetzt sind. Und der Code fällt auf die Nase wenn jemand zum Beispiel in seinem Passwort ein '=' verwendet.

Das entfernen des letzten Zeichens machst Du an der falschen Stelle, diese Operation sollte man *einmal* machen. Insbesondere bei `dbname` fällt das auf.

Die einzelnen Jobs würde ich in eigene Funktionen stecken statt diesem riesigen ``if``/``elif``-Gebilde.

`os.system()` ist IMHO keine gute Idee, weil Du Dich um das "escapen" der Argumente für die Shell selber kümmern musst, bzw. da Du das gar nicht tust Probleme mit Aufrufen bis hin zu Sicherheitslücken hast, wenn jemand über die Datenbank "bösartige" Werte an die Shell übergeben lässt. Wenn es da eine Lücke gibt, können beliebige Programme mit den Rechten dieses Skriptes ausgeführt werden.

Ich sehe kein `commit()` bei der Datenbank. Die Zeiten, dass man bei MySQL grundsätzlich sagen konnte, dass man das nicht braucht, sind ja nun auch schon eine Weile vorbei. Und auch wenn das bei der ID wahrscheinlich unproblematisch ist, sollte man aus Sicherheitsgründen keine Werte per Zeichenkettenformatierung in SQL-Anweisungen bringen, sondern das zweite Argument von `execute()` verwenden.

Und natürlich die üblichen Stilkommentare: Zeilen die länger als 80 Zeichen sind. Keine Leerzeichen hinter Kommata.

Verfasst: Mittwoch 1. Oktober 2008, 10:35
von lunar
stat() auf /proc/%d setzt die Existenz des /proc-Verzeichnisses vorraus, was nicht bei jedem Unix der Fall ist. Auch ist diese Operation nicht atomar, und deswegen eine potentielle Race Condition.

Gegen die Benutzung von os.system() sprechen neben potentiellen Sicherheitslücken noch die Schwierigkeiten bei der Fehlerbehandlung. Um Fehler wie nicht existierende Programmdateien zu erkennen, ist man auf die nicht standardisierten Rückgabewerte der Shell und der Funktion selbst angewiesen. subprocess dagegen wirft in solchen Fällen sinnvolle Ausnahmen.

Verfasst: Mittwoch 1. Oktober 2008, 10:52
von BlackJack
Ich hab's mal überarbeitet: http://paste.pocoo.org/show/86694/

Ist natürlich ungetestet.

Verfasst: Mittwoch 1. Oktober 2008, 11:04
von sma
Irgendwie hatte ich Linux angenommen. Doch auch ohne /proc kommt man sicherlich irgendwie an die IDs der laufenden Prozesse heran. Zur Not ruft man "ps" auf - ob nun per os.system oder anders ist mir egal. Mir ging es bei meinem Beitrag nur darum, sicher mit mehreren Worker-Prozessen arbeiten zu können - ohne das man Datenbank-Transaktionen braucht.

In wie fern ist da der Test, ob ein Worker noch lebt, eine Racing-Condition? Habe ich da einen Denkfehler?

Ich bestimme die Liste der (ID, PID) aus der Datenbank wo PID != NULL. Dies sind die aktiven bzw. hängen gebliebenden Jobs. Nun schaue ich, gruppiert nach PID, ob der Prozess noch lebt. Falls nein, ist das ein hängender Job und ich wiederbelebe alle diese Jobs, indem ich PID auf NULL setze. Was ich bei diesem Durchlauf nicht erwische - etwa weil ein Prozess gerade abnibbelt - das kommt beim nächsten Durchlauf in wenigen Sekunden oder Minuten dran. Was mir nicht passieren darf ist, dass eine PID vom OS wiederverwendet wird, sodass ich einen eigentlich toten Job für lebendig halte.

Stefan

Verfasst: Mittwoch 1. Oktober 2008, 11:24
von Y0Gi
Auf `None` sollte man mit `is` bzw. `is not` testen, nicht mit `==`/`!=`, hab ich mal irgendwo gelesen. Und nein, ich denke nicht, dass das hier den Performance-Ausschlag geben wird ;)

Verfasst: Mittwoch 1. Oktober 2008, 11:34
von jens

Verfasst: Mittwoch 1. Oktober 2008, 11:36
von lunar
sma hat geschrieben:In wie fern ist da der Test, ob ein Worker noch lebt, eine Racing-Condition? Habe ich da einen Denkfehler?

Ich bestimme die Liste der (ID, PID) aus der Datenbank wo PID != NULL. Dies sind die aktiven bzw. hängen gebliebenden Jobs. Nun schaue ich, gruppiert nach PID, ob der Prozess noch lebt. Falls nein, ist das ein hängender Job und ich wiederbelebe alle diese Jobs, indem ich PID auf NULL setze. Was ich bei diesem Durchlauf nicht erwische - etwa weil ein Prozess gerade abnibbelt - das kommt beim nächsten Durchlauf in wenigen Sekunden oder Minuten dran. Was mir nicht passieren darf ist, dass eine PID vom OS wiederverwendet wird, sodass ich einen eigentlich toten Job für lebendig halte.
Das kann aber passieren, da die gesamte Operation nicht atomar ist. Der Kernel kann den Prozess durchaus zwischen der Abfrage der PIDs und der Abfrage der Prozesse schlafen legen. Wenn ein anderer Prozess während der Sleepphase nun einen langlebigen der Prozess erzeugt, für den der Kernel eine PID wiederverwendet, dann ist der mit dieser PID assozierte Job effektiv gesperrt.

Es reicht imho nicht, nur zu prüfen, ob ein Prozess mit dieser PID noch lebt, man muss diesen Prozess auch als Worker identifizieren, z.B. in dem man ihn über ein Socket kontaktiert.

Verfasst: Mittwoch 1. Oktober 2008, 11:56
von sma
lunar hat geschrieben:
sma hat geschrieben:Was mir nicht passieren darf ist, dass eine PID vom OS wiederverwendet wird, sodass ich einen eigentlich toten Job für lebendig halte.
Das kann aber passieren, da die gesamte Operation nicht atomar ist.
Diesen Punkt habe ich doch aber die ganze Zeit erwähnt. Ich dachte, du meinst eine weitere prinzipielle Race Condition.

Die Idee, zu prüfen, ob die PID zu einem Worker gehört, ist gut. Wenn man das /proc-Dateisystem hat, ist das auch recht einfach. Dort kann man nachschauen, wie der den Prozess gestarted habende Befehl heißt und einiges mehr.

Am einfachsten wäre aber eine MySQL-Datenbank mit ImmoDB und Transaktionen und ein "SELECT ... FOR UPDATE"-Befehl, der die gefundene Zeile in der DB lockt. Stürzt der Worker dann ab, wird die Transaktion niemals beendet und der Server macht ein Rollback, was die Zeile wieder freigibt.

Stefan

Verfasst: Mittwoch 1. Oktober 2008, 12:28
von liberavia
Wow,

bin echt beeindruckt, wie ihr euch ins Zeug legt. Danke für den "gesäuberten" Code.

Zeigt auf jeden Fall, dass noch viel für mich zu lernen ist :)

Wie muss die Konfigurationsdatei aufgebaut sein? Der Python Interpreter beschwert sich gerade, dass keine Section Header vorhanden sind:

Code: Alles auswählen

Traceback (most recent call last):
  File "/usr/bin/cbjobobserver.py", line 75, in <module>
    main()
  File "/usr/bin/cbjobobserver.py", line 44, in main
    dbhost, dbuser, dbpasswd, dbname = read_config(CONFIG_FILENAME)
  File "/usr/bin/cbjobobserver.py", line 14, in read_config
    config_parser.read(filename)
  File "/usr/lib/python2.5/ConfigParser.py", line 267, in read
    self._read(fp, filename)
  File "/usr/lib/python2.5/ConfigParser.py", line 462, in _read
    raise MissingSectionHeaderError(fpname, lineno, line)
ConfigParser.MissingSectionHeaderError: File contains no section headers.
file: /etc/cbwebgui/cbconnect.conf, line: 1
'dbhost=localhost\n'
P.S. Das ist die Version von jens

Verfasst: Mittwoch 1. Oktober 2008, 12:40
von lunar
sma hat geschrieben:
lunar hat geschrieben:
sma hat geschrieben:Was mir nicht passieren darf ist, dass eine PID vom OS wiederverwendet wird, sodass ich einen eigentlich toten Job für lebendig halte.
Das kann aber passieren, da die gesamte Operation nicht atomar ist.
Diesen Punkt habe ich doch aber die ganze Zeit erwähnt. Ich dachte, du meinst eine weitere prinzipielle Race Condition.
Naja, in deinem ersten Posting mit diesem Vorschlag hast du noch gesagt, dass stat() auf /proc/%d reichen sollte, da die PIDs deines Wissens nach nicht so schnell neu vergeben werden würden. Das ist halt imho nicht unbedingt der Fall.
Die Idee, zu prüfen, ob die PID zu einem Worker gehört, ist gut. Wenn man das /proc-Dateisystem hat, ist das auch recht einfach. Dort kann man nachschauen, wie der den Prozess gestarted habende Befehl heißt und einiges mehr.
Alternativ könnte man auch eine Art von IPC umsetzen (z.B. über DBus oder Sockets), bei der sich verschiedene Worker die Jobs, die sie bearbeiten, gegenseitig mitteilen. Das ist zwar komplexer, kommt dafür ohne Änderungen an der Datenbank aus. Dafür dürfte eine transaktionsgestütze Datenbanklösung wohl am einfachsten zu implementieren sein.

Verfasst: Mittwoch 1. Oktober 2008, 12:50
von BlackJack
@liberavia: Der ConfigParser besteht auf "Sections" in der Ini-Datei:

Code: Alles auswählen

[db]
dbhost=localhost
dbname=foobar
usw.