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

Cannot see finch VM status #23

Closed
shaonm opened this issue Nov 22, 2022 · 7 comments · Fixed by #83
Closed

Cannot see finch VM status #23

shaonm opened this issue Nov 22, 2022 · 7 comments · Fixed by #83
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@shaonm
Copy link

shaonm commented Nov 22, 2022

What is the problem you're trying to solve?.
I want to check the status of the VM that finch has started.

Describe the feature you'd like
finch vm should have a status command to see the vm status.

Usage:
  finch vm [command]

Available Commands:
  init        Initialize the virtual machine
  remove      Remove the virtual machine instance
  start       Start the virtual machine
  stop        Stop the virtual machine

Add status command,
status Status of the virtual machine

@ningziwen ningziwen added the enhancement New feature or request label Nov 22, 2022
@AkihiroSuda
Copy link

Workaround:

$ LIMA_HOME=/Applications/Finch/lima/data /Applications/Finch/lima/bin/limactl ls
NAME     STATUS     SSH                ARCH      CPUS    MEMORY    DISK      DIR
finch    Running    127.0.0.1:56835    x86_64    2       8GiB      100GiB    /Applications/Finch/lima/data/finch

@jlbutler jlbutler added the good first issue Good for newcomers label Nov 26, 2022
@niklasmtj
Copy link
Contributor

Hey,
I would like to help out with this one and started implementing it over in a fork.

One thing that I found which should be handled in a different way than with printing the output via sva.logger.Infof("%s", logs) is the output of lima's ls command. Because this way it is formatted in a wrong way as you can see below:

INFO[0000] NAME     STATUS     SSH                ARCH       CPUS    MEMORY    DISK      DIR
finch    Running    127.0.0.1:51977    aarch64    2       4GiB      100GiB    /Users/nm/Code/oss/finch/_output/lima/data/finch

Is there a custom logger available to print it in a right way? Or just use the regular fmt.Printf?

Since I'm pretty new to Golang I could also need a little help with testing the new command. So I am really open for help and advice.

@ningziwen
Copy link
Member

ningziwen commented Nov 28, 2022

@niklasmtj Thanks! There is an existing internal method to get the lima VM status. Maybe the new command can wrap this internal method and do the proper translation.

You can add new e2e tests here and run "make test-e2e" to test it.

Assigned the issue to you and I could provide you the help if needed. Feel free to use this issue or #finch channel in CNCF server to raise the questions.

@niklasmtj
Copy link
Contributor

niklasmtj commented Nov 29, 2022

Thanks for assigning me. It seems that I did not understand correctly at the beginning of this issue. Since I thought this status command should log all the information about the vm, more on this at the end.

Anyway, I have now used the internal lima.GetVMStatus method to translate the different vmStatus appropriately. As follows:

func (sva *statusVMAction) run() error {
	status, err := lima.GetVMStatus(sva.creator, sva.logger, limaInstanceName)
	if err != nil {
		return err
	}
	switch status {
	case lima.Running:
		sva.logger.Infof("the instance %q is running", limaInstanceName)
		return nil
	case lima.Nonexistent:
		return fmt.Errorf("the instance %q does not exist", limaInstanceName)
	case lima.Stopped:
		return fmt.Errorf("the instance %q is stopped. run `finch %s start` to start the instance", limaInstanceName, virtualMachineRootCmd)
	case lima.Unknown:
		return fmt.Errorf("the instance status of %q is unknown", limaInstanceName)
	default:
		return fmt.Errorf("the instance %q gave a not defined status", limaInstanceName)
	}
}

Find more context here.

Before adding (e2e) and tests in general I would like to get the method right. Does this seem like the wrapper command you though of?

In the course of this issue, I would also add the output of lima's ls in general for a subsequent feature to get the information about the vm such as CPU, memory etc. mentioned in the output in my previous comment.

@ningziwen
Copy link
Member

My understanding is that VM status (running/stopped/nonexistent) is the major request of this issue. We could add more information like CPU/memory later. @shaonm can correct me if I'm wrong.

About the translation, I think the command "finch vm status", could just objectively return whatever the status is as "Running"/"Stopped"/"Nonexistent", instead of returning Error when the vm is not running. Returning Error when the vm is not running implies it has assumption that the vm should be running, which is not an assumption for the "finch vm status".

@niklasmtj
Copy link
Contributor

Yeah you're right about the assumptions. This shouldn't be the case when one just want to check if it's available or not. Will update it with only the logging info. Thanks for the feedback!

@ningziwen
Copy link
Member

About "logging info", I feel the status should be printed in stdout instead of stderr as it is the actual result instead of diagnostic information. (Difference between them)

Logger prints to stderr by default and normally prints the diagnostic information.

We can potentially use the Stdout() in stdlib.go. There are multiple ways of using it. We can discuss the details in PR.

@weikequ weikequ linked a pull request Dec 2, 2022 that will close this issue
1 task
ningziwen pushed a commit that referenced this issue Jan 17, 2023
Issue #, if available: #23

*Description of changes:*
Adding the `finch vm status` command.

*Testing done:*
`make && make lint && make test-unit` since I did not yet updated the
e2e tests where I need help to understand how to test the command


- [x] I've reviewed the guidance in CONTRIBUTING.md


#### License Acceptance

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

Signed-off-by: Niklas Metje <22395665+niklasmtj@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
Status: v0.3.0
Development

Successfully merging a pull request may close this issue.

5 participants