make it easier to stop and remove instances
ClosedPublic
Actions

Authored by kparal on Jul 13 2016, 9:38 AM.

Details

Reviewers

lbrabec
tflink
mkrizek

Group Reviewers

testcloud

Commits

rTCLOUD25dbb1108da2: make it easier to stop and remove instances

Summary

This fixes the following:

instance.stop() no longer crashes when the instance is already

stopped

instance.remove() now has autostop=True parameter which stops the

instance automatically before removing it, instead of raising errors
3. --force option is removed for cli call instance remove, because
the instance is now stopped automatically. I can't imagine a situation
where you'd like to remove an instance from cli and not want autostop
enabled, therefore --force is now unnecessary.

This is a follow-up of some issues identified in D908.

Test Plan

apply this patch onto the original code:

diff --git a/testcloud/instance.py b/testcloud/instance.py
index e1baf48..8a85fa4 100644
--- a/testcloud/instance.py
+++ b/testcloud/instance.py
@@ -377,7 +377,7 @@ class Instance(object):
                                         instance or if the timeout is reached
                                         while looking for a network interface
         """
-
+        raise KeyboardInterrupt()
         log.debug("Creating domain {}".format(self.name))
         dom = self._get_domain()
         create_status = dom.create()

Now run libtaskotron, and it will keep a VM defined and not cleaned up. That's because
we try to stop an instance which is not running, which fails. Now apply this diff, and
run it again. libtaskotron should clean up the VM properly (it no longer crashes during
instance.stop()).

Diff Detail

Repository

rTCLOUD testcloud

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

kparal retitled this revision from to make it easier to stop and remove instances.Jul 13 2016, 9:38 AM

kparal updated this object.

kparal edited the test plan for this revision. (Show Details)

kparal added reviewers: testcloud, lbrabec, tflink.

kparal added a subscriber: lnie.

kparal mentioned this in D931: Instance destroy renamed to instance remove.Jul 13 2016, 9:56 AM

testcloud/instance.py
426	In `cli.py` there is similar code in function `_stop_instance()`, it checks whether the instance exists and raises if not. I can see arguments for one place or the other, but I'm not sure that we should have both.

If I understand this correctly, doing testcloud instance remove <uuid> removes the instance without asking the user if he really wants to do so. Running the remove command by mistake results in losing the instance. I consider this dangerous from the user standpoint. Why remove the --force option which is considered as "unix standard"?

This revision now requires changes to proceed.Jul 13 2016, 10:54 AM

In D933#17536, @mkrizek wrote:

If I understand this correctly, doing testcloud instance remove <uuid> removes the instance without asking the user if he really wants to do so. Running the remove command by mistake results in losing the instance. I consider this dangerous from the user standpoint. Why remove the --force option which is considered as "unix standard"?

Testcloud never used to prompt before removing and that did not change. We're talking about once specific use case, when you try to remove a running instance, in which case it printed an error. With this patch, when you try to remove a running instance, it stops it and removes it. It saves you work. (We could also remove the instance without stopping it, which is supported by libvirt, but not implemented in testcloud atm).

If you want to compare this to unix tools, say rm, you can also remove a file while it is being used, you don't need to close all apps using it first (hello, Windows). It does not prompt you about this, unless you specify --interactive. You can delete a file by mistake, sure.

Is protection against mistakes more important than having a comfortable CLI (one command instead of two)? I don't know, and honestly I don't care much about testcloud CLI. If you think it's better the way it was, just say the word, and I'll revert the CLI change, it's trivial. Whatever you prefer. (Please note, however, that it will still not prompt, it will print an error in case the instance is running and require you to use two commands or --force, or it will remove the instance without any questions in case the instance is not running. Which is not really similar to rm --interactive.)

testcloud/instance.py
426	I noticed that too and considered it a bit weird as well. However, I'm not sure what is the best solution here, because I'm not sure what the original motivation for this approach was. You see, the same code exists in `_remove_instance` in `cli.py`, and I merely copied an existing check from `Instance.remove()`. It seemed inconsistent to perform the check during remove, but not during stop. This is not something that I coded, it was already there. We can look into this issue, but I'd rather have it as a separate diff, it's a different issue.

In D933#17540, @kparal wrote:

In D933#17536, @mkrizek wrote:

If I understand this correctly, doing testcloud instance remove <uuid> removes the instance without asking the user if he really wants to do so. Running the remove command by mistake results in losing the instance. I consider this dangerous from the user standpoint. Why remove the --force option which is considered as "unix standard"?

Testcloud never used to prompt before removing and that did not change.

No, but the patch changes the behavior when it comes to protection against removing a running instance.

We're talking about once specific use case, when you try to remove a running instance, in which case it printed an error. With this patch, when you try to remove a running instance, it stops it and removes it. It saves you work. (We could also remove the instance without stopping it, which is supported by libvirt, but not implemented in testcloud atm).

If you want to compare this to unix tools, say rm, you can also remove a file while it is being used, you don't need to close all apps using it first (hello, Windows). It does not prompt you about this, unless you specify --interactive. You can delete a file by mistake, sure.

Is protection against mistakes more important than having a comfortable CLI (one command instead of two)? I don't know, and honestly I don't care much about testcloud CLI. If you think it's better the way it was, just say the word, and I'll revert the CLI change, it's trivial. Whatever you prefer. (Please note, however, that it will still not prompt, it will print an error in case the instance is running and require you to use two commands or --force, or it will remove the instance without any questions in case the instance is not running. Which is not really similar to rm --interactive.)

I prefer the -f/--force option but it has to do with how I am used to work with testcloud cli. @tflink, as another "heavy" testcloud user :) , do you have any preference on this?

revert CLI change

Since this is not a straightforward change, I decided to split it into two separate diffs. I hope the rest of the code is non-controversial and we can commit it right away, and then we can discuss the CLI changes in a follow-up ticket.

kparal mentioned this in T816: Talking about whether we should clean up the files left for users.Jul 14 2016, 10:42 AM

ack

mkrizek accepted this revision.Jul 14 2016, 11:02 AM

This revision is now accepted and ready to land.Jul 14 2016, 11:02 AM

kparal updated this object.Jul 14 2016, 11:32 AM

Closed by commit rTCLOUD25dbb1108da2: make it easier to stop and remove instances (authored by kparal). · Explain WhyJul 14 2016, 11:37 AM

This revision was automatically updated to reflect the committed changes.

kparal mentioned this in D934: remove '--force' cmdline option.Jul 14 2016, 11:48 AM

The follow-up diff is in D934.

lnie mentioned this in D909: clean up the space for users when testcloud is interrupted by an error.Aug 10 2016, 7:56 AM

Revision Contents

			Path	Packages
M			testcloud/cli.py (5 lines)
M			testcloud/instance.py (35 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	2374	7c58f02		Jul 13 2016, 9:38 AM	★	★
Diff 2	2376	7c58f02	- revert CLI change	Jul 14 2016, 10:16 AM	★	★
Diff 3	2377	7c58f02	rTCLOUD25dbb1108da28fc3655308415bf8715961024483	Jul 14 2016, 11:35 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
282d1d496a12	fab94d0ea8b0	575a68443685	Kamil Páral	revert CLI change (Show More…)	Jul 14 2016, 10:13 AM
575a68443685	0d71a2ee5af4	7c58f028f235	Kamil Páral	make it easier to stop and remove instances (Show More…)	Jul 13 2016, 9:29 AM

Diff 2377

View Options

testcloud/cli.py

Show First 20 Lines • Show All 148 Lines • ▼ Show 20 Line(s)		143	def _remove_instance(args):
149	log.debug("remove instance: {}".format(args.name))	149		log.debug("remove instance: {}".format(args.name))
150		150
151	tc_instance = instance.find_instance(args.name)	151		tc_instance = instance.find_instance(args.name)
152		152
153	if tc_instance is None:	153		if tc_instance is None:
154	raise TestcloudCliError("Cannot remove instance {} because it does "	154		raise TestcloudCliError("Cannot remove instance {} because it does "
155	"not exist".format(args.name))	155		"not exist".format(args.name))
156		156
157	if args.force:	157		tc_instance.remove(autostop=args.force)
158	tc_instance.stop()
159
160	tc_instance.remove()
161		158
162		159
163	def _reboot_instance(args):	160		def _reboot_instance(args):
164	"""Handler for 'instance reboot' command. Expects the following elements in args:	161		"""Handler for 'instance reboot' command. Expects the following elements in args:
165	* name(str)	162		* name(str)
166		163
167	:param args: args from argparser	164		:param args: args from argparser
168	"""	165		"""
▲ Show 20 Lines • Show All 200 Lines • Show Last 20 Lines