Fehler...

[nemomuk]

Hallo,

ich habe mich gerade an einem kleinen Cronjob Skript für meinen Server versucht.

1. Wo finde ich unter Debian die Fehlermeldungen von Python?
2. Was könnte man technisch an diesem Skript verbessern?
3. Warum bekomme ich hier immer eine Mail mit FAILED bei Backups und bei den Archiven gibt er nichts mehr aus...

Code: Alles auswählen

nachricht = ""

# Archivieren und backupen
nachricht += "\n\nBackups:\n\n"
import shutil
import time
import os

os.mkdir("/backups/temp/")

try:
    shutil.copytree("/var/www/vhosts/", "/backups/temp/")
    nachricht += "[ OK ] vhosts wurde erfolgreich kopiert.\n"
except:
    nachricht += "[ FAILED ] Beim kopieren der vhosts ist ein Fehler aufgetreten.\n"

kommandos = (
    "mysqldump --opt --user=q --password= g > /backups/temp/g.sql",
    "mysqldump --opt --user=q --password= h > /backups/temp/h.sql",
    "tar cfvz backup_%s.tar.gz /backups/temp/" %time.strftime("%d.%m.%Y_%H_%M",time.localtime())
)
for kommando in kommandos:
    try:
        prozess = os.system(kommando)
        prozess.wait()
        nachricht += "[ OK ] Der Befehl \"%s\" wurde erfolgreich ausgeführt.\n" %kommando
    except:
        nachricht += "[ FAILED ] Der Befehl \"%s\" konnte nicht ausgeführt werden.\n" %kommando

try:
    os.remove("/backups/temp")
    nachricht += "[ OK ] Das Verzeichnis /backups/temp wurde erfolgreich gelöscht.\n"
except:
    nachricht += "[ FAILED ] Das Verzeichnis /backups/temp konnte nicht gelöscht werden.\n"

# Alte Archive löschen
nachricht += "\n\nAlte Archive löschen:\n\n"
import os
from datetime import datetime, timedelta

time_diff = datetime.now() - timedelta(days=10)

for root, dirs, files in os.walk("/backups/"):
    for name in files:
        path = os.path.join(root, name)
        last_mod = datetime.fromtimestamp(os.path.getmtime(path))
        if last_mod < time_diff:
            try:
                os.remove(path)
                nachricht += "[ OK ] Die Datei %s wurde gelöscht.\n" %path
            except OSError:
                nachricht += "[ FAILED ] Die Datei %s konnte nicht gelöscht werden.\n" %path

# Bericht an x senden
import smtplib

recipient = "qwe"
email = '''	
Content-Type: text/plain; charset=UTF-8
From: Server
To: %s
Subject: Server-Cronjob
%s''' %(recipient, nachricht)

mailserver = smtplib.SMTP("localhost")
mailserver.sendmail("Server", recipient, email)
mailserver.quit()

Vielen Dank!

[EyDu]

Hallo,

ich hätte noch ein paar Verbesserungsvorschläge für dich, bei den anderen Fragen kann ich dir leider nicht helfen:

Ich würde die ganze Funktionalität in Funktionen unterteilen und die Ausführung durch einen 'if __name__=="__main__"' schützen. Dann fliegt nicht alles auf Modulebene rum.

Dann sollten zur besseren Übersicht alle Importe im Kopf stehen, einige machst du mittendrin. Das zerreißt meiner Meinung nach ein wenig den Code.

Anstelle "nachricht" als String zu verwalten würde ich daraus eine Liste machen. Wenn du diese dann brauchst, machst du ein

Code: Alles auswählen

"\n".join(nachrichten)

und schon brauchst du dich nicht mehr um die Newlines kümmern. Du könntest eventuell auch Funktionen erstellen, die das "[ OK ]" und das "[ FAILED ]" vor den Nachrichten einfügen. Je nach dem wie groß das Script wird, könnte man auch die Meldungen noch zusammen in ein Dictionary packen, so dass man eine Übersicht über diese hat und ggf. schnell anpassen kann.

Das Escapen der doppelten Anführungszeichen kannst du dir sparen, wenn du den String mit einfachen Anführungszeichen umschließt:

Code: Alles auswählen

'Ein Text mit "Anfuehrungszeichen".'

Dann solltest du bei einigen "except"s noch angeben, welche Exceptions abgefangen werden sollen. Vielleicht ist es ganz hilfreich, die Fehlermeldung mit in die Nachrichten einzubringen.

Die Kommandos würde ich noch in ein Dictionary packen, dann haben sie einen Namen und du kannst immer noch über sie iterieren.

Und zwei Kleinigkeiten noch: hinter den "%"-Operator kommt nach PEP8 noch ein Leerzeichen. Außerdem solltest du dich für englische ODER deutsche Bezeichner entscheiden.

Bis dann.

[CM]

Noch ein zusätzlicher Vorschlag: Wenn Du das logging-Modul verwendest kannst du selber bestimmen wohin Deine logs gehen sollen.

[nemomuk]

Vielen Dank für eure Tips!

So, habe das ganze nun überarbeitet...

Mein problem ist nur, dass die Befehle nicht ausgeführt werden, die im Dict sind, sondern mir immer dieser Fehler ausgegeben wird:

Code: Alles auswählen

tar: /backups/tmp/domains.tar.gz: Cannot open: No such file or directory
tar: Error is not recoverable: exiting now
tar: Removing leading `/' from member names
tar: /backups/tmp/domains.tar.gz: Cannot write: Broken pipe
tar: Error is not recoverable: exiting now
tar: Removing leading `/' from member names
tar: /backups/tmp: Cannot stat: No such file or directory
tar: Error exit delayed from previous errors
sh: /backups/tmp/a.sql: No such file or directory
sh: /backups/tmp/a.sql: No such file or directory

Wenn ich die Befehle manuell auf der Konsole eingebe funktioniert es einwandfrei... ein Neustart des Systems hat leider nicht gebracht.

Jemand eine Idee? Danke!

Code: Alles auswählen

#-*- coding: utf-8 -*-

import shutil
import time
import os
import smtplib
from datetime import datetime, timedelta

messages = []

def backup():
    """ Archiviert alle Domains und Datenbankfiles """
    
    global messages
    messages.append('\nBackups:')
    
    if os.path.exists('/backups/tmp/') == False:
        os.mkdir('/backups/tmp/')

    commands = {
        'DB-Backup' : 'mysqldump --opt --user=a --password=a a > /backups/tmp/a.sql',
        'DB-Backup' : 'mysqldump --opt --user=a --password=a a > /backups/tmp/a',
        'Komprimieren von domains.tar.gz' : 'tar -czf /backups/tmp/domains.tar.gz /var/www/vhosts/',
        'Komprimieren von allen Daten' : 'tar -czf /backups/backup_%s.tar.gz /backups/tmp/' % time.strftime('%d.%m.%Y_%H_%M',time.localtime()),
        'Loeschen von /backups/temp' : 'rm -r /backups/tmp/'
    }
    for action, command in commands.items():
        try:
            process = os.system(command)
            messages.append('[ OK ] %s' % action)
        except OSError:
            messages.append('[ FAILED ] %s' % action)

def delete():
    """ Löscht alle Archive, die älter als X Tage sind """
    
    global messages
    messages.append('\nAlte Archive loeschen:')

    time_diff = datetime.now() - timedelta(days=10)

    counter = 0
    for root, dirs, files in os.walk('/backups/'):
        for name in files:
            path = os.path.join(root, name)
            last_mod = datetime.fromtimestamp(os.path.getmtime(path))
            if last_mod < time_diff:
                try:
                    os.remove(path)
                    messages.append('[ OK ] Die Datei %s wurde geloescht.' % path)
                    counter += 1
                except OSError:
                    messages.append('[ FAILED ] Die Datei %s konnte nicht geloescht werden.' % path)
    if counter == 0:
        messages.append('[ OK ] Keine Dateien wurden geloescht.')

def mailer():
    """ Sendet den Statusbericht an eine eMail-Adresse """
    
    message = ''
    message = '\n'.join(messages)
    recipient = 'a'
    email = '''
    Content-Type: text/plain; charset=UTF-8
    From: Server
    To: %s
    Subject: Server-Cronjob
    %s''' % (recipient, message)

    mailserver = smtplib.SMTP('localhost')
    mailserver.sendmail('Server', recipient, email)
    mailserver.quit()

if __name__ == '__main__':
    backup()
    delete()
    mailer()

[EyDu]

Also ich finde das Programm jetzt wirklich viel schöner

Weil ich dir bei deinem eigentlichen Problem aber keine Ahnung, noch ein paar Kleinigkeiten

Das "messages" als globale Variable würde ich besser lassen (einfach vergessen, dass es "global" überhaupt gibt ). Entweder solltest du es jedes Mal als Parameter an die Funktionen übergeben, oder du machst einfach eine Klasse draus, mit "messages" als Attribut.

Den Vergleich aus Zeile 18 kannst du auch so schreiben:

Code: Alles auswählen

if not os.path.exists('/backups/tmp/'):

Das mit den Funktionen für Nachrichten meinte ich übrigens so:

Code: Alles auswählen

def message(messages, msg):
    messages.append(msg)

def succeeded(messages, msg):
    message(messages, "[ OK ]" + msg)

def failed(messages, msg):
    message(messages, "[ FAILED ]" + msg)

Dann ist Zeile 61 noch überflüssig.

[lunar]

Ich hätte da noch einige kleinere Anmerkungen

``global`` ist eh überflüssig, auch ohne das man ``messages`` "lokalisiert" oder in eine Klasse wrappt. ``messages`` wird nirgendwo neu gebunden, deswegen hat ``global`` gar keinen Effekt und kann einfach entfernt werden. Natürlich ist und bleibt ``messages`` als globale Datenstruktur eine ziemlich unschöne Sache, das ``logging`` Modul wäre hier wesentlich besser geeignet.

``os.system`` würde ich schnellstens loswerden. Damit hast du keine Kontrolle über den Kindprozess, und kannst dir zudem durch das Shell Quoting auch ganz schnell schwer auffindbare Fehler einhandeln. ``subprocess``, ``tarfile`` und ``shutil.rmtree`` sollten die Mittel der Wahl sein. ``shutil`` importierst du ja eh schon, dann kannst du es auch gleich nutzen

Zeile 18/19 erzeugt eine schöne Race Condition, da der Prozess zwischen Überprüfung und Ausführung vom Kernel schlafen gelegt werden kann, wobei sich der Dateisystemzustand ändern kann. Lieber direkt das Anlegen probieren, die Ausnahme abfangen, und überprüfen, ob der Errorcode 17 (EEXIST) zurückkommt.

Ein Wörterbuch als Datenstruktur für ``commands`` ist ungeeignet, weil die Reihenfolge der Elemente relevant ist. Ein Wörterbuch aber ist unsortiert, was bei dir dazu führt, dass ``rm -rf /backups/tmp/`` vor den anderen Kommandos ausgeführt wird. Daher rühren auch die Fehlermeldungen, die du erhältst. Eine Liste von Tupeln wäre in deinem Fall die richtige Datenstruktur.

Die Ausnahmebehandlung in Z 29 ff ist auch eher sinnlos, weil sie nur dann anschlägt, wenn ``/bin/sh`` beim Aufruf von ``os.system`` nicht existiert. In diesem Fall ist der Traceback dann auch dein kleinstes Problem Du willst wahrscheinlich eher den Rückgabewert von ``os.system``, also den Returncode des von ``/bin/sh`` aufgerufenenen Programms prüfen. Auch dabei sitzt dir die Shell allerdings im Weg, da du nur den Return-Code der Shell erhältst. ``subprocess.check_call`` würde sich hier als Ersatz anbieten, noch besser wäre natürlich die Nutzung der oben genannten Module der Standardbibliothek.

In Zeile 53 wäre es ratsam, die Ausnahme mit Namen abzufangen, und diesen Namen dann anstelle von ``path`` im String-Formatting zu verwenden. Das gibt dann auch gleich den Grund mit auf den Weg, warum die Datei nicht gelöscht werden konnte. Ein ``import locale; locale.setlocale(locale.LC_ALL, '')`` direkt nach Z 75 sorgt dann auch dafür, dass die Fehlermeldung in Deutsch erscheint (sofern die Locale-Einstellungen korrekt sind).

Die schon erwähnte Nutzung von ``logging`` ist bei Cronjobs dringenst anzuraten, da man mit einem globalen Exceptionhandler und ``logging.exception`` auch eventuell auftretenden Tracebacks loggen kann, was das Debugging erheblich erleichtert.

Z 61. ist im Übrigen ziemlich überflüssig, da der eben gebundene Name in der nächsten Zeile sofort neu gebunden wird.

Do it!

[nemomuk]

Ich bedanke mich recht herzlich für eure Tips!!

Ich habe jetzt versucht das ganze dementsprechend zu verbessern, wollte nur nochmal fragen, wie man Fehler bei subprocess genau abfangen kann?
Irgendwie mit check_all, nur wie?

Gibt es sonst nmoch irgendwelche Anregunden? Vielen Dank!

Code: Alles auswählen

#-*- coding: utf-8 -*-
import tarfile
import shutil
import subprocess
import time
import os
import smtplib
import datetime
import logging
logging.basicConfig(level=logging.DEBUG,
                    format='%(asctime)s %(levelname)-8s %(message)s',
                    datefmt='%a, %d %b %Y %H:%M:%S',
                    filename='/backups/log',
                    filemode='w')
                    
class Cronjob(object):
    """ Täglicher Cronjob für die Sicherung des Servers. Methoden: backup, delete. """
    
    def __init__(self):
        
        logging.info(' +++ Neues Backup +++ ')
        
        # auszuführende Shellskripte
        self.commands = (
            ('DB-Backup b', 'mysqldump --opt --user=b --password=bb > /backups/tmp/b.sql'),
            ('DB-Backup a', 'mysqldump --opt --user=a --password=a maindata > /backups/tmp/a.sql')
        )
        # zu archivierende Daten -- quelle, ziel
        self.tars = (
            ('/var/www/vhosts', '/backups/tmp/domains.tar.gz'),
            ('/backups/tmp/', '/backups/backup_%s.tar.gz' % time.strftime('%d.%m.%Y_%H_%M',time.localtime())),
        )
        
    def complete(self):
        """Führt den kompletten Cronjob aus. """
        
        self.backup()
        self.delete()
        
    def backup(self):
        """ Archiviert alle Domains und Datenbankfiles """
        
        if not os.path.exists('/backups/tmp/'):
            try:
                os.mkdir('/backups/tmp/')
            except:
                logging.error("Fehler beim erstellen von /backups/tmp/")
                
        for action, command in self.commands:
            subprocess.Popen(command, shell=True)
        
        for src, dest in self.tars:
            try:
                tar = tarfile.open(dest, 'w:gz')
                tar.add(src)
                tar.close()
            except:
                logging.error("Fehler beim komprimieren von %s" % src)
            
        try:    
            shutil.rmtree('/backups/tmp/')
        except:
            logging.error("Fehler beim loeschen von /backups/tmp/")

    def delete(self):
        """ Löscht alle Archive, die älter als X Tage sind """

        time_diff = datetime.datetime.now() - datetime.timedelta(days=10)

        for root, dirs, files in os.walk('/backups/'):
            for name in files:
                path = os.path.join(root, name)
                last_mod = datetime.datetime.fromtimestamp(os.path.getmtime(path))
                if last_mod < time_diff:
                    try:
                        os.remove(path)
                    except:
                        logging.error("Fehler beim loeschen von %s" % path)

if __name__ == '__main__':
    Cronjob().complete()

[EyDu]

Sorry wegen meiner Empfehlung mit den Dictionaries mit den Befehlen und den daraus entstehenden Fehlern. Da habe ich total übersehen, dass diese der Reihe nach abgearbeitet werden sollten und nicht von irgendwo her einzeln aufgerufen werden .

[BlackJack]

Klassemissbrauch würde ich sagen. Das ist im Grunde ein Haufen Funktionen die über globale Variablen kommunizieren, als Klasse verkleidet.

[nemomuk]

naja, vorhin wurde mir gesagt, dass ich das ganze evtl. in eine Klasse packen soll..., weil globale Variablen schlecht sind...

Wie sollte ich es denn dann machen?

[Hyperion]

Wie sollte ich es denn dann machen?

Wie wärs mit Parametern? So viele Attribute nutzt Du ja nicht ... also kannst Du die auch übergeben!

[nemomuk]

naja, dieses Skript wird von mir ja noch erweitert werden mit weiteren Methoden und Attributen, bzw. Einstellungsmöglichkeiten als Attribute.

Mir ist es jetzt nur mal grundsätzlich um die Technik gegangen...

[lunar]

Ich habe jetzt versucht das ganze dementsprechend zu verbessern, wollte nur nochmal fragen, wie man Fehler bei subprocess genau abfangen kann?
Irgendwie mit check_all, nur wie?

Was genau ist an der Dokumentation so schwer zu verstehen?

Wenn du schon except ohne Typangabe verwenden willst, dann bitte wenigstens mit ``logging.exception``, so bleibt wenigstens der Traceback erhalten. Besser wäre es natürlich, typisierte Abfangklauseln zu verwenden und **aussagekräftige** Fehlermeldungen ins Log zu schreiben. Exceptions haben nicht umsonst eine ``__str__`` Methode

Zeile 43ff ist ziemlich seltsam. Du nutzt die Abfrage trotzdem, obwohl (!) du eh noch mal Ausnahmen abfängst. Wo liegt da der Sinn? Viel sinnvoller ist doch folgendes:

Code: Alles auswählen

try:
    os.mkdir('/backups/tmp')
except OSError, err
    if err.errno != 17:  # alternativ errno.EEXIST
        logging.error('Fehler beim Erstellen des temporären Verzeichnisses: %s', err)
except Exception:
     # traceback sichern
     logging.exception('Ausnahme aufgetreten')

Wenn du ``subprocess`` mit ``shell=True`` nutzt, kannst du auch gleich wieder ``os.system`` nutzen. Dann ist der Vorteil von ``subprocess`` eh hinfällig. ``mysqldump`` rufst du besser so auf:

Code: Alles auswählen

cmd=['mysqldump', '--opt', '--user=b', '--pasword=bb']
proc = subprocess.Popen(cmd, stdout=subprocess.PIPE)
with open('/backups/tmp/b.sql') as stream:
    for line in proc.stdout:
         stream.write(line)

Das ist zwar länger, aber dafür steht dir die Shell nicht im Weg. Das bedeutet unter anderem, dass du im Falle eines Fehlers **aussagekräftige** und **typisierte** Ausnahme erhältst anstatt bloß den - per se ziemlich nichts sagenden - Rückgabewert von ``/bin/sh`.

Zeile 56 gehört in einen ``finally``-Block, da die Datei im Falle eines Fehlers sonst geöffnet bleibt.

Der ``smptlib`` Import ist im Übrigen überflüssig.

Du kannst die Klasse sinnvoll werden lassen, in dem du sie generisch machst. Das bedeutet, dass z.B. die zu erstellenden Backups nicht hart in der Klasse kodiert werden, sondern als Argument übergeben werden. Das wäre für diese kleine Aufgabe allerdings irgendwie over engineered.

Funktionen, die über Argumente kommunizieren, sind in diesem Fall wohl der offensichtlichste Ansatz, oder?

[nemomuk]

also, erstmal Vielen Dank für deine Bemühungen!

Ich habe möglichst alles umgestellt und habe versucht die Klasse sinnvoller einzusetzen. Die klasse agiert jetzt als allgemeine Backup-"Funktion", mit der ich alle meine Backups durchführe...
Ich habe das ganze nun in zwei Dateien gesplittet:

backup.py:

Code: Alles auswählen

#-*- coding: utf-8 -*-
import tarfile
import shutil
import subprocess
import time
import os
import datetime
import logging
logging.basicConfig(level=logging.DEBUG,
                    format='%(asctime)s %(levelname)-8s %(message)s',
                    datefmt='%a, %d %b %Y %H:%M:%S',
                    filename='/backups/log',
                    filemode='w')
                    
class Cronjob(object):
    """ Täglicher Cronjob für die Sicherung des Servers. Methoden: backup, delete. """
    
    def __init__(self,commands,tars):
        
        logging.info(' +++ Neues Backup +++ ')

        self.commands = commands # auszuführende Shellskripte
        self.tars = tars # zu archivierende Daten -- quelle, ziel
        
    def backup(self):
        """ Archiviert alle Domains und Datenbankfiles """
        
        try:
            os.mkdir('/backups/tmp')
        except:
            logging.exception('Fehler aufgetreten.')
                
        for command in self.commands:
            subprocess.Popen(command, shell=False)
        
        for src, dest in self.tars:
            try:
                tar = tarfile.open(dest, 'w:gz')
                tar.add(src)
            except:
                logging.exception('Fehler aufgetreten.')
            finally:
                tar.close()
            
        try:    
            shutil.rmtree('/backups/tmp/')
        except:
            logging.exception('Fehler aufgetreten.')

    def delete(self):
        """ Löscht alle Archive, die älter als X Tage sind """

        time_diff = datetime.datetime.now() - datetime.timedelta(days=10)

        for root, dirs, files in os.walk('/backups/'):
            for name in files:
                path = os.path.join(root, name)
                last_mod = datetime.datetime.fromtimestamp(os.path.getmtime(path))
                if last_mod < time_diff:
                    try:
                        os.remove(path)
                    except:
                        logging.exception('Fehler aufgetreten.')

cron.py:

Code: Alles auswählen

import sys
sys.path.insert(0, '')
import backup

def complete():
    commands = (
        'mysqldump --opt --user=a --password=a a > /backups/tmp/a.sql',
        'mysqldump --opt --user=a --password=a a > /backups/tmp/a.sql'
    )
    tars = (
        ('/var/www/vhosts', '/backups/tmp/domains.tar.gz'),
        ('/backups/tmp/', '/backups/standard_%s.tar.gz' % time.strftime('%d.%m.%Y_%H_%M',time.localtime())),
    )
    exe = Cronjob(commands, tars)
    exe.backup()
    exe.delete()
    
if __name__ == '__main__':
    complete()

Ich habe das mit subprocess im einfacheren Modus gelassen, da mir das dann als etwas over engineered vorgekommen ist^^

Meine Frage nun, ich würde gerne das Skript cron.py mit verschiedenen Paramtern aufrufen... zB cron.py --complete oder cron.py --domains oder ähnliches und somit verschiedene Funktionen innerhalb davon aufrufen...
Wie könnte man soetwas realisieren?

Danke!

[BlackJack]

Also ich finde das immer noch "over engineered". Die Klasse erfüllt keinen wirklichen Zweck im OOP-Sinn. Wo ist der Vorteil gegenüber Funktionen?

[lunar]

Der Code funktioniert nicht mehr. Wenn du beim Aufruf von ``subprocess.Popen`` ``shell`` auf ``False`` setzt, dann musst du das Kommando als Liste übergeben, und die Ausgabeumleitung selbst implementieren. Da sitzt dann nämlich keine Shell mehr dazwischen, die den Umleitungsoperator interpretieren könnte.

Auch solltest du darauf warten, dass sich der Prozess beendet, bevor du sein "Arbeitsverzeichnis" löschst.