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

reset command improvements #959

Merged
merged 1 commit into from
Jun 14, 2021

Conversation

jasmingacic
Copy link
Contributor

Fixes part of the issue #928.
Reset command will remove service stubs regardless of the role installed.
When running reset command without k0s binaries already being unpacked in the data directory reset command is not able to detect which role has already been installed. So reset will now try to remove both service stubs k0scontroller and k0sworker.

Signed-off-by: Jasmin Gacic jgacic@mirantis.com

@kke
Copy link
Contributor

kke commented Jun 7, 2021

The behavior is a bit sketchy.

First it gets the role:

	role := install.GetRoleByStagedKubelet(c.K0sVars.BinDir)

This will return "worker" if no installation was detected.

Then it proceeds to uninstall both controller and worker:

	err := install.UninstallService("controller")
	if err != nil {  ...	}
	err = install.UninstallService("worker")
	if err != nil {  ...	}

I assume one of them will always report the logger.Infof("failed to uninstall k0s service: %v", err) in the err check block, as both of them will always run.

Finally it will proceed to do cleanup based on the install.GetRoleByStagedKubelet from above, regardless of which Uninstall succeeded. In case of controller+worker both cleanups will run in series.

I think the best would be to fix the GetRoleByStagedKubelet to only return "worker" if it somehow can validate that a worker config exists and use install.InstalledService to figure out if a service was installed.

@jasmingacic
Copy link
Contributor Author

@kke GetRoleByStagedKubelet relies on existence of kubelet or api binary on the host in case that neither exist it will return empty role. So in case that the install failed due to existence of service file and no binaries have been before extracted reset won't be able to detect the role and therefore to remove service stubs which are named k0scontroller and k0sworker.
So this is a crude solution for only one part of the issue we are seeing.

@jasmingacic jasmingacic force-pushed the reset_improvement branch 3 times, most recently from 5c78b71 to de25ffe Compare June 8, 2021 10:56
@kke
Copy link
Contributor

kke commented Jun 9, 2021

@kke GetRoleByStagedKubelet relies on existence of kubelet or api binary on the host in case that neither exist it will return empty role. So in case that the install failed due to existence of service file and no binaries have been before extracted reset won't be able to detect the role and therefore to remove service stubs which are named k0scontroller and k0sworker.
So this is a crude solution for only one part of the issue we are seeing.

// This function attempts to find out the host role, by staged binaries
func GetRoleByStagedKubelet(binPath string) string {
apiBinary := fmt.Sprintf("%s/%s", binPath, "kube-apiserver")
kubeletBinary := fmt.Sprintf("%s/%s", binPath, "kubelet")
if util.FileExists(apiBinary) && util.FileExists(kubeletBinary) {
return "controller+worker"
} else if util.FileExists(apiBinary) {
return "controller"
} else {
return "worker"
}
}

seems to me, it will never return an empty role, but always fall back to "worker".

@kke kke mentioned this pull request Jun 9, 2021
@jasmingacic
Copy link
Contributor Author

Yeah returning "worker" when binaries are not present caused reset to behave weird.

Copy link
Collaborator

@jnummelin jnummelin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some specific comments.

There's bunch of funcs that are exported but lacks proper comments.

In general this is pretty good, I like the added structure and robustness for the reset/cleanup functionality.

cmd/backup/backup.go Outdated Show resolved Hide resolved
cmd/reset/reset.go Outdated Show resolved Hide resolved
pkg/cleanup/cleanup_unix.go Show resolved Hide resolved
pkg/cleanup/services.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@jnummelin jnummelin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit about the output:

/ # ./k0s reset
*  containerd steps
INFO[2021-06-11 07:15:58] successfully removed k0s containers!         
INFO[2021-06-11 07:15:58] no config file given, using defaults         
*  remove directories step
WARN[2021-06-11 07:15:58] failed to unmount /var/lib/k0s               
*  CNI leftovers cleanup step
k0s cleanup operations done. To ensure a full reset, a node reboot is recommended.
Error: errors received during clean-up: [failed to delete /var/lib/k0s. err: unlinkat /var/lib/k0s: device or resource busy]
2021-06-11 07:15:58.173736 I | errors received during clean-up: [failed to delete /var/lib/k0s. err: unlinkat /var/lib/k0s: device or resource busy]

The output is now coming from both fmt.Println and through logrus which makes it bit "messy" IMO. I'd rather have all output through logrus.

…irectories - remove all k0s directories, services - removes service stubs, users - removes controller users, cni - removes CNI leftovers

Signed-off-by: Jasmin Gacic <jgacic@mirantis.com>
@kke
Copy link
Contributor

kke commented Jun 11, 2021

The output is now coming from both fmt.Println and through logrus which makes it bit "messy" IMO. I'd rather have all output through logrus.

There's also direct output from the commands started via os.Exec as the stdout and stderr are piped directly.

@jasmingacic jasmingacic merged commit 02af676 into k0sproject:main Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants