charm calling systemctl enable for all services even if they are already enabled

[Edward Hope-Morley]

Opening a new bug for the issue described in comments in https://bugs.launchpad.net/charm-ovn-chassis/+bug/1854004 since it appears to be a different issue to the one that bug was originally raised for. Re-posting description here.

The charm will run [1] on every update-status hook which in turn calls [2] and then [3]. This results in a call to service_resume() for all services (in this case neutron-ovn-metadata-agent and ovn-host) at [4] which translates to a systemctl umask then enable which produces the stderr msg we see in the unit log i.e.

# systemctl enable neutron-ovn-metadata-agent
Synchronizing state of neutron-ovn-metadata-agent.service with SysV service script with /lib/systemd/systemd-sysv-install.
Executing: /lib/systemd/systemd-sysv-install enable neutron-ovn-metadata-agent

[1] https://github.com/openstack/charm-layer-openstack/blob/7c671b0696977f455616565d956895b2f890464b/reactive/layer_openstack.py#L173
[2] https://github.com/openstack/charms.openstack/blob/aec72e735b4678e3c8970dbfffe16b002a05b812/charms_openstack/charm/classes.py#L258
[3] https://github.com/juju/charm-helpers/blob/511bc0346e3002cb31475c03697c2f5b065967f3/charmhelpers/contrib/openstack/utils.py#L1605
[4] https://github.com/juju/charm-helpers/blob/511bc0346e3002cb31475c03697c2f5b065967f3/charmhelpers/core/host.py#L242

This appears to be default behaviour for any charm that uses layer_openstack since nothing ever unsets the flags that are causing that method to be called each time a hook fires. It can be overridden by the ovn-chassis charm and I feel like perhaps it should be since there is no need to be continuously doing this for a service that is already enabled.

[Alex Kavanagh (ajkavanagh)]

Good bit of sleuthing!

I think there's a couple of ways that this could be solved.

1) prevent the function from being called by using a 'gating' flag or similar; i.e. after the service is enable, set a flag that it is enabled. This is a bit tricky as it's essentially caching metadata about the real world, and that may change, whilst the flag stays the same (i.e. an external actor may disable the service).
2) my preference; make the `enable_services` function both idempotent (which is sort of is) but add additional logic to not do the enable for a service if it is already enabled. This will maintain the 'self-repair' aspect of the charm function, but lose the annoying side-effects of continually enabling a service.

[Edward Hope-Morley (hopem)]

We could use the 'systemctl is-enabled <service>' command to check if it is already enabled.

[Edward Hope-Morley (hopem)]

Here is a simple demo of how this could work:

>>> service_pause('neutron-ovn-metadata-agent')
active
Synchronizing state of neutron-ovn-metadata-agent.service with SysV service script with /lib/systemd/systemd-sysv-install.
Executing: /lib/systemd/systemd-sysv-install disable neutron-ovn-metadata-agent
Removed /etc/systemd/system/multi-user.target.wants/neutron-ovn-metadata-agent.service.
Created symlink /etc/systemd/system/neutron-ovn-metadata-agent.service → /dev/null.
True
>>> print("is enabled") if service('is-enabled', 'neutron-ovn-metadata-agent') else print("is not enabled")
masked
is not enabled
>>> service_resume('neutron-ovn-metadata-agent')
Removed /etc/systemd/system/neutron-ovn-metadata-agent.service.
Synchronizing state of neutron-ovn-metadata-agent.service with SysV service script with /lib/systemd/systemd-sysv-install.
Executing: /lib/systemd/systemd-sysv-install enable neutron-ovn-metadata-agent
Created symlink /etc/systemd/system/multi-user.target.wants/neutron-ovn-metadata-agent.service → /lib/systemd/system/neutron-ovn-metadata-agent.service.
failed
True
>>> print("is enabled") if service('is-enabled', 'neutron-ovn-metadata-agent') else print("is not enabled")
enabled
is enabled

[Edward Hope-Morley (hopem)]

charm-helpers patch: https://github.com/juju/charm-helpers/pull/882/files

[Arif Ali (arif-ali)]

I have tested that patch in my lab env and works as expected, thanks Ed

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/c/x/charm-ovn-chassis/+/915472

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/x/charm-ovn-chassis/+/915472
Committed: https://opendev.org/x/charm-ovn-chassis/commit/c725945a630459d20d8d6b9c54b9ba1804a8c9a3
Submitter: "Zuul (22348)"
Branch: master

commit c725945a630459d20d8d6b9c54b9ba1804a8c9a3
Author: Edward Hope-Morley <email address hidden>
Date: Thu Apr 11 11:48:47 2024 +0100

    Rebuild to pull in charm-helpers change

    The change this pulls in ensures that when service_resume is called (which
    happens on every update-status hook), services are only enabled and started if
    they are disabled and stopped respectively.

    Also pins charmcraft to 2.2/stable since 2.5.5 throws an error building charmtools.

    Related-Bug: #2058505
    Change-Id: I2e42dd30066f5a099c97e7504fcdc6138c4a7496

[Edward Hope-Morley (hopem)]

charm-helpers patch merged to stable/{bobcat,antelope,zed,yoga}

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/23.09
Review: https://review.opendev.org/c/x/charm-ovn-chassis/+/917027

[OpenStack Infra (hudson-openstack)]

Change abandoned by "Edward Hope-Morley <email address hidden>" on branch: stable/23.09
Review: https://review.opendev.org/c/x/charm-ovn-chassis/+/917027
Reason: need to resubmit with different changeid

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/23.09
Review: https://review.opendev.org/c/x/charm-ovn-chassis/+/917028

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/23.03
Review: https://review.opendev.org/c/x/charm-ovn-chassis/+/917029

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/22.09
Review: https://review.opendev.org/c/x/charm-ovn-chassis/+/917030

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/22.03
Review: https://review.opendev.org/c/x/charm-ovn-chassis/+/917032

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/x/charm-ovn-chassis/+/917028
Committed: https://opendev.org/x/charm-ovn-chassis/commit/a5677172e3644d86e5dba7a9b7846cacf233c049
Submitter: "Zuul (22348)"
Branch: stable/23.09

commit a5677172e3644d86e5dba7a9b7846cacf233c049
Author: Edward Hope-Morley <email address hidden>
Date: Thu Apr 25 12:26:10 2024 +0100

    Rebuild to pull in charm-helpers change

    The change this pulls in ensures that when service_resume is called (which
    happens on every update-status hook), services are only enabled and started if
    they are disabled and stopped respectively.

    Also pins charmcraft to 2.2/stable since 2.5.5 throws an error building charmtools.

    Related-Bug: #2058505
    Change-Id: I2e42dd30066f5a099c97e7504fcdc6138c4a7496

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/x/charm-ovn-chassis/+/917029
Committed: https://opendev.org/x/charm-ovn-chassis/commit/da4f329be9ca9eaeb36734ba2303168f771c7e56
Submitter: "Zuul (22348)"
Branch: stable/23.03

commit da4f329be9ca9eaeb36734ba2303168f771c7e56
Author: Edward Hope-Morley <email address hidden>
Date: Thu Apr 25 12:30:47 2024 +0100

    Rebuild to pull in charm-helpers change

    The change this pulls in ensures that when service_resume is called (which
    happens on every update-status hook), services are only enabled and started if
    they are disabled and stopped respectively.

    Related-Bug: #2058505
    Change-Id: I2e42dd30066f5a099c97e7504fcdc6138c4a7496

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/x/charm-ovn-chassis/+/917030
Committed: https://opendev.org/x/charm-ovn-chassis/commit/9b63bf0625926eac3429f33df3f19f99338ccdd6
Submitter: "Zuul (22348)"
Branch: stable/22.09

commit 9b63bf0625926eac3429f33df3f19f99338ccdd6
Author: Edward Hope-Morley <email address hidden>
Date: Thu Apr 25 12:33:49 2024 +0100

    Rebuild to pull in charm-helpers change

    The change this pulls in ensures that when service_resume is called (which
    happens on every update-status hook), services are only enabled and started if
    they are disabled and stopped respectively.

    Related-Bug: #2058505
    Change-Id: I2e42dd30066f5a099c97e7504fcdc6138c4a7496

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/x/charm-ovn-chassis/+/917032
Committed: https://opendev.org/x/charm-ovn-chassis/commit/a7a67c37bcb32675299775e479da139ee8608517
Submitter: "Zuul (22348)"
Branch: stable/22.03

commit a7a67c37bcb32675299775e479da139ee8608517
Author: Edward Hope-Morley <email address hidden>
Date: Thu Apr 25 12:38:40 2024 +0100

    Rebuild to pull in charm-helpers change

    The change this pulls in ensures that when service_resume is called (which
    happens on every update-status hook), services are only enabled and started if
    they are disabled and stopped respectively.

    Related-Bug: #2058505
    Change-Id: I2e42dd30066f5a099c97e7504fcdc6138c4a7496