[PUP-6397] File autorequire in Mount causes dependency loops Created: 2016/06/09  Updated: 2018/02/06  Resolved: 2016/08/19

Status: Closed
Project: Puppet
Component/s: Types and Providers
Affects Version/s: PUP 4.5.0, PUP 4.5.1
Fix Version/s: PUP 4.6.1

Type: Bug Priority: Major
Reporter: Thomas Equeter Assignee: John Duarte
Resolution: Fixed Votes: 1
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified
Environment:

Linux


Issue Links:
Blocks
Duplicate
is duplicated by PUP-6561 PUP-6099 breaks mounted filesystem pe... Closed
is duplicated by PUP-6601 Autorequire in mount.rb causes depend... Closed
Relates
relates to MODULES-7255 New attribute :manage_mountpoint for ... Accepted
relates to PUP-8106 "mount" type should autorequire its m... Resolved
relates to PUP-6099 Additional file and mount autorequire Closed
Template:
Acceptance Criteria:

It should be possible to:

$_instance_path = '/opt/tomcat_myinstance'
 
mount { $_instance_path:
  ensure => 'mounted',
  device => '/dev/foo',
  fstype => 'test',
}
 
tomcat::instance { 'myinstance':
  catalina_home => '/usr/share/tomcat6',
  catalina_base => $_instance_path,
  require       => Mount[$_instance_path],
}

Since Puppet 4.5.0, this fails with a dependency cycle:

Error: Failed to apply catalog: Found 1 dependency cycle:
(File[/opt/tomcat_myinstance] => Mount[/opt/tomcat_myinstance] => Tomcat::Instance[tomcat_myinstance] => File[/opt/tomcat_myinstance])

tomcat::instance is from puppetlabs/tomcat.

Story Points: 0
Sprint: Client 2016-08-24
Release Notes: Bug Fix
Release Notes Summary: Overly aggressive autorelationships between mount and file types have been scaled back

 Description   

One of the autorequires added to Mount in 4.5.0 causes dependency loops in our install. See Acceptance Criteria first for a sample use case.

The root cause is the File type assuming that the path is a unique identifier (namevar), while that's not true when Mounts are involved. PUP-6099 broke the workaround for that situation.

When /x is a mountpoint, there are actually two /x directories: the mountpoint on the parent filesystem (/x in the / filesystem, invisible once mounted) and the mounted directory (the root of the mounted /x filesystem). These two directories are different system objects and may have different owners/modes/etc.

The mountpoint is just a technical prerequisite to make mount work. What we usually want to manage in depth is the mounted directory. However, PUP-6099 makes Puppet only manage the mountpoint:

File[/x] -> Mount[/x]                    # Forced by PUP-6099
Mountpoint[/x] -> Mount[/x] -> File[/x]  # What we would want to do

With Mountpoint a (not yet written) lightweight File clone that just ensures directory existence and root-ownership. So far I used an Exec hack as a quick workaround.

Why it is important that File manages the mounted directory and not the mountpoint:

  • This is where the content lives and where we want the full power of the File type.
  • Doing otherwise breaks modules which manage the mounted directory as a File (see Acceptance Criteria).

Suggested course of action: roll back the autorequire (lib/puppet/type/mount.rb:301) until a resolution for the more general issue described above is available. At the very least, please make it optional.



 Comments   
Comment by Mihkel Ader [ 2016/07/05 ]

An even simpler example that fails with dependency loop in Puppet 4.5.x but works fine in 4.4.x.
Seems that the mount autobefore matches totally irrelevant "child" file resources?

file

{ '/tmp/home/test/': ensure => 'directory', }

->
file

{ '/tmp/home/test/test': ensure => 'file', }

->
mount

{ "/home": }
Comment by Kylo Ginsberg [ 2016/07/05 ]

I haven't had a chance to look at this one, but ping Matt Schuchard for possible insight (since you worked on the linked PUP-6099).

Comment by Matt Schuchard [ 2016/07/28 ]

The issue reported by Mikhail is apparently something we all missed at https://github.com/puppetlabs/puppet/blob/master/lib/puppet/type/mount.rb#L307. That regexp needs to be tightened (maybe all we need is a ^ at the beginning) and maybe a new spec test added for that situation he described.

As for the

File[/x] -> Mount[/x]                    # Forced by PUP-6099
Mountpoint[/x] -> Mount[/x] -> File[/x]  # What we would want to do

that is a good idea to me. Puppet needs something to differentiate between creating a directory and applying permissions to it since the mount action occurs between the two and the same File resource cannot be declared twice. Maybe add a new attribute to Mount that creates a directory if it is missing like with the managehome attribute in the User type?

Either way, funny how PUP-6099 ended up revealing both the gap in autorequire/autobefore spec testing and now the gap in file ensure vs. permissions and the relationship between directory creation, mounting, and directory permissions in Linux versus the expectations of Puppet.

Comment by Matt Schuchard [ 2016/07/28 ]

Proposal:

1. Remove the File['/foo'] -> Mount['/foo'] autorequire and its corresponding doc and spec test in favor of...
2. Add an attribute (name?) to the Mount type that creates the mountpoint directory if it does not exist, add doc for that attribute, and add spec test for that attribute. The mountpoint should have restrictive permissions as per the concerns of Paul Anderson below.
3. Tighten the regexp on the Mount['/foo'] -> File['/foo/bar'] autobefore and add a spec test for the issue Mikhail identified.
4 (optional). Add the Mount['/foo'] -> File['/foo'] autobefore into the regexp, add a doc for that new autobefore, and add a spec test for that autbefore

Addendum:

Paul Anderson suggested additionally changing the autorequire File['/foo'] -> Mount['/foo'] to an autobefore Mount['/foo'] -> File['/foo']. I have no opinion on this, so I invite people to weigh in. If yes, part 4 above is included.

Let me know how this sounds and I can start going after it.

Comment by Paul Anderson [ 2016/08/01 ]

I like Matt's Proposal because it would allow someone to describe the proper state of a mounted filesystem and have it implemented in a single puppet run.

I don't want to overthink the issue, but there's one potentially unresolved issue:

Should it be possible to set the permissions of the directory BEFORE it is mounted and the final permissions are set?

For instance, an admin may want to make sure that a directory has very restrictive permissions before it is mounted so that if the mounting were to fail, or if there were an unexpected ordering issue, files wouldn't get written to the local file system. Only after the mount happened would less restrictive permissions be set.

For instance:

Start of puppet run:
[root@node ~]# ls -lad /tmp/mnt
ls: cannot access /tmp/mnt: No such file or directory

Just before mounting:
[root@node ~]# ls -lad /tmp/mnt/
dr-x------ 2 root root 6 Aug 1 20:41 /tmp/mnt/

[root@node ~]# mount /tmp/mnt

Post mount, end of puppet run
[root@node ~]# ls -lad /tmp/mnt
drwxr-xr-x 2 staff staff 6 Aug 1 20:41 /tmp/mnt

Comment by David Kramer [ 2016/08/03 ]

We're going to rollback PUP-6099 as a short term solution, and add documentation to the mount type about this issue.

Comment by Matt Schuchard [ 2016/08/03 ]

No David, PUP-6099 cannot be rolled back. Please get with our TAM (Elizabeth) on the business need for it if you need to.
Please consider taking the proper route on this, especially because I am really going to be the one putting in the effort.
It would be good for the quality of Puppet as a software product if we put in the effort to improve it instead of just ignoring issues.
Please get someone (Kylo, Josh, Branan, etc.) to review the proposal so we can do the right thing here.
Thanks.

Comment by David Kramer [ 2016/08/03 ]

Thanks for the feedback, Matt. I'm going to pull this back into the Client Triage sprint until the team has a chance to discuss this further.

Comment by Matt Schuchard [ 2016/08/03 ]

I appreciate it David. Sorry if I seem testy about this, and I am trying not to be, but this is a major need for us and something I had to spend a while working on because I was ambitious by trying to forge into new Puppet territory with auto-require/before spec tests.

I don't understand the internal process you guys use, so maybe my 'why can't someone just look over my proposal and make suggestions as necessary before I work it' is overly simplified.

As soon as someone provides feedback I can go after this and it should just be quick to bang out. What might take longer is constructing the spec test for what Mikhail mentioned.

Also, I need to go update the proposal a little based on something Paul Anderson said.

Comment by Branan Riley [ 2016/08/03 ]

Matt Schuchard David is taking over managerial responsibilities for our team from Kylo. I was in the room when we decided to revert this, as was Eric Sorenson

To be clear, I think we only need to revert the autorequire portion of PUP-6099. The autobefore portion can stay as-is.

For some context, we've frozen Puppet 4.6.0 at this point. I would call your proposal a Y-release new feature, rather than a Z-release bugfix. That means it would have to wait for 4.7, which is probably about a quarter out if our release cadence doesn't change.

That leaves us with a few super shitty options:

1) Remove the autorequire as a bugfix in 4.6.1, since it broke users, and get the new plan out in 4.7.0
2) Push your changes in 4.6.1, as a "bugfix" to this feature
3) Let it all sit until 4.7.0

None of these are great. Number 2 is playing games with semver, which I don't like. 3 is pretty obviously the worst overall

Of these, 1 is the most palatable to me. /somebody/ is going to suffer no matter what we do here, and I tend to err on the side of "Pull out something that broke users, and get the correct solution in ASAP after that". In this case "ASAP" is likely 4.7.0

Comment by Matt Schuchard [ 2016/08/03 ]

Thank you for the background info Branan.

The autorequire part is not necessary for us and the autobefore is. That issue Mikhail brought up is, to me, much more alarming than the issues raised by Paul and Thomas. Part 3 of the proposal deals with it and can be considered a bugfix. While what Mikhail raised is not affecting myself or my company personally, I want to fix that because it bothers me out of personal responsibility and integrity.

So I agree completely that 1 is the most palatable. What if we made changes 1 and 3 in my proposal for 4.6.1 (they are bugfixes), and then changes 2 and possibly 4 (depending upon feedback) for 4.7.0 (they are a new feature and improvement respectively)?

I can start on 1 and 3 for 4.6.1 tomorrow morning.

Comment by Branan Riley [ 2016/08/03 ]

That sounds perfect.

Thanks for understanding, and especially thank you for being willing to take on the work.

Comment by Matt Schuchard [ 2016/08/04 ]

I have something in place for 1 and 3 now. Once 4.6.0 is cut, I can branch off 4.6.1, redo the changes against that branch, and then PR.

Comment by Kylo Ginsberg [ 2016/08/04 ]

That's awesome Matt Schuchard - thanks for jumping on that! I suspect 4.6.0 will be tagged in the next day or two, so hang tight.

Comment by Matt Schuchard [ 2016/08/13 ]

PR

I forgot the header in the commit messages so I need to amend and force push, but the code appears to be solid.

Comment by Matt Schuchard [ 2016/08/18 ]

Followup PUP-6637.

Comment by John Duarte [ 2016/08/19 ]

Validated using pre-release version of puppet-agent at SHA cd3af7c containing puppet at SHA 738a8d7.

Using the manifest from the acceptance criteria of this ticket, the catalog no longer results in a cyclic dependency.

root@tfoxaebkvjclhab:~# cat pup6397.pp
$_instance_path = '/opt/tomcat_myinstance'
 
mount { $_instance_path:
  ensure => 'mounted',
  device => '/dev/foo',
  fstype => 'test',
}
 
tomcat::instance { 'myinstance':
  catalina_home => '/usr/share/tomcat6',
  catalina_base => $_instance_path,
  require       => Mount[$_instance_path],
}
root@tfoxaebkvjclhab:~# puppet apply pup6397.pp
Notice: Compiled catalog for tfoxaebkvjclhab.delivery.puppetlabs.net in environment production in 0.48 seconds
Notice: /Stage[main]/Main/Mount[/opt/tomcat_myinstance]/ensure: ensure changed 'unmounted' to 'mounted'
Error: /Stage[main]/Main/Mount[/opt/tomcat_myinstance]: Could not evaluate: Execution of '/bin/mount /opt/tomcat_myinstance' returned 32: mount: unknown filesystem type 'test'

Generated at Sun Aug 18 00:47:35 PDT 2019 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.