Differential D931

Instance destroy renamed to instance remove
ClosedPublic
Actions

Authored by lbrabec on Jul 12 2016, 12:22 PM.

Details

Reviewers
kparal
Commits
rTCLOUD7c58f028f235: Instance destroy renamed to instance remove
Summary

Just a short diff to make @kparal happy.

Test Plan

tested via cli

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.
lbrabec retitled this revision from to Instance destroy renamed to instance remove.Jul 12 2016, 12:22 PM
lbrabec updated this object.
lbrabec edited the test plan for this revision. (Show Details)
lbrabec added a reviewer: kparal.
lbrabec added a subscriber: kparal.
lbrabec updated this revision to Diff 2370.Jul 12 2016, 12:48 PM
  • docstrings fix
kparal accepted this revision.Jul 12 2016, 12:48 PM

Yay for reasonable names!

This revision is now accepted and ready to land.Jul 12 2016, 12:48 PM
Closed by commit rTCLOUD7c58f028f235: Instance destroy renamed to instance remove (authored by Lukas Brabec <lbrabec@redhat.com>). · Explain WhyJul 12 2016, 12:49 PM
This revision was automatically updated to reflect the committed changes.
tflink added a comment.Jul 13 2016, 3:37 AM

I know there aren't many but we do have some folks who use the testcloud cli for testing cloud images

I suspect that I'm going to be the only one tripped up by this change but if I wrote the original and the change makes more sense to you all, it'll probably make more sense to others as well.

That being said, I don't want to get into the habit of changing the cli interface or the library api in non-backwards compatible ways using no ticket and leaving the review posted for 30 minutes before pushing. It's probably harmless in this case but that won't always be the case

kparal added a comment.Jul 13 2016, 9:56 AM

Yeah, you're right @tflink, sorry about that. This should have been properly explained and definitely not pushed immediately. Let's remedy the situation a bit:

We will be talking here about stopping and removing a VM. There are two major calls in libvirt to do that:

  • destroy - will immediately destroy a VM, no time to react (similar to SIGKILL)
  • undefine - will remove the VM from libvirt VM listing, i.e. forgetting everything about the VM (you'll no longer see the VM in virt-manager or virsh list --all)

The same two calls in testcloud are these:

  • stop - calls libvirt.destroy
  • destroy - calls libvirt.undefine and removes the VM from testcloud VM listing

My major gripe was the confusion in terminology - testcloud is a wrapper around libvirt, yet it uses "destroy" term for a completely different operation. When I was dealing with issues from D908 and D933, I found it very hard to think and talk about it, when the same word meant a different thing in libvirt and testcloud. We agreed it would make sense to replace it with something that does not overload the original term and conveys the meaning better. We agreed on replacing testcloud's "destroy" with "remove". It better explains what's going to happen and it does not overload the terminology. So now the terminology looks like this:

  • testcloud stop -> libvirt destroy
  • testcloud remove -> libvirt undefine

That feels much better, I think. The "stop" is still somewhat misleading, I think, because it implies the VM is stopped gracefully (sending ACPI shut down signal or similar), which is not the case - the VM is immediately destroyed, same as pulling the power cord. However, we did not want to make too many changes, so we settled for just the once change described above.

Any thoughts regarding this change? Do you agree with it (we can always revert changes)?

If you believe testcloud is being used by a larger audience, we should probably add backwards compatibility, i.e. support destroy on cli and in API for some time, while printing a deprecation message. Any thoughts on that?

kparal added a comment.Jul 15 2016, 12:16 PM

Compat options added in D936.

Revision Contents

PathPackages
Mtestcloud/cli.py (26 lines)
Mtestcloud/instance.py (6 lines)

Diff 2371

View Options

testcloud/cli.py

Show First 20 Lines • Show All 134 Lines • ▼ Show 20 Line(s)126def _stop_instance(args):
135 135​
136 if tc_instance is None: 136​ if tc_instance is None:
137 raise TestcloudCliError("Cannot stop instance {} because it does " 137​ raise TestcloudCliError("Cannot stop instance {} because it does "
138 "not exist".format(args.name)) 138​ "not exist".format(args.name))
139 139​
140 tc_instance.stop() 140​ tc_instance.stop()
141 141​
142 142​
143def _destroy_instance(args): 143​def _remove_instance(args):
144 """Handler for 'instance destroy' command. Expects the following elements in args: 144​ """Handler for 'instance remove' command. Expects the following elements in args:
145 * name(str) 145​ * name(str)
146 146​
147 :param args: args from argparser 147​ :param args: args from argparser
148 """ 148​ """
149 log.debug("destroy 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​ if args.force:
158 tc_instance.stop() 158​ tc_instance.stop()
159 159​
160 tc_instance.destroy() 160​ tc_instance.remove()
161 161​
162 162​
163def _reboot_instance(args): 163​def _reboot_instance(args):
164 """Handler for 'instance reboot' command. Expects the following elements in args: 164​ """Handler for 'instance reboot' command. Expects the following elements in args:
165 * name(str) 165​ * name(str)
166 166​
167 :param args: args from argparser 167​ :param args: args from argparser
168 """ 168​ """
▲ Show 20 Lines • Show All 67 Lines • ▼ Show 20 Line(s)231 instarg_start.add_argument("--timeout",
236 default=config_data.BOOT_TIMEOUT) 236​ default=config_data.BOOT_TIMEOUT)
237 instarg_start.set_defaults(func=_start_instance) 237​ instarg_start.set_defaults(func=_start_instance)
238 238​
239 # instance stop 239​ # instance stop
240 instarg_stop = instarg_subp.add_parser("stop", help="stop instance") 240​ instarg_stop = instarg_subp.add_parser("stop", help="stop instance")
241 instarg_stop.add_argument("name", 241​ instarg_stop.add_argument("name",
242 help="name of instance to stop") 242​ help="name of instance to stop")
243 instarg_stop.set_defaults(func=_stop_instance) 243​ instarg_stop.set_defaults(func=_stop_instance)
244 # instance destroy 244​ # instance remove
245 instarg_destroy = instarg_subp.add_parser("destroy", help="destroy instance") 245​ instarg_remove = instarg_subp.add_parser("remove", help="remove instance")
246 instarg_destroy.add_argument("name", 246​ instarg_remove.add_argument("name",
247 help="name of instance to destroy") 247​ help="name of instance to remove")
248 instarg_destroy.add_argument("-f", 248​ instarg_remove.add_argument("-f",
249 "--force", 249​ "--force",
250 help="Stop the instance if it's running", 250​ help="Stop the instance if it's running",
251 action="store_true") 251​ action="store_true")
252 instarg_destroy.set_defaults(func=_destroy_instance) 252​ instarg_remove.set_defaults(func=_remove_instance)
253 # instance reboot 253​ # instance reboot
254 instarg_reboot = instarg_subp.add_parser("reboot", help="reboot instance") 254​ instarg_reboot = instarg_subp.add_parser("reboot", help="reboot instance")
255 instarg_reboot.add_argument("name", 255​ instarg_reboot.add_argument("name",
256 help="name of instance to reboot") 256​ help="name of instance to reboot")
257 instarg_reboot.add_argument("--timeout", 257​ instarg_reboot.add_argument("--timeout",
258 help="Time (in seconds) to wait for boot to " 258​ help="Time (in seconds) to wait for boot to "
259 "complete before completion, setting to 0" 259​ "complete before completion, setting to 0"
260 " disables all waiting.", 260​ " disables all waiting.",
▲ Show 20 Lines • Show All 108 Lines • Show Last 20 Lines
View Options

testcloud/instance.py

Show First 20 Lines • Show All 122 Lines • ▼ Show 20 Line(s)120 else:
123 instance['state'] = system_domains[instance['name']] 123​ instance['state'] = system_domains[instance['name']]
124 124​
125 instances.append(instance) 125​ instances.append(instance)
126 126​
127 return instances 127​ return instances
128 128​
129 129​
130class Instance(object): 130​class Instance(object):
131 """Handles creating, starting, stopping and destroying virtual machines 131​ """Handles creating, starting, stopping and removing virtual machines
132 defined on the local system, using an existing :py:class:`Image`. 132​ defined on the local system, using an existing :py:class:`Image`.
133 """ 133​ """
134 134​
135 def __init__(self, name, image=None, hostname=None): 135​ def __init__(self, name, image=None, hostname=None):
136 self.name = name 136​ self.name = name
137 self.image = image 137​ self.image = image
138 self.path = "{}/instances/{}".format(config_data.DATA_DIR, self.name) 138​ self.path = "{}/instances/{}".format(config_data.DATA_DIR, self.name)
139 self.seed_path = "{}/{}-seed.img".format(self.path, self.name) 139​ self.seed_path = "{}/{}-seed.img".format(self.path, self.name)
▲ Show 20 Lines • Show All 274 Lines • ▼ Show 20 Line(s)
414 def stop(self): 414​ def stop(self):
415 """Stop the instance""" 415​ """Stop the instance"""
416 416​
417 log.debug("stopping instance {}.".format(self.name)) 417​ log.debug("stopping instance {}.".format(self.name))
418 418​
419 # stop (destroy) the vm using virsh 419​ # stop (destroy) the vm using virsh
420 self._get_domain().destroy() 420​ self._get_domain().destroy()
421 421​
422 def destroy(self): 422​ def remove(self):
423 """Destroy an already stopped instance 423​ """Remove an already stopped instance
424 424​
425 :raises TestcloudInstanceError: if the image does not exist or is still 425​ :raises TestcloudInstanceError: if the image does not exist or is still
426 running 426​ running
427 """ 427​ """
428 428​
429 log.debug("removing instance {} from libvirt.".format(self.name)) 429​ log.debug("removing instance {} from libvirt.".format(self.name))
430 430​
431 # this should be changed if/when we start supporting configurable 431​ # this should be changed if/when we start supporting configurable
Show All 20 Lines