Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logfile rotation, and delete log files when running crc delete --clear-cache #3720

Merged
merged 4 commits into from
Jul 10, 2023

Conversation

anjannath
Copy link
Member

both the crc.log and crcd.log files will be rotated (backed up by
renaming and creating a new log file) after they grow to 5 MBs in
size

@openshift-ci openshift-ci bot requested review from evidolob and gbraad June 16, 2023 07:32
@gbraad
Copy link
Contributor

gbraad commented Jun 16, 2023

after they grow to 5 MBs in size

You mean this happens continuously ?
The title of the PR suggests differently.

@anjannath anjannath linked an issue Jun 16, 2023 that may be closed by this pull request
@anjannath
Copy link
Member Author

after they grow to 5 MBs in size

You mean this happens continuously ? The title of the PR suggests differently.

yes, this keeps on happening making new backup files and new log files

@gbraad
Copy link
Contributor

gbraad commented Jun 16, 2023

make 'crc delete --clear-cache' remove the logfiles

crc delete is instance specific.
--clear-cache is generic, but handles with .cache
log files does not sound like any of this

I even question the --clear-cache as that might be wrong to be part of delete.

@gbraad gbraad changed the title Add logfile rotation and delete log files when running crc delete --clear-cache Add logfile rotation, and delete log files when running crc delete --clear-cache Jun 16, 2023
@anjannath
Copy link
Member Author

anjannath commented Jun 16, 2023

make 'crc delete --clear-cache' remove the logfiles

crc delete is instance specific. --clear-cache is generic, but handles with .cache log files does not sound like any of this

I even question the --clear-cache as that might be wrong to be part of delete.

yeah, its not very intuitive, but since we always had a --clear-cache flag for delete i think our users are used to this CLI

but deleting the log files also with the --clear-cache is something i am looking for feedback

with the log file rotation added and the number of backups also configurable (set to 10 backups in this PR) the deleting of log files might not be needed at all.

@gbraad
Copy link
Contributor

gbraad commented Jul 5, 2023

but since we always had a --clear-cache flag for delete i think our users are used to this CLI

i doubt many users inow about this. i see many users with gigs stored in their cache. what does telemetry show?

I'd rather go for the more intuitive option

@anjannath
Copy link
Member Author

anjannath commented Jul 5, 2023

but since we always had a --clear-cache flag for delete i think our users are used to this CLI

i doubt many users inow about this. i see many users with gigs stored in their cache. what does telemetry show?

I'd rather go for the more intuitive option

you mean, not remove the log files with any command and do it automatically, i.e drop this commit b1b06ac from the PR ?

@gbraad or should we remove the need to supply --clear-cache flag, and delete log files with just crc delete ?

@praveenkumar
Copy link
Member

with the log file rotation added and the number of backups also configurable (set to 10 backups in this PR) the deleting of log files might not be needed at all.

why we need 10 backups? shouldn't we just OK with 2 or even single one, as a user I check those logs file only when I hit an error to understand what I have as part of debug and share it during issue creation apart from that I might not care about this log file or is there any other use case of these backups?

@anjannath
Copy link
Member Author

with the log file rotation added and the number of backups also configurable (set to 10 backups in this PR) the deleting of log files might not be needed at all.

why we need 10 backups? shouldn't we just OK with 2 or even single one, as a user I check those logs file only when I hit an error to understand what I have as part of debug and share it during issue creation apart from that I might not care about this log file or is there any other use case of these backups?

i added 10 backups, thinking 5mb limit will be reached soon because of all the status calls to the daemon and we might lose logs, but only 2 should be sufficient and we can always change this if we discover we're losing some logs, update the PR!

@praveenkumar
Copy link
Member

@anjannath looks like during the update you added the other commits also?

the `crc delete --clear-cache` command will now also remove the
crc.log and crcd.log files
both the crc.log and crcd.log files will be rotated (backed up by
renaming and creating a new log file) after they grow to 5 MBs in
size
@anjannath
Copy link
Member Author

@praveenkumar yeah, i rebased on the other PR branch it seems, was supposed to rebase on main, updated again, thanks!

@praveenkumar
Copy link
Member

/lgtm
/approve

@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: praveenkumar

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:

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

@openshift-ci openshift-ci bot added the approved label Jul 7, 2023
@praveenkumar
Copy link
Member

/retest

@anjannath
Copy link
Member Author

/test e2e-crc

@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2023

@anjannath: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-crc a3ca19e link true /test e2e-crc

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@anjannath
Copy link
Member Author

anjannath commented Jul 10, 2023

unrelated to the PR, but e2e test's Installing the Redis Operator in openshift_story is failing consistently in other PRs as well, we might need to update the test

https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/crc-org_crc/3720/pull-ci-crc-org-crc-main-e2e-crc/1677269032417366016

from another PR https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/crc-org_crc/3748/pull-ci-crc-org-crc-main-e2e-crc/1676523231856562176

@adrianriobo
Copy link
Contributor

This is a known issue already reported on the community operators project redhat-openshift-ecosystem/community-operators-prod#2865

@praveenkumar praveenkumar merged commit 01cb0a6 into crc-org:main Jul 10, 2023
anjannath added a commit to anjannath/crc that referenced this pull request Jul 11, 2023
currently after crc-org#3720 was merged it is only deleting the
'crc.log' and 'crcd.log' files and not deleting the backups
anjannath added a commit to anjannath/crc that referenced this pull request Jul 20, 2023
currently after crc-org#3720 was merged it is deleting the
'crc.log' and 'crcd.log' files and not deleting the backups

this adds deletion of the backed up log files named crc-*.log
and removes deletion of the crc.log file which is handled  by
'crc cleanup'

it removes the deletion of crcd.log file as when delete is
performed the daemon still keeps running in the background
anjannath added a commit to anjannath/crc that referenced this pull request Jul 24, 2023
currently after crc-org#3720 was merged it is deleting the
'crc.log' and 'crcd.log' files and not deleting the backups

this adds deletion of the backed up log files named crc-*.log
and removes deletion of the crc.log file which is handled  by
'crc cleanup'

it removes the deletion of crcd.log file as when delete is
performed the daemon still keeps running in the background
anjannath added a commit to anjannath/crc that referenced this pull request Jul 25, 2023
currently after crc-org#3720 was merged it is deleting the
'crc.log' and 'crcd.log' files and not deleting the backups

this adds deletion of the backed up log files named crc-*.log
and removes deletion of the crc.log file which is handled  by
'crc cleanup'

it removes the deletion of crcd.log file as when delete is
performed the daemon still keeps running in the background
anjannath added a commit to anjannath/crc that referenced this pull request Jul 31, 2023
currently after crc-org#3720 was merged it is deleting the
'crc.log' and 'crcd.log' files and not deleting the backups

this adds deletion of the backed up log files named crc-*.log
and removes deletion of the crc.log file which is handled  by
'crc cleanup'

it removes the deletion of crcd.log file as when delete is
performed the daemon still keeps running in the background
praveenkumar pushed a commit that referenced this pull request Aug 8, 2023
currently after #3720 was merged it is deleting the
'crc.log' and 'crcd.log' files and not deleting the backups

this adds deletion of the backed up log files named crc-*.log
and removes deletion of the crc.log file which is handled  by
'crc cleanup'

it removes the deletion of crcd.log file as when delete is
performed the daemon still keeps running in the background
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Add cache clean and log file removal functionality
5 participants