[PUP-6165] it should not be possible to disable manifest ordering Created: 2016/04/12  Updated: 2019/01/09  Resolved: 2018/09/17

Status: Closed
Project: Puppet
Component/s: None
Affects Version/s: PUP 4.4.1
Fix Version/s: PUP 6.0.0

Type: Bug Priority: Normal
Reporter: R.I.Pienaar Assignee: Jacob Helwig
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to PUP-9107 Deprecate non-manifest orderings Resolved
Template: PUP Bug Template
Epic Link: Puppet 6.0 Breaking Changes
Team: Coremunity
Sprint: Platform Core KANBAN
Release Notes: Deprecation
Release Notes Summary: The deprecated `ordering` setting has been removed, and catalogs now always have the ordering previously provided by the "manifest" value of this setting.

 Description   

Manifest ordering have as far as I can tell been proven to work, problem is with it being configurable it's not something one can really rely on in public code as you just have no way to know if your users have it on or not.

When next allowed by semver I'd propose it be turned on and remain on without the option to configure.



 Comments   
Comment by Moses Mendoza [ 2017/05/16 ]

Eric Sorenson can you confirm + fix version?

Comment by Henrik Lindberg [ 2017/05/17 ]

The making of "manifest-order" into the only setting also turns it into a guarantee - if we introduce an opt-in to optimize with parallel execution that guarantee would be broken. Thus, if we do this we miss the future possibility of automatically fully optimizing the agent's time to apply a catalog by parallelizing work. Users would then need to explicitly state what can be "executed in parallel when possible".

Alternatively, we could offer offer a way to place resources in manifest order sequence as if explicitly joined by relationships - that is, what you would need to do manually today in a public module if you care about the order. This could take the form of a function call performed in a class - for example:

class example {
  resources_in_sequence()
  notify { 'who': message => 'I am on first': }
  notify { 'what': message => 'I am on second': }
}

or

class example {
  with_resources_in_sequence() | | {
    notify { 'who': message => 'I am on first': }
    notify { 'what': message => 'I am on second': }
  }
}

Doing the opposite (parallel is ok) would work in analog.

Comment by R.I.Pienaar [ 2017/05/17 ]

Henrik Lindberg will that not recreate the issues with include/contain?

class with resources_in_sequence includes other class they don't control, now the whole thing is unknown again?

Generally I think MOAR is such a massive massive win on all levels for Puppet that introducing new hurdlers to it's use is a bad idea, you'd rather annotate which resources are parallel safe rather than which is not. In any way given that we (as a user) have never even considered parallel execution of resources that does seem to me the only viable option.

Comment by Henrik Lindberg [ 2017/05/18 ]

R.I.Pienaar it would be possible to parallelize what is contained, the contained is always anchored between the start and the end of what it is contained in, so it would not float away to the top.

I get the point about parallell being the exceptional case (especially since it does not exist (yet) )

Note, that MOAR defines the order per class/define & manifest - it does not perform a global ordering based on order of evaluation at compile time. Automatic parallel application can still be performed between unrelated resources.

I can see a hybrid MOAR guarantee + parallell execution opt in + explicit allow of parallell execution in a body/block as being possible to implement should we want to. It does not block our decision to turn on MOAR as the default/guarantee since the MOAR is only a local partial ordering.

Comment by Eric Sorenson [ 2018/08/29 ]

+1 to making this the only option; we made the ordering algorithm pluggable in https://projects.puppetlabs.com/issues/18508 , theoretically to allow different algorithms like a "fuzzer" random order. But nobody ever used this AFAIK and I agree with RI that it'd be good to have a guarantee that it's always on in Puppet 6 and above.

Comment by Jacob Helwig [ 2018/09/05 ]

Seems like if the only valid option will be "manifest", that we might as well add a deprecation to 5.5.x, and remove the setting entirely in 6.0.0. PUP-9107 is to deprecate in 5.5.x.

Comment by Jorie Tappa [ 2018/09/11 ]

merged to master at 2119235cc8f110b8f305fcfec5e589ab655a61c1

Comment by Melissa Stone [ 2018/09/17 ]

All three merge commits associated with this ticket are ancestors of the latest passing sha of puppet included in puppet-agent 5.99.2.337.gabeb904, i.e., this has passed ci

Comment by Scott Garman [ 2018/09/17 ]

Since we had a green run of CI puppet-agent 6.0.0 last night, I'm bulk-changing these issues from Ready for CI -> Resolved in preparation of the release.

Comment by Christopher Wood [ 2019/01/09 ]

As a post-hoc note, I do in fact have random ordering turned on. It is indeed an excellent fuzzer. However I think everybody at work here except for me will be delighted with this change.

 

[root@puppetmaster1 ~]# grep ^order /etc/puppetlabs/puppet/puppet.conf
ordering = random

Generated at Thu Jun 27 05:47:17 PDT 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.