Merge ~tcuthbert/container-log-archive-charm:master into container-log-archive-charm:master

[Thomas Cuthbert]

Implement console and filebased logging

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Stuart Bishop (stub)]

This looks mostly fine. Per inline comments, it would be better if the log file didn't get created at import time. And we want to use a different class to FileHandler because we are rotating the logs. You could drop the logrotate stuff entirely if you use one of the Python builtin log rotating handlers, but they might not have all the features you want such as compression.

review: Needs Fixing

[Stuart Bishop (stub)]

If we have focal/python3.8, I think the following will lazily instantiate the log:

_log = None

def __getattr__(name):
    if name == 'log':
        global _log
        if _log is None:
            _log = create_logger()
        return _log
    raise AttributeError(name)

(per 3rd answer on https://stackoverflow.com/questions/880530/can-modules-have-properties-the-same-way-that-objects-can#880550)

[Stuart Bishop (stub)]

The log instances are still getting created at import time unfortunately; the 'log = get_log()' lines are still evaluated at import time, so this hasn't changed much. I'd probably revert the last commit (except for the WatchedFileHandler bit, which we need).

I've realized the simplest fix for the issue I raised is to rename create_logger() initialize_logging(), and to initialize the module global 'log' variables with just log=logging.getLogger(). The modules will then get loggers with no handlers attached. You then call initialize_logging() from the __main__ section of your scripts, and the handlers get attached.

[Stuart Bishop (stub)]

Yup

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 15e5780ea039a712d25937961e36a72ac662a716

diff --git a/files/archive-to-azure.py b/files/archive-to-azure.py
index 4af0861..82a0d54 100755
--- a/files/archive-to-azure.py
+++ b/files/archive-to-azure.py
@@ -16,7 +16,6 @@
 # 'archive-to-azure.py -s -f [config_file] -l [logdir1] -l [logdir2] -c [container_name] -a [archive_after] -d [delete_after] -p [retention_days]'
 
 import os
-import logging
 import sys
 import datetime
 
@@ -26,9 +25,7 @@ import hmac
 import hashlib
 import base64
 import xml.etree.ElementTree as ET
-from archive import CloudStorage, main, Status
-
-log = logging.getLogger()
+from archive import CloudStorage, main, initialise_logger, Status
 
 
 class AzureCloudStorage(CloudStorage):
@@ -310,4 +307,5 @@ class AzureCloudStorage(CloudStorage):
 
 
 if __name__ == "__main__":
+    log = initialise_logger()
     sys.exit(main(AzureCloudStorage))
diff --git a/files/archive-to-gce.py b/files/archive-to-gce.py
index 147626c..d32917f 100755
--- a/files/archive-to-gce.py
+++ b/files/archive-to-gce.py
@@ -18,7 +18,6 @@
 import os
 import io
 import json
-import logging
 import sys
 import datetime
 
@@ -27,9 +26,7 @@ import apiclient
 from apiclient import discovery
 from apiclient.http import MediaIoBaseUpload
 from oauth2client.service_account import ServiceAccountCredentials
-from archive import CloudStorage, main, Status
-
-log = logging.getLogger()
+from archive import CloudStorage, main, initialise_logger, Status
 
 
 class GCECloudStorage(CloudStorage):
@@ -160,4 +157,5 @@ class GCECloudStorage(CloudStorage):
 
 
 if __name__ == "__main__":
+    log = initialise_logger()
     sys.exit(main(GCECloudStorage))
diff --git a/files/archive-to-s3.py b/files/archive-to-s3.py
index 45af6a4..c82cbd0 100755
--- a/files/archive-to-s3.py
+++ b/files/archive-to-s3.py
@@ -16,16 +16,13 @@
 # 'archive-to-s3.py -s -f [config_file] -l [logdir1] -l [logdir2] -c [container_name] -a [archive_after] -d [delete_after] -p [retention_days]'
 
 import os
-import logging
 import sys
 import datetime
 
 import json
 import boto
 from boto.s3.connection import OrdinaryCallingFormat
-from archive import CloudStorage, main, Status
-
-log = logging.getLogger()
+from archive import CloudStorage, main, initialise_logger, Status
 
 
 class S3CloudStorage(CloudStorage):
@@ -220,4 +217,5 @@ class S3CloudStorage(CloudStorage):
 
 
 if __name__ == "__main__":
+    log = initialise_logger()
     sys.exit(main(S3CloudStorage))
diff --git a/files/archive-to-swift.py b/files/archive-to-swift.py
index ac83a25..535c7f5 100755
--- a/files/archive-to-swift.py
+++ b/files/archive-to-swift.py
@@ -16,15 +16,12 @@
 # 'archive-to-swift.py -s -f [config_file] -l [logdir1] -l [logdir2] -c [container_name] -a [archive_after] -d [delete_after] -p [retention_days]'
 
 import os
-import logging
 import sys
 import datetime
 
 import json
 import swiftclient
-from archive import CloudStorage, main, Status
-
-log = logging.getLogger()
+from archive import CloudStorage, main, initialise_logger, Status
 
 
 class SwiftCloudStorage(CloudStorage):
@@ -242,4 +239,5 @@ class SwiftCloudStorage(CloudStorage):
 
 
 if __name__ == "__main__":
+    log = initialise_logger()
     sys.exit(main(SwiftCloudStorage))
diff --git a/files/archive.py b/files/archive.py
index 7ffd4e0..e47b524 100644
--- a/files/archive.py
+++ b/files/archive.py
@@ -22,7 +22,18 @@ import time
 
 from preprocessor import get_preprocessor
 
-log = logging.getLogger()
+
+def initialise_logger():
+    lg = logging.getLogger()
+    lg.setLevel(logging.DEBUG)
+    fh = logging.FileHandler('/var/log/container-log-archive.log')
+    ch = logging.StreamHandler()
+    lg.addHandler(fh)
+    lg.addHandler(ch)
+    return lg
+
+
+log = initialise_logger()
 
 
 class Status:
diff --git a/files/container-log-archive.logrotate b/files/container-log-archive.logrotate
new file mode 100644
index 0000000..5dd38b0
--- /dev/null
+++ b/files/container-log-archive.logrotate
@@ -0,0 +1,12 @@
+#-------------------------------------------------#
+# This file is Juju managed - do not edit by hand #
+#-------------------------------------------------#
+/var/log/container-log-archive.log {
+    rotate 7
+    daily
+    dateext
+    compress
+    notifempty
+    missingok
+    create 0644 root root
+}
diff --git a/hooks/hooks.py b/hooks/hooks.py
index 45b5254..0d36c77 100755
--- a/hooks/hooks.py
+++ b/hooks/hooks.py
@@ -8,6 +8,7 @@ import random
 import os
 import stat
 import os.path
+import shutil
 import socket
 import time
 sys.path.insert(0, os.path.join(os.environ['CHARM_DIR'], 'lib'))
@@ -261,6 +262,17 @@ def deconfigure_nrpe():
     service_reload("nagios-nrpe-server")
 
 
+def configure_logrotate():
+    logrotate_etcdir = "/etc/logrotate.d"
+    logrotate_file = "files/container-log-archive.logrotate"
+    logrotate_path = os.path.join(logrotate_etcdir, "container-log-archive")
+    try:
+        shutil.copyfile(logrotate_file, logrotate_path)
+    except OSError as e:
+        log("CHARM: Failed to copy logrotate file {} to {}".format(logrotate_file, logrotate_path))
+        raise(e)
+
+
 def remove_legacy_files():
     """ Previous versions of the charm didn't use the service_name in the
         various system files. This removes those older files if they exist
@@ -305,6 +317,7 @@ def config_changed():
         configure_container_creds()
         configure_sudo()
         configure_cronjob()
+    configure_logrotate()
     configure_nrpe()
     remove_legacy_files()