Hi Chris,
On Wed, Sep 16, 2009 at 8:15 PM, Chris Lalancette <clalance@xxxxxxxxxx> wrote:
> It looks relatively good, but one suggestion and two missing pieces:
>
> Ryota Ozaki wrote:
>
> <snip>
>
>> diff --git a/src/lxc_driver.c b/src/lxc_driver.c
>> index 0ec1e92..2399d98 100644
>> --- a/src/lxc_driver.c
>> +++ b/src/lxc_driver.c
>> @@ -1862,6 +1862,188 @@ static char *lxcGetHostname (virConnectPtr conn)
>> return result;
>> }
>>
>> +static int lxcFreezeContainer(lxc_driver_t *driver, virDomainObjPtr vm)
>> +{
>> + int timeout = 3; /* In seconds */
>> + int check_interval = 500; /* In milliseconds */
>> + int n_try = (timeout * (1000 / check_interval));
>> + int i = 0;
>> + int ret = -1;
>> + char *state = NULL;
>> + virCgroupPtr cgroup = NULL;
>> +
>> + if (!(driver->cgroup &&
>> + virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) == 0))
>> + return -1;
>> +
>> + while (++i <= n_try) {
>> + int r;
>> + /*
>> + * Writing "FROZEN" to the "freezer.state" freezes the group,
>> + * i.e., the container, temporarily transiting "FREEZING" state.
>> + * Once the freezing is completed, the state of the group transits
>> + * to "FROZEN".
>> + * (see linux-2.6/Documentation/cgroups/freezer-subsystem.txt)
>> + */
>> + r = virCgroupSetFreezerState(cgroup, "FROZEN");
>> +
>> + /*
>> + * Returning EBUSY explicitly indicates that the group is
>> + * being freezed but incomplete and other errors are true
>> + * errors.
>> + */
>> + if (r < 0 && r != -EBUSY) {
>> + VIR_DEBUG("Writing freezer.state failed with errno: %d", r);
>> + goto error;
>> + }
>> +
>> + /*
>> + * Unfortunately, returning 0 (success) is likely to happen
>> + * even when the freezing has not been completed. Sometimes
>> + * the state of the group remains "FREEZING" like when
>> + * returning -EBUSY and even worse may never transit to
>> + * "FROZEN" even if writing "FROZEN" again.
>> + *
>> + * So we don't trust the return value anyway and always
>> + * decide that the freezing has been complete only with
>> + * the state actually transit to "FROZEN".
>> + */
>> + usleep(check_interval * 1000);
>
> Suggestion: it might be better to sleep for shorter periods of time here in a
> loop. That is, sleep for say 1 ms, check if it's changed, sleep for 1 ms, etc.
> If it doesn't appear after 500 ms, then fail. That way you'll know about the
> change faster.
Sure. Current waiting time might be a bit lazy.
With my experience, once the state of a loaded container gets FREEZING,
it doesn't transit to FROZEN in short periods such as 1ms. So 50ms or
somewhat is probably appropriate to avoid busy checking. OTOH, the max
waiting time also might be able to be shorter than current 3 sec.,
e.g., 500ms as you say or 1 sec. Actually a container staying FREEZING
state for 1 sec. seems never transiting to FROZEN.
For the moment, I'll try around 50ms interval and 500ms max waiting time.
>
> <snip>
>
>> +static int lxcDomainSuspend(virDomainPtr dom)
>> +{
>> + lxc_driver_t *driver = dom->conn->privateData;
>> + virDomainObjPtr vm;
>> + int ret = -1;
>> +
>> + lxcDriverLock(driver);
>> + vm = virDomainFindByUUID(&driver->domains, dom->uuid);
>> +
>> + if (!vm) {
>> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>> + virUUIDFormat(dom->uuid, uuidstr);
>> + lxcError(dom->conn, dom, VIR_ERR_NO_DOMAIN,
>> + _("no domain with matching uuid '%s'"), uuidstr);
>> + goto cleanup;
>> + }
>> +
>> + if (!virDomainIsActive(vm)) {
>> + lxcError(dom->conn, dom, VIR_ERR_OPERATION_INVALID,
>> + "%s", _("domain is not running"));
>> + goto cleanup;
>> + }
>> +
>> + if (vm->state != VIR_DOMAIN_PAUSED) {
>> + if (lxcFreezeContainer(driver, vm) < 0) {
>> + lxcError(dom->conn, dom, VIR_ERR_OPERATION_FAILED,
>> + "%s", _("suspend operation failed"));
>> + goto cleanup;
>> + }
>> + vm->state = VIR_DOMAIN_PAUSED;
>> + }
>> +
>> + if (virDomainSaveStatus(dom->conn, driver->stateDir, vm) < 0)
>> + goto cleanup;
>> + ret = 0;
>
> Here, you'll want to generate an event (virDomainEventNewFromObj) for the
> change. You can look at lxcDomainCreateAndStart() for an example of what kind
> of event you would want to generate.
I really forgot it ;) I'll add.
>
> <snip>
>
>> +static int lxcDomainResume(virDomainPtr dom)
>> +{
>> + lxc_driver_t *driver = dom->conn->privateData;
>> + virDomainObjPtr vm;
>> + int ret = -1;
>> +
>> + lxcDriverLock(driver);
>> + vm = virDomainFindByUUID(&driver->domains, dom->uuid);
>> +
>> + if (!vm) {
>> + char uuidstr[VIR_UUID_STRING_BUFLEN];
>> + virUUIDFormat(dom->uuid, uuidstr);
>> + lxcError(dom->conn, dom, VIR_ERR_NO_DOMAIN,
>> + _("no domain with matching uuid '%s'"), uuidstr);
>> + goto cleanup;
>> + }
>> +
>> + if (!virDomainIsActive(vm)) {
>> + lxcError(dom->conn, dom, VIR_ERR_OPERATION_INVALID,
>> + "%s", _("domain is not running"));
>> + goto cleanup;
>> + }
>> +
>> + if (vm->state == VIR_DOMAIN_PAUSED) {
>> + if (lxcUnfreezeContainer(driver, vm) < 0) {
>> + lxcError(dom->conn, dom, VIR_ERR_OPERATION_FAILED,
>> + "%s", _("resume operation failed"));
>> + goto cleanup;
>> + }
>> + vm->state = VIR_DOMAIN_RUNNING;
>> + }
>> +
>> + if (virDomainSaveStatus(dom->conn, driver->stateDir, vm) < 0)
>> + goto cleanup;
>> + ret = 0;
>
> Same thing here.
and here as well.
Thanks for your suggestions!
ozaki-r
>
> --
> Chris Lalancette
>
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list