1197: crictl rm to also remove container log files

[fformica]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Container log files are not deleted by the runtime, but the user might want to remove them alongside the container.
This PR changes the default behaviour to removing the logs and introduces a new option to the rm command to preserve the container log file and its rotations.
Failing to delete files doesn't fail the command and is only logged.

Which issue(s) this PR fixes:

Fixes #1197

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Changed default behaviour in `rm` command to also delete the container's log file.
Added `--keeplogs, -k` option to the `rm` command to preserve logs.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: fformica / name: Ferdinando Formica (67679ee, 949545e, 5afb4c7, 09b9303, 64e373b)

[k8s-ci-robot]

Welcome @fformica!

It looks like this is your first PR to kubernetes-sigs/cri-tools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cri-tools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[kwilczynski]

@fformica, thank you for taking this on! Appreciated.

However, we can have a different spin on the problem at hand.

Generally, most of the time, users want to get everything (all the created artefacts) removed once the container is deleted, including the log files.

This should be the default behaviour, and it would follow the principle of least surprise here.

Now, the different spin I mentioned, would be now having an option to not delete logs, if requested. This would allow users to preserve log files should these be needed for something.

So we would have crictl rm --keep-logs or perhaps crictl rm --preseve-logs instead here.

What do you think?

@saschagrunert, thoughts?

[saschagrunert]

I agree having it the other way around makes more sense. We just have to double check that we do not break anything in CI, and a highlighted release note that this is now the default behavior should be included.

[fformica]

Thanks for the feedback!
Side note, it looks like containerd currently doesn't set logPath, so I'll test that a bit (#1197 was observed on OpenShift).

[saschagrunert]

@fformica do we need to change / enhance docs for this?

[fformica]

The crictl documentation doesn't explore the options for every command, so I don't think it's necessary to document this separately; I'm not sure though, so let me know if there's anything you think should be updated.

[saschagrunert]

Is this save for the log rotation? We would also possible remove files which are not rotated logs but other files, right?

[fformica]

It's possible, although the rotations attach to the file name so it's unlikely, e.g. this was from my test with cri-o:

-rw------- 1 root root 5.3M Feb 26 05:49 0.log
-rw-r--r-- 1 root root 1.2M Feb 26 05:44 0.log.20240226-053352.gz
-rw------- 1 root root  11M Feb 26 05:44 0.log.20240226-054404

Another container would be e.g. 1.log, but a user could have backed up a 0.log.bak, and this code would delete that too. I wasn't sure if the log rotation always follows this format though.
Reading https://kubernetes.io/docs/concepts/cluster-administration/logging/ the rotation in /var/log should only be performed by kubelet, so I could use that as a guide, removing files in this format in that directory and ignoring rotations outside it.

[fformica]

It looks like kubelet uses a very similar pattern: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/logs/container_log_manager.go#L312
For the time being I'll do the same and just add the missing dot; I'm averse to adding hardcoded references to the dir, especially because it seems a bit more variable under Windows (the doc mentions C:\var\logs and C:\var\log\pods).

[kwilczynski]

/approve
/lgtm

[k8s-ci-robot]

@kwilczynski: changing LGTM is restricted to collaborators

In response to this:

/approve
/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fformica]

Sorry 😭 I only just noticed that by adding the dot I excluded the current log file...

[kwilczynski]

Sorry 😭 I only just noticed that by adding the dot I excluded the current log file...

No worries. 👍

Might be good to do some manual testing to confirm things work as expected.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fformica, kwilczynski, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment