Repository: code-ready/crc
Pull request #1758 (2020-12-07 13:16:23)

Backup log file as part of delete command
crc log file is opened as append mode and never gets deleted until
user manually delete it or remove the base directory. Currently it
is filled with lot of message which might not be require to debug
an issue. This patch make sure every new crc cycle (setup => start => delete)
have required logs to logfile.

## Testing
- Run crc start => check the log file exist in `~/. crc/crc.log`
- Run crc delete => check the log file shouldn't present

Any other command shouldn't delete this file.


openshift-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/1758#" title="Author self-approved">praveenkumar</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=code-ready%2Fcrc).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/code-ready/crc/blob/master/OWNERS)~~ [praveenkumar]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
anjannath: Maybe this was discussed, but why are we not doing some kind of log rotation instead of deleting the log file completely, we often suggest to do `crc delete` => `crc setup` to fix some issues, in that case won't the old logs be gone?
cfergeau: > Maybe this was discussed, but why are we not doing some kind of log rotation instead of deleting the log file completely, we often suggest to do `crc delete` => `crc setup` to fix some issues, in that case won't the old logs be gone?

Not a big fan of log rotation for our usecase, what if the latest log does not have all the info that we need? Why would 6 months old logs still be relevant? Why reinvent the wheel (log rotation) rather than integrate with pre-existing system solutions (journald, windows equivalent, ...)?
cfergeau: > > Maybe this was discussed, but why are we not doing some kind of log rotation instead of deleting the log file completely, we often suggest to do `crc delete` => `crc setup` to fix some issues, in that case won't the old logs be gone?
> 
> Not a big fan of log rotation for our usecase, what if the latest log does not have all the info that we need? Why would 6 months old logs still be relevant? Why reinvent the wheel (log rotation) rather than integrate with pre-existing system solutions (journald, windows equivalent, ...)?

Actually, I might have misunderstood your suggestion, renaming the log file on `crc delete` and creating a new one for subsequent `crc` runs could indeed be a good alternative. We could remove all logs at `crc cleanup` time to avoid keeping them indefinitely.
anjannath: > Actually, I might have misunderstood your suggestion, renaming the log file on crc delete and creating a new one for subsequent crc runs could indeed be a good alternative. We could remove all logs at crc cleanup time to avoid keeping them indefinitely.

I was thinking of what is done in #1777 it also deletes old log files
guillaumerose: We can go with this but it would be good in the future to have a proper struct for log functions (instead of package function).

/lgtm
openshift-ci-robot: New changes are detected. LGTM label has been removed.

Patch:
diff --git a/pkg/crc/logging/logging.go b/pkg/crc/logging/logging.go
index d4febe9c4..5c368bc59 100644
--- a/pkg/crc/logging/logging.go
+++ b/pkg/crc/logging/logging.go
@@ -1,7 +1,9 @@
 package logging
 
 import (
+	"fmt"
 	"os"
+	"time"
 
 	"github.com/code-ready/crc/pkg/crc/constants"
 	"github.com/sirupsen/logrus"
@@ -19,11 +21,11 @@ func OpenLogFile(path string) (*os.File, error) {
 	if err != nil {
 		return nil, err
 	}
-	l, err := os.OpenFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0600)
+	logfile, err = os.OpenFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0600)
 	if err != nil {
 		return nil, err
 	}
-	return l, nil
+	return logfile, nil
 }
 
 func CloseLogFile() {
@@ -35,13 +37,18 @@ func CloseLogging() {
 	logrus.StandardLogger().ReplaceHooks(make(logrus.LevelHooks))
 }
 
+func BackupLogFile() {
+	os.Rename(logfile.Name(), fmt.Sprintf("%s_%s", logfile.Name(), time.Now().Format("20060102150405"))) // nolint
+}
+
 func InitLogrus(logLevel, logFilePath string) {
-	logFile, err := OpenLogFile(logFilePath)
+	var err error
+	logfile, err = OpenLogFile(logFilePath)
 	if err != nil {
 		logrus.Fatal("Unable to open log file: ", err)
 	}
 	// send logs to file
-	logrus.SetOutput(logFile)
+	logrus.SetOutput(logfile)
 
 	logrus.SetLevel(logrus.TraceLevel)
 
diff --git a/pkg/crc/machine/delete.go b/pkg/crc/machine/delete.go
index 9388f4f20..30c7543f9 100644
--- a/pkg/crc/machine/delete.go
+++ b/pkg/crc/machine/delete.go
@@ -1,9 +1,13 @@
 package machine
 
-import "github.com/pkg/errors"
+import (
+	"github.com/code-ready/crc/pkg/crc/logging"
+	"github.com/pkg/errors"
+)
 
 func (client *client) Delete() error {
 	libMachineAPIClient, cleanup, err := createLibMachineClient(false)
+	defer logging.BackupLogFile()
 	defer cleanup()
 	if err != nil {
 		return errors.Wrap(err, "Cannot initialize libmachine")
diff --git a/pkg/crc/preflight/preflight_checks_common.go b/pkg/crc/preflight/preflight_checks_common.go
index 76db469f1..69f192f30 100644
--- a/pkg/crc/preflight/preflight_checks_common.go
+++ b/pkg/crc/preflight/preflight_checks_common.go
@@ -52,6 +52,11 @@ var genericPreflightChecks = [...]Check{
 		cleanup:            removeCRCMachinesDir,
 		flags:              CleanUpOnly,
 	},
+	{
+		cleanupDescription: "Removing older logs",
+		cleanup:            removeOldLogs,
+		flags:              CleanUpOnly,
+	},
 }
 
 func checkBundleExtracted() error {
@@ -131,3 +136,17 @@ func removeCRCMachinesDir() error {
 	}
 	return nil
 }
+
+func removeOldLogs() error {
+	logFiles, err := filepath.Glob(filepath.Join(constants.CrcBaseDir, "*.log_*"))
+	if err != nil {
+		return fmt.Errorf("Failed to get old logs: %w", err)
+	}
+	for _, f := range logFiles {
+		logging.Debugf("Deleting %s log file", f)
+		if err := os.RemoveAll(f); err != nil {
+			return fmt.Errorf("Failed to delete %s: %w", f, err)
+		}
+	}
+	return nil
+}


Test patch:
diff --git a/pkg/crc/preflight/preflight_darwin_test.go b/pkg/crc/preflight/preflight_darwin_test.go
index 5f94d8c75..19bc178b1 100644
--- a/pkg/crc/preflight/preflight_darwin_test.go
+++ b/pkg/crc/preflight/preflight_darwin_test.go
@@ -16,9 +16,9 @@ func TestCountConfigurationOptions(t *testing.T) {
 }
 
 func TestCountPreflights(t *testing.T) {
-	assert.Len(t, getPreflightChecks(false, network.DefaultMode), 12)
-	assert.Len(t, getPreflightChecks(true, network.DefaultMode), 18)
+	assert.Len(t, getPreflightChecks(false, network.DefaultMode), 13)
+	assert.Len(t, getPreflightChecks(true, network.DefaultMode), 19)
 
-	assert.Len(t, getPreflightChecks(false, network.VSockMode), 11)
-	assert.Len(t, getPreflightChecks(true, network.VSockMode), 17)
+	assert.Len(t, getPreflightChecks(false, network.VSockMode), 12)
+	assert.Len(t, getPreflightChecks(true, network.VSockMode), 18)
 }
diff --git a/pkg/crc/preflight/preflight_linux_test.go b/pkg/crc/preflight/preflight_linux_test.go
index 3b9e22649..393afeee0 100644
--- a/pkg/crc/preflight/preflight_linux_test.go
+++ b/pkg/crc/preflight/preflight_linux_test.go
@@ -66,6 +66,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
+			{cleanup: removeOldLogs},
 			{check: checkIfRunningAsNormalUser},
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkVirtualizationEnabled},
@@ -95,6 +96,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
+			{cleanup: removeOldLogs},
 			{check: checkIfRunningAsNormalUser},
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkVirtualizationEnabled},
@@ -117,6 +119,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
+			{cleanup: removeOldLogs},
 			{check: checkIfRunningAsNormalUser},
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkVirtualizationEnabled},
@@ -145,6 +148,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
+			{cleanup: removeOldLogs},
 			{check: checkIfRunningAsNormalUser},
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkVirtualizationEnabled},
@@ -167,6 +171,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
+			{cleanup: removeOldLogs},
 			{check: checkIfRunningAsNormalUser},
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkVirtualizationEnabled},
@@ -195,6 +200,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
+			{cleanup: removeOldLogs},
 			{check: checkIfRunningAsNormalUser},
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkVirtualizationEnabled},
@@ -217,6 +223,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
+			{cleanup: removeOldLogs},
 			{check: checkIfRunningAsNormalUser},
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkVirtualizationEnabled},
@@ -247,6 +254,7 @@ var checkListForDistros = []checkListForDistro{
 			{check: checkBundleExtracted},
 			{configKeySuffix: "check-ram"},
 			{cleanup: removeCRCMachinesDir},
+			{cleanup: removeOldLogs},
 			{check: checkIfRunningAsNormalUser},
 			{cleanup: adminhelper.CleanHostsFile},
 			{check: checkVirtualizationEnabled},
diff --git a/pkg/crc/preflight/preflight_windows_test.go b/pkg/crc/preflight/preflight_windows_test.go
index ac08d4bda..5b7650ae7 100644
--- a/pkg/crc/preflight/preflight_windows_test.go
+++ b/pkg/crc/preflight/preflight_windows_test.go
@@ -15,9 +15,9 @@ func TestCountConfigurationOptions(t *testing.T) {
 }
 
 func TestCountPreflights(t *testing.T) {
-	assert.Len(t, getPreflightChecks(false, network.DefaultMode), 14)
-	assert.Len(t, getPreflightChecks(true, network.DefaultMode), 16)
+	assert.Len(t, getPreflightChecks(false, network.DefaultMode), 15)
+	assert.Len(t, getPreflightChecks(true, network.DefaultMode), 17)
 
-	assert.Len(t, getPreflightChecks(false, network.VSockMode), 15)
-	assert.Len(t, getPreflightChecks(true, network.VSockMode), 17)
+	assert.Len(t, getPreflightChecks(false, network.VSockMode), 16)
+	assert.Len(t, getPreflightChecks(true, network.VSockMode), 18)
 }