[pve-devel] [PATCH v2 ha-manager 07/12] Add stop command to the API

[Thomas Lamprecht]

On 10/1/19 9:28 AM, Fabian Ebner wrote:
> On 9/30/19 5:13 PM, Thomas Lamprecht wrote:
>> On 9/30/19 9:22 AM, Fabian Ebner wrote:
>>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>>> ---
>>>   src/PVE/API2/HA/Resources.pm | 37 ++++++++++++++++++++++++++++++++++++
>>>   src/PVE/CLI/ha_manager.pm    |  2 ++
>>>   2 files changed, 39 insertions(+)
>> besides the nit^Wfact that the commit subject should read
>> "Add stop command to the API and CLI", not sure if this is required?
>>
>> Couldn't we just have a common helper somewhere and then the
>> qm stop or pct stop just calls that (indirectly?).
>>
>> Just need to rethink concepts for this one, code itself looks OK.
> 
> Isn't the separation between the Qemu/LXC code and HA manager better like this?
> 
> It seems like the calls from inside Qemu/LXC code that handle services use the 'ha-manager' command and so I thought it would be natural to use it here as well.

Yeah, there you're right.. Just not 100% happy with the duality of
doing things (request state stop via 'set <sid>' or through this)

Maybe you could keep it but not expose it thorugh API? At least
initially. You could do a ha-manager CLI internal implementation
like "status" is. Then we do not need to keep the API compatible
and are a bit more flexible if we get a better idea.

Maybe do it as subcommand so that I can call
# ha-manager crm-command stop <sid>

migrate/relocate could also moved there (and the old
ones aliased to the new ones for backwards compat.)

What do you think?

> 
> Where would a good place to put the helper be?
> 
>>> diff --git a/src/PVE/API2/HA/Resources.pm b/src/PVE/API2/HA/Resources.pm
>>> index 2b62ee8..ecc5f0c 100644
>>> --- a/src/PVE/API2/HA/Resources.pm
>>> +++ b/src/PVE/API2/HA/Resources.pm
>>> @@ -353,4 +353,41 @@ __PACKAGE__->register_method ({
>>>       return undef;
>>>       }});
>>>   +__PACKAGE__->register_method ({
>>> +    name => 'stop',
>>> +    protected => 1,
>>> +    path => '{sid}/stop',
>>> +    method => 'POST',
>>> +    description => "Request the service to be stopped.",
>>> +    permissions => {
>>> +    check => ['perm', '/', [ 'Sys.Console' ]],
>>> +    },
>>> +    parameters => {
>>> +    additionalProperties => 0,
>>> +    properties => {
>>> +        sid => get_standard_option('pve-ha-resource-or-vm-id',
>>> +                      { completion => \&PVE::HA::Tools::complete_sid }),
>>> +        timeout => {
>>> +        description => "Timeout in seconds. If set to 0 a hard stop will be performed.",
>>> +        type => 'integer',
>>> +        minimum => 0,
>>> +        optional => 1,
>>> +        },
>>> +    },
>>> +    },
>>> +    returns => { type => 'null' },
>>> +    code => sub {
>>> +    my ($param) = @_;
>>> +
>>> +    my ($sid, $type, $name) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
>>> +
>>> +    PVE::HA::Config::service_is_ha_managed($sid);
>>> +
>>> +    check_service_state($sid);
>>> +
>>> +    PVE::HA::Config::queue_crm_commands("stop $sid $param->{timeout}");
>>> +
>>> +    return undef;
>>> +    }});
>>> +
>>>   1;
>>> diff --git a/src/PVE/CLI/ha_manager.pm b/src/PVE/CLI/ha_manager.pm
>>> index 5ce4c30..c9d4e7f 100644
>>> --- a/src/PVE/CLI/ha_manager.pm
>>> +++ b/src/PVE/CLI/ha_manager.pm
>>> @@ -114,6 +114,8 @@ our $cmddef = {
>>>       migrate => [ "PVE::API2::HA::Resources", 'migrate', ['sid', 'node'] ],
>>>       relocate => [ "PVE::API2::HA::Resources", 'relocate', ['sid', 'node'] ],
>>>   +    stop => [ "PVE::API2::HA::Resources", 'stop', ['sid', 'timeout'] ],
>>> +
>>>   };
>>>     1;
>>>