[container_log] Get rotated logs #3678

Akrog · 2024-06-17T10:40:45Z

In Kubernetes/OpenShift rotated logs have no symbolic link in /var/log/containers and they cannot be retrieved using oc logs either, so there is no way to get these rotated logs in a SOS report.

This patch proposes a new functionality for the container_log plugin to retrieve rotated logs for the containers.

Since there may be lots of rotated logs we also add the maxage option.

The plugin will not only download the /var/log/pods files but also the /var/log/container symlinks.

Closes #3677
Signed-off-by: Gorka Eguileor geguileo@redhat.com

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

Is the commit message split over multiple lines and hard-wrapped at 72 characters?
Is the subject and message clear and concise?
Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
Does the commit contain a Signed-off-by: First Lastname email@example.com?
Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
Are all passwords or private data gathered by this PR obfuscated?

packit-as-a-service · 2024-06-17T10:44:05Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/sosreport-sos-3678
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

gibizer

Thanks @Akrog

fmount · 2024-06-17T13:42:34Z

+1 on this change, thank you @Akrog

TurboTurtle

I don't like the idea of adding yet another "get logs for containers" plugin from the start. On top of that though, this should be an addition to the openshift and kubernetes plugins as a simple add_copy_spec() using the existing maxage parameter.

If the rotated logs are written to disk, there's no reason the openshift plugin can't just collect those outside of the oc logs command.

Akrog · 2024-06-17T16:40:23Z

I don't like the idea of adding yet another "get logs for containers" plugin from the start. On top of that though, this should be an addition to the openshift and kubernetes plugins as a simple add_copy_spec() using the existing maxage parameter.

If the rotated logs are written to disk, there's no reason the openshift plugin can't just collect those outside of the oc logs command.

Makes sense not adding yet another plugin.

Would it be ok to add it as an option to the container_log plugin instead?

I'm saying it because that plugin is already getting /var/log/containers which in many cases are just symlinks to /var/log/pods.

bogdando · 2024-06-18T11:38:58Z

well done, thanks a lot!

TurboTurtle · 2024-06-18T14:22:28Z

I don't like the idea of adding yet another "get logs for containers" plugin from the start. On top of that though, this should be an addition to the openshift and kubernetes plugins as a simple add_copy_spec() using the existing maxage parameter.
If the rotated logs are written to disk, there's no reason the openshift plugin can't just collect those outside of the oc logs command.

Would it be ok to add it as an option to the container_log plugin instead?

I'm saying it because that plugin is already getting /var/log/containers which in many cases are just symlinks to /var/log/pods.

Yeah, that makes sense.

jcastill · 2024-06-20T14:21:45Z

@Akrog can you squash these two commits into one please?

Akrog · 2024-06-20T14:49:00Z

@jcastill Done. I usually try to do 1 thing on each patch, but each project is different. :-)

TurboTurtle · 2024-06-20T22:38:49Z

sos/report/plugins/container_log.py

+        # Remove size limit from containers logs when getting rotated, since
+        # they are just symlinks to the pods logs


Our sizelimiting takes the link's destination into account when checking sizes for collections, so I'm not sure what the desire is here? We definitely do not want to have a wide open "collect all logs regardless of sizes" default posture here. We've had that bite us in the rear in the past several times - no one likes several GB sos reports.

I tried, and failed, to explain it in the PR description.

The idea is that if you set the rotated option but you don't change the limits at all you are going to either get the exact same content as without the option (because like you said getting the symlinks checks the actual destination files) or we are going to get logs from the pods that don't have a symlink (not necessarily rotated logs), in both cases it seems wrong.

Another option is for the rotated option to ignore all the .log files from the pods directory and only get rotated logs instead.

This later options sounds reasonable to me and in line with the option name. I'll update the PR.

TurboTurtle · 2024-06-20T22:39:18Z

sos/report/plugins/container_log.py

+    def collect_subdirs(self, root=logdir, sizelimit=None, maxage=None):
        """Collect *.log files from subdirs of passed root path
        """
        for dir_name, _, _ in os.walk(root):
-            self.add_copy_spec(self.path_join(dir_name, '*.log'))
+            self.add_copy_spec(self.path_join(dir_name, '*.log'),
+                               sizelimit=sizelimit,


Yeah, not ignoring sizelimit entirely as a default behavior.

TurboTurtle · 2024-06-20T22:41:09Z

sos/report/plugins/container_log.py

+        PluginOpt('maxage', default=None, val_type=int,
+                  desc='gather only logs with `mtime` not older than this many'
+                  ' hours')


I think if we do anything here, we should add a maxage (or since) -esque options to all plugins by default, like we do for timeout and postproc.

Adding that global feature in this PR doesn't feel right, so I'll submit another PR for that one.

Does this PR look reasonable? #3681

TurboTurtle · 2024-06-23T16:24:21Z

sos/report/plugins/container_log.py

+    option_list = [
+        PluginOpt('rotated', default=False, val_type=bool,
+                  desc='also get rotated logs from /var/log/pods'),
+    ]


Just noticed this on a second pass, but I'm assuming this should be dropped with the intent being that the rotated logs are collected by -k container_log.all-logs=true once #3681 is merged?

I'm stupid, you are right.
This PR is changing existing behavior and it's adding a plugin option that is not being used.
I'll make the rotated log gathering conditional to the rotated plugin option.

In Kubernetes/OpenShift rotated logs have no symbolic link in /var/log/containers and they cannot be retrieved using `oc logs` either, so there is no way to get these rotated logs in a SOS report. This patch proposes extending the `container_log` plugin to make it also capable of retrieving rotated logs from `var/log/pods` using a plugin boolean option called `rotated`. Closes sosreport#3677 Signed-off-by: Gorka Eguileor <geguileo@redhat.com>

Akrog force-pushed the sos_pods branch from 38ac7dc to e4671de Compare June 17, 2024 10:50

gibizer reviewed Jun 17, 2024

View reviewed changes

TurboTurtle requested changes Jun 17, 2024

View reviewed changes

Akrog force-pushed the sos_pods branch from 0ae2aad to 213b056 Compare June 20, 2024 14:13

Akrog changed the title ~~Add new pod_log plugin~~ [container_log] Get rotated logs Jun 20, 2024

Akrog force-pushed the sos_pods branch from 213b056 to 20212ba Compare June 20, 2024 14:48

pmoravec approved these changes Jun 20, 2024

View reviewed changes

TurboTurtle reviewed Jun 20, 2024

View reviewed changes

Akrog force-pushed the sos_pods branch from 20212ba to f23dbba Compare June 21, 2024 12:24

TurboTurtle approved these changes Jun 21, 2024

View reviewed changes

TurboTurtle reviewed Jun 23, 2024

View reviewed changes

Akrog force-pushed the sos_pods branch from f23dbba to 22ed237 Compare June 24, 2024 10:15

TurboTurtle merged commit a79b866 into sosreport:main Jul 4, 2024
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[container_log] Get rotated logs #3678

[container_log] Get rotated logs #3678

Akrog commented Jun 17, 2024 •

edited

Loading

packit-as-a-service bot commented Jun 17, 2024

gibizer left a comment

fmount commented Jun 17, 2024

TurboTurtle left a comment

Akrog commented Jun 17, 2024

bogdando commented Jun 18, 2024

TurboTurtle commented Jun 18, 2024

jcastill commented Jun 20, 2024

Akrog commented Jun 20, 2024

TurboTurtle Jun 20, 2024 •

edited

Loading

Akrog Jun 21, 2024

TurboTurtle Jun 20, 2024

TurboTurtle Jun 20, 2024

Akrog Jun 21, 2024

Akrog Jun 21, 2024

TurboTurtle Jun 23, 2024

Akrog Jun 24, 2024

		# Remove size limit from containers logs when getting rotated, since
		# they are just symlinks to the pods logs

[container_log] Get rotated logs #3678

[container_log] Get rotated logs #3678

Conversation

Akrog commented Jun 17, 2024 • edited Loading

packit-as-a-service bot commented Jun 17, 2024

gibizer left a comment

Choose a reason for hiding this comment

fmount commented Jun 17, 2024

TurboTurtle left a comment

Choose a reason for hiding this comment

Akrog commented Jun 17, 2024

bogdando commented Jun 18, 2024

TurboTurtle commented Jun 18, 2024

jcastill commented Jun 20, 2024

Akrog commented Jun 20, 2024

TurboTurtle Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Akrog Jun 21, 2024

Choose a reason for hiding this comment

TurboTurtle Jun 20, 2024

Choose a reason for hiding this comment

TurboTurtle Jun 20, 2024

Choose a reason for hiding this comment

Akrog Jun 21, 2024

Choose a reason for hiding this comment

Akrog Jun 21, 2024

Choose a reason for hiding this comment

TurboTurtle Jun 23, 2024

Choose a reason for hiding this comment

Akrog Jun 24, 2024

Choose a reason for hiding this comment

Akrog commented Jun 17, 2024 •

edited

Loading

TurboTurtle Jun 20, 2024 •

edited

Loading