(PUP-9051) Fix SMF provider to properly implement enableable semantics #6993
Conversation
Previously, the SMF provider had the same semantics for enable as it did for ensure. Specifically, #enable and #disable delegated to #start and #stop. This is completely inconsistent with the enableable semantics for service providers, semantics which other Unix-based service providers like systemd implement. For example, if you had a SMF-based service resource with enable => true and ensure => false, the net effect of the operation would be a disabled service that would remain disabled when rebooted, which is incorrect and inconsistent. The correct and consistent effect should be a disabled service that would start up when rebooted. Essentially, enableable semantics requires that enable and ensure be independent operations where enable handles whether the service starts or stops at boot time, while ensure handles whether the service starts or stops in the current running instance. The previous SMF provider was conflating the two properties together so that enable and ensure were no longer independent operations. This commit fixes that. It rewrites core parts of the SMF provider to now conform to enableable semantics and also fixes some possible issues with it. The key takeaways here are: * Service commands now use the service FMRI instead of the resource name. This is the unique resource identifier for the particular instance. Previously, we'd pass in the resource name instead which would match a pattern corresponding to multiple service instances. We only want to manage one service instance in a single service resource. The code now fails if more than one FMRI exists for a given service resource name. * We now sync. the enable and ensure properties of the service resource in a single transaction, specifically inside the #flush method. This is due to the dependency between the ensure and enable properties caused by how SMF works under the hood. Acceptance tests for this new behavior have been added. The existing nonexistent service tests on the AIX service provider have been refactored to a common helper function that both the SMF and AIX provider tests can call on. This helper function should provide a clean interface for nonexistent service tests that may be used for other service providers in the future.
| else | ||
| self.maybe_clear_service_then_svcadm(cur_state, 'disable', '-s') | ||
| end | ||
|
|
There was a problem hiding this comment.
I think some logging here might be useful. If so, what type of log messages would be good? Maybe something to indicate that we're now setting the service's ensure state after enabling it?
|
|
||
| # This hash contains the properties we need to sync. in our flush method. | ||
| # | ||
| # TODO (PUP-9051): Should we use @property_hash here? It seems like |
There was a problem hiding this comment.
Question about property_hash here. My main concern is if it's an instance variable that's predictable and something we control. Main use would be to track the properties we need to sync.
|
CLA signed by all contributors. |
|
Have some questions about this so placed the "Don't Merge" label until those questions are addressed. Otherwise, this is good for a review. |
ekinanp
added
Do Not Merge
and removed
blocked
|
Closing this PR since we will not be landing this in time for the Puppet 6 release. This will likely be rebased and targeted at Puppet 7 in the future. |
|
There's a cleaner way to do this without requiring the use of flush. If we have enable/disable maintain the current ensure state, then things will work correctly. This PR should not only be rebased, but also be re-worked to remove the use of the flush. |
Previously, the SMF provider had the same semantics for enable as it did
for ensure. Specifically, #enable and #disable delegated to #start and #stop.
This is completely inconsistent with the enableable semantics for service
providers, semantics which other Unix-based service providers like systemd
implement. For example, if you had a SMF-based service resource with
enable => true and ensure => false, the net effect of the operation would
be a disabled service that would remain disabled when rebooted, which is
incorrect and inconsistent. The correct and consistent effect should be
a disabled service that would start up when rebooted.
Essentially, enableable semantics requires that enable and ensure be
independent operations where enable handles whether the service starts
or stops at boot time, while ensure handles whether the service starts
or stops in the current running instance. The previous SMF provider was
conflating the two properties together so that enable and ensure were
no longer independent operations.
This commit fixes that. It rewrites core parts of the SMF provider to
now conform to enableable semantics and also fixes some possible issues
with it. The key takeaways here are:
Service commands now use the service FMRI instead of the resource
name. This is the unique resource identifier for the particular
instance. Previously, we'd pass in the resource name instead which would
match a pattern corresponding to multiple service instances. We only
want to manage one service instance in a single service resource. The code now
fails if more than one FMRI exists for a given service resource name.
We now sync. the enable and ensure properties of the service resource in a single
transaction, specifically inside the #flush method. This is due to the dependency
between the ensure and enable properties caused by how SMF works under the hood.
Acceptance tests for this new behavior have been added. The existing
nonexistent service tests on the AIX service provider have been
refactored to a common helper function that both the SMF and AIX
provider tests can call on. This helper function should provide a clean
interface for nonexistent service tests that may be used for other
service providers in the future.