S3 Destination #4624

alltilla · 2023-09-05T07:01:53Z

No description provided.

kira-syslogng · 2023-09-05T07:17:15Z

Build FAILURE

kira-syslogng · 2023-09-05T15:07:22Z

Build FAILURE

kira-syslogng · 2023-09-06T16:05:36Z

Build FAILURE

kira-syslogng · 2023-09-07T15:54:32Z

Build FAILURE

kira-syslogng · 2023-09-08T15:21:43Z

Build FAILURE

kira-syslogng · 2023-09-08T16:14:00Z

Build FAILURE

alltilla · 2023-09-11T09:45:16Z

This is now ready for review, we still need the reloc and metric labeling support in python, but the business logic is done.

kira-syslogng · 2023-09-11T10:05:17Z

Build FAILURE

kira-syslogng · 2023-09-11T10:40:33Z

Build FAILURE

OverOrion

I had some minor remarks only, the overall picture is very great, thanks!

We also discussed the compress/compresslevel vs compresslevel only part, and we had agreed on keeping it the current way for easier configuration.

OverOrion · 2023-09-15T09:34:11Z

modules/python-modules/syslogng/modules/s3/compressable_file_buffer.py

+    def __init__(
+        self,
+        path: Path,
+        compress: bool = False,


To me it seems that the compress field serves as a guard for compresslevel only. However as compresslevel is Optional[int], compress seems redundant because setting compression can be done by passing some level or just omitting it if no compression is required. How about removing it?

OverOrion · 2023-09-15T09:45:42Z

modules/python-modules/syslogng/modules/s3/s3_destination.py

+
+
+class S3Destination(LogDestination):
+    S3_OBJECT_TIMEOUT_INTERVAL = 60


[nitpicking]
I think S3_OBJECT_TIMEOUT_INTERVAL_SECONDS might be a bit more suitable name

Thanks, fixed!

OverOrion · 2023-09-15T09:47:14Z

modules/python-modules/syslogng/modules/s3/s3_destination.py

+
+signal(SIGINT, SIG_IGN)
+


This stood out to me for a while, is it there to propagate the signal to syslog-ng?

This should not be needed as we are running python in isolated mode, which means it should not install signal handles

I am still having a problem if I remove that line:

^C[2023-09-19T14:02:51.779809] syslog-ng shutting down; version='4.3.1.174.g93ca97d.dirty' [2023-09-19T14:02:51.975195] Exception while calling a Python function; caller='d_s3#0', class='syslogng.modules.s3.S3Destination', function='deinit', exception='KeyboardInterrupt: ' Traceback (most recent call last): File "/home/alltilla/repos/syslog-ng/build/install/lib/syslog-ng/python/syslogng/modules/s3/s3_destination.py", line 148, in deinit def deinit(self) -> None: KeyboardInterrupt

This happens, when I Ctrl+C a running syslog-ng.

Checked it with bare python and python -I, both of them catch SIGINT :/

$ sudo ps -o pid,caught,cmd -p 143434 PID CAUGHT CMD 143434 0000000008000002 python3 $ sudo ps -o pid,caught,cmd -p 214536 PID CAUGHT CMD 214536 0000000000000002 python3 -I

$ python -I Python 3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> ^C KeyboardInterrupt

OverOrion · 2023-09-15T10:24:10Z

modules/python-modules/syslogng/modules/s3/s3_destination.py

+            self.object_key_timestamp: Optional[LogTemplate] = options["object_key_timestamp"]
+            self.message_template: LogTemplate = options["template"]
+            self.compression = bool(options["compression"])
+            self.compresslevel = int(options["compresslevel"])


I think this should be Optional[int], shouldn't it?

No, setting the defaults for each option is the responsibility of the SCL. If the user uses this class as a python dest, and they mess this up, they will see an error.

except KeyError: assert False, ( f"S3: {str(exc_info()[1])[1:-1]}() option is missing. " "If you are using this driver via the python() destination, please use the s3() driver directly. " "The option validation and propagation should be done by the s3.conf SCL." )

block destination s3( url("") bucket() access_key() secret_key() object_key() object_key_timestamp("") template("${MESSAGE}\n") compression(no) compresslevel(9)

modules/python-modules/syslogng/modules/s3/s3_destination.py

OverOrion · 2023-09-15T10:38:37Z

modules/python-modules/syslogng/modules/s3/s3_destination.py

+                return False
+
+            if retries == 0:
+                self.logger.error("Upload queue full, waiting for available space")


Suggested change

self.logger.error("Upload queue full, waiting for available space")

self.logger.error("Upload queue is full, waiting for available space")

Thanks, fixed!

OverOrion · 2023-09-15T10:48:15Z

modules/python-modules/syslogng/modules/s3/s3_object.py

+
+
+class S3Object:
+    MIN_CHUNK_SIZE = 5 * 1024 * 1024


Suggested change

MIN_CHUNK_SIZE = 5 * 1024 * 1024

MIN_CHUNK_SIZE_BYTES = 5 * 1024 * 1024

Thanks, fixed!

OverOrion · 2023-09-15T10:55:43Z

modules/python-modules/syslogng/modules/s3/scl/s3.conf

+    compression(no)
+    compresslevel(9)


How about removing the compression option and using compresslevel(0) as a way to disable compression?

OverOrion · 2023-09-15T11:17:21Z

modules/python-modules/syslogng/modules/s3/s3_destination.py

+            self.object_key_timestamp: Optional[LogTemplate] = options["object_key_timestamp"]
+            self.message_template: LogTemplate = options["template"]
+            self.compression = bool(options["compression"])
+            self.compresslevel = int(options["compresslevel"])


Could you also please also add some sanity checks regarding its value?

Thanks, fixed!

alltilla · 2023-09-18T11:01:44Z

@OverOrion Thanks for the comments. As we discussed IRL, I have kept compress() and compresslevel() as they were, just added a sanity check.

modules/python-modules/syslogng/modules/s3/compressable_file_buffer.py

modules/python-modules/syslogng/modules/s3/s3_object.py

modules/python-modules/syslogng/modules/s3/s3_destination.py

It is needed for SCLs, where the 2 double-quotes get resolved to nothing. We want to have na empty template in that case. Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>

MrAnno

👍🏻 👍🏻
(please squash the fixups)

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>

Python 3.6 does not support fres boto3 and botocore versions, also compiling type hinted code (which is done for some reason during setup.py) does not seem to work. We tackle these 2 in the following ways: In requirements.txt we allow installing older boto3 and botocore versions. We should not install them at all, but filtering based on the python version would be complex. We skip adding the s3 module to the packages in setup.py for <= 3.6. The only platform we support and has 3.6 is centos-7. We can remove these workarounds when the centos-7 support is dropped (June 2024). Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>

alltilla force-pushed the s3-dest branch from e803348 to 83ef414 Compare September 5, 2023 15:01

alltilla force-pushed the s3-dest branch from 83ef414 to 4bf18d6 Compare September 6, 2023 15:59

alltilla force-pushed the s3-dest branch from 4bf18d6 to 8f1ee39 Compare September 7, 2023 15:32

alltilla force-pushed the s3-dest branch from 8f1ee39 to 8ebaa30 Compare September 8, 2023 15:00

alltilla force-pushed the s3-dest branch from 8ebaa30 to 8bac1f4 Compare September 8, 2023 15:52

alltilla force-pushed the s3-dest branch from 8bac1f4 to 47e4c59 Compare September 11, 2023 09:44

alltilla changed the title ~~s3: add dest~~ S3 Destination Sep 11, 2023

alltilla marked this pull request as ready for review September 11, 2023 09:44

alltilla force-pushed the s3-dest branch 4 times, most recently from bee6759 to 747691f Compare September 11, 2023 10:19

alltilla force-pushed the s3-dest branch 6 times, most recently from c5d4478 to e147ec5 Compare September 13, 2023 14:18

OverOrion self-requested a review September 15, 2023 08:46

OverOrion reviewed Sep 15, 2023

View reviewed changes

alltilla force-pushed the s3-dest branch from e147ec5 to b8d774c Compare September 18, 2023 10:59

alltilla force-pushed the s3-dest branch 2 times, most recently from bbdf4b7 to 5ec14e8 Compare September 18, 2023 11:00

alltilla force-pushed the s3-dest branch from 5ec14e8 to 7344efb Compare September 18, 2023 11:25

alltilla added this to the syslog-ng 4.4 milestone Sep 19, 2023

MrAnno self-requested a review September 20, 2023 10:39

MrAnno reviewed Sep 21, 2023

View reviewed changes

alltilla requested a review from MrAnno September 22, 2023 07:45

alltilla added 2 commits September 22, 2023 10:04

python: support setting empty LogTemplate in options

24be6af

It is needed for SCLs, where the 2 double-quotes get resolved to nothing. We want to have na empty template in that case. Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>

python: add reloc support

64f7b8e

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>

MrAnno approved these changes Sep 22, 2023

View reviewed changes

alltilla added 3 commits September 22, 2023 11:45

s3: add dest

85ef8ff

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>

s3: stop gracefully when python deps are not available

183592d

Signed-off-by: Attila Szakacs <attila.szakacs@axoflow.com>

alltilla force-pushed the s3-dest branch from 66964fc to 183592d Compare September 22, 2023 09:45

MrAnno approved these changes Sep 22, 2023

View reviewed changes

MrAnno merged commit 7a860fa into syslog-ng:master Sep 22, 2023



		class S3Destination(LogDestination):
		S3_OBJECT_TIMEOUT_INTERVAL = 60

	self.logger.error("Upload queue full, waiting for available space")
	self.logger.error("Upload queue is full, waiting for available space")

	MIN_CHUNK_SIZE = 5 * 1024 * 1024
	MIN_CHUNK_SIZE_BYTES = 5 * 1024 * 1024


		signal(SIGINT, SIG_IGN)

S3 Destination #4624

S3 Destination #4624

Uh oh!

Conversation

alltilla commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kira-syslogng commented Sep 5, 2023

Uh oh!

kira-syslogng commented Sep 5, 2023

Uh oh!

kira-syslogng commented Sep 6, 2023

Uh oh!

kira-syslogng commented Sep 7, 2023

Uh oh!

kira-syslogng commented Sep 8, 2023

Uh oh!

kira-syslogng commented Sep 8, 2023

Uh oh!

alltilla commented Sep 11, 2023

Uh oh!

kira-syslogng commented Sep 11, 2023

Uh oh!

kira-syslogng commented Sep 11, 2023

Uh oh!

OverOrion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alltilla commented Sep 18, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MrAnno left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alltilla commented Sep 5, 2023 •

edited

Loading

MrAnno left a comment •

edited

Loading