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.eb...@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;
>>>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
