Issue #507 Implement cleanup feature for macos

[praveenkumar]

Fixes: Issue #507

Solution/Idea

Implement the cleanup for macos so user can run crc cleanup and it removes the file which created by setup command including the tray.

Proposed changes

List main as well as consequential changes you introduced or had to introduce.

1. Add cleanup command.

Testing

Run crc cleanup command and it should remove the files and change the file permission back to original user which created by crc setup command.

[anjannath]

LGTM

[anjannath]

Not related to this PR but to cleanup feature, i think the cleanups should also do the checks, if we perform the checks in doPreflightCleanUp and skip over the ones that fails, we'll only do cleanup for the needed ones, now even when tray is not installed, it tries to unload and remove it.

[praveenkumar]

Not related to this PR but to cleanup feature, i think the cleanups should also do the checks, if we perform the checks in doPreflightCleanUp and skip over the ones that fails, we'll only do cleanup for the needed ones, now even when tray is not installed, it tries to unload and remove it.

@anjannath that is by design, we (me and @cfergeau ) had same conversation when implementing it for linux, we don't care if something is present or not but our cleanup stuff should run for each steps even a skip one.

[cfergeau]

Actually, @anjannath makes a good point. We discussed ignoring the skip- options when doing the cleanups. However, the cleanups functions have some code to check if the user setup seems to be in a state which was set by crc. Why not use the check() methods which we already have for that?

diff --git a/pkg/crc/preflight/preflight.go b/pkg/crc/preflight/preflight.go
index e817e873..523baa56 100644
--- a/pkg/crc/preflight/preflight.go
+++ b/pkg/crc/preflight/preflight.go
@@ -102,6 +102,11 @@ func (check *PreflightCheck) doCleanUp() error {
 	}
 
 	logging.Infof("%s", check.cleanupDescription)
+	err := check.check()
+	if err != nil {
+		logging.Debug("User setup does not match what crc expect, not trying to cleanup")
+		return nil
+	}
 
 	return check.cleanup()
 }
diff --git a/pkg/crc/preflight/preflight_checks_linux.go b/pkg/crc/preflight/preflight_checks_linux.go
index 286e1854..db408b66 100644
--- a/pkg/crc/preflight/preflight_checks_linux.go
+++ b/pkg/crc/preflight/preflight_checks_linux.go
@@ -364,12 +364,6 @@ func fixLibvirtCrcNetworkAvailable() error {
 
 func removeLibvirtCrcNetwork() error {
 	logging.Debug("Removing libvirt 'crc' network")
-	_, _, err := crcos.RunWithDefaultLocale("virsh", "--connect", "qemu:///system", "net-info", libvirt.DefaultNetwork)
-	if err != nil {
-		// Ignore if no crc network exists for libvirt
-		// User may have manually deleted the `crc` network from libvirt
-		return nil
-	}
 	_, stderr, err := crcos.RunWithDefaultLocale("virsh", "--connect", "qemu:///system", "net-destroy", libvirt.DefaultNetwork)
 	if err != nil {
 		logging.Debugf("%v : %s", err, stderr)
@@ -523,27 +517,19 @@ func fixCrcDnsmasqConfigFile() error {
 }
 
 func removeCrcDnsmasqConfigFile() error {
-	if err := checkNetworkManagerInstalled(); err != nil {
-		// When NetworkManager is not installed, this file won't exist
-		return nil
+	logging.Debug("Removing dnsmasq configuration")
+	err := crcos.RemoveFileAsRoot(
+		fmt.Sprintf("removing dnsmasq configuration in %s", crcDnsmasqConfigPath),
+		crcDnsmasqConfigPath,
+	)
+	if err != nil {
+		return fmt.Errorf("Failed to remove dnsmasq config file: %s: %v", crcDnsmasqConfigPath, err)
 	}
-	// Delete the `crcDnsmasqConfigPath` file if exists,
-	// ignore all the os PathError except `IsNotExist` one.
-	if _, err := os.Stat(crcDnsmasqConfigPath); !os.IsNotExist(err) {
-		logging.Debug("Removing dnsmasq configuration")
-		err := crcos.RemoveFileAsRoot(
-			fmt.Sprintf("removing dnsmasq configuration in %s", crcDnsmasqConfigPath),
-			crcDnsmasqConfigPath,
-		)
-		if err != nil {
-			return fmt.Errorf("Failed to remove dnsmasq config file: %s: %v", crcDnsmasqConfigPath, err)
-		}
 
-		logging.Debug("Reloading NetworkManager")
-		sd := systemd.NewHostSystemdCommander()
-		if _, err := sd.Reload("NetworkManager"); err != nil {
-			return fmt.Errorf("Failed to restart NetworkManager: %v", err)
-		}
+	logging.Debug("Reloading NetworkManager")
+	sd := systemd.NewHostSystemdCommander()
+	if _, err := sd.Reload("NetworkManager"); err != nil {
+		return fmt.Errorf("Failed to restart NetworkManager: %v", err)
 	}
 	return nil
 }

[cfergeau]

Unloading?

[cfergeau]

This is consistent with the other messages, but maybe they should all be changed to CodeReady Containers daemon

[cfergeau]

the flag is named 'experimental'.
I guess we could name it in full with the --enable-experimental-features flag

[cfergeau]

cleanup does not have an 'experimental' flag, does it?