Re: [libvirt] [PATCH v2 4/4] virsh: add --persistent to cmd schedinfo
于 2011年05月10日 09:31, Hu Tao 写道:
On Mon, May 09, 2011 at 05:37:04PM +0800, Osier Yang wrote:
> 于 2011年05月09日 16:31, Hu Tao 写道:
>> This enables user to modify cpu.shares even when domain is inactive.
>> ---
>> tools/virsh.c | 14 +++++++++++++-
>> 1 files changed, 13 insertions(+), 1 deletions(-)
>>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index 2b16714..58facc4 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -1590,6 +1590,7 @@ static const vshCmdOptDef opts_schedinfo[] = {
>> {"set", VSH_OT_STRING, VSH_OFLAG_NONE,
N_("parameter=value")},
>> {"weight", VSH_OT_INT, VSH_OFLAG_NONE, N_("weight for
XEN_CREDIT")},
>> {"cap", VSH_OT_INT, VSH_OFLAG_NONE, N_("cap for
XEN_CREDIT")},
>> + {"persistent", VSH_OT_BOOL, 0, N_("persist VM on
destination")},
>> {NULL, 0, 0, NULL}
>> };
>>
>> @@ -1697,6 +1698,7 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
>> int update = 0;
>> int i, ret;
>> bool ret_val = false;
>> + unsigned int flags = 0;
>>
>> if (!vshConnectionUsability(ctl, ctl->conn))
>> return false;
>> @@ -1704,6 +1706,16 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd)
>> if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>> return false;
>>
>> + if (vshCommandOptBool(cmd, "persistent"))
>> + flags |= VIR_DOMAIN_SCHEDPARAM_CONFIG;
>> + if (virDomainIsActive(dom))
>> + flags |= VIR_DOMAIN_SCHEDPARAM_LIVE;
>> +
>> + if (!(flags& (VIR_DOMAIN_SCHEDPARAM_CONFIG |
VIR_DOMAIN_SCHEDPARAM_LIVE))) {
>> + vshError(ctl, "%s", _("Domain is not running and you
didn't specify --persistent parameter."));
>> + goto cleanup;
>> + }
>> +
>> /* Print SchedulerType */
>> schedulertype = virDomainGetSchedulerType(dom,&nparams);
>
> Hi HuTao,
>
> AFAIK, virDomainGetSchedulerParameters won't work for inactive domain,
> cgroup for domain is removed when the domain is destroyed/shutdown'ed,
> (perhaps we will support persistent XML for domain cgroup, but currently
> it doesn't), means it will fail earlier, before could executing
> virDomainSetSchedulerParametersFlags.
Yes, virDomainGetSchedulerParameters tries to read cpu.shares value
from domain's cgroup entry which doesn't exist when domain is off.
But, IMHO, the configuration (cpu.shares) should not depend on whether
domain is on or not. When domain is off, we can read/write the value
from/to its XML(domain.cputune.shares is already for this). What's
your opinion?
Yes, we have persistent XML for cpu.shares now, we can change it
even if domain is off, the problem is cmdSchedinfo mixes get/set
schedParams together, it needs to use virDomainGetSchedulerParameters
to get the schedParams the driver suppports, then looks up to see
if there is something is set to be updated, (it's reasonable to do
like so, as perhaps more driver will support scheduler setting, and
more scheduler setting will be supported, though currently only
cpu.shares is supported by only QEMU/LXC, we can't hardcode it).
but virDomainGetSchedulerParameters can't work when domain is off.
We can seperate get/set schedParams into two standalone functions,
so that could form the params with anything the user provides, and
simply pass it to internal driver function, it reports error if
user provides wrong params feild. Then we can do changing on inactive
domain. But I guess this won't be accepted, as "virsh schedinfo" exists
for long time, changing the behaviour will affects existed scripts
that based on virsh.
That was my mainly concern.
>
> Actually I made similar patches about one month before:
>
> https://www.redhat.com/archives/libvir-list/2011-April/msg00806.html
Thanks, I will take a look at it.
>
> the difference with your patches is it only allows changing on running
> domain, options are "--live" and "--persistent", perhaps your
principle
> is better though. ：）
>