an already mounted bind mount get wrong opts in fstab

[disaster123]

Hi,

if a bind mount is already mounted it get's the wrong options in fstab.

Description of Issue/Question

state file:

mount /home/tmp:
  mount.mounted:
    - name: /home/tmp
    - device: /home/tmp
    - fstype: none
    - opts:
      - bind
      - nodev
      - noexec
      - nosuid
      - rw
    - persist: True

resulting fstab entry:
grep home/tmp /etc/fstab

/home/tmp               /home/tmp       none    noexec,rw,inode64,bind,swidth=9216,logbsize=256k,attr2,nodiratime,usrquota,sunit=512,nosuid,noatime,nodev   0 0

expected entry would be:
/home/tmp /home/tmp none bind,nodev,noexec,nosuid,rw 0 0

this only happens if i did
mount -o bind,nodev,noexec,nosuid,rw /home/tmp /home/tmp

before running salt. The other options i see are options "copied" from the root fs.

Versions Report

Salt Version:
Salt: 2016.11.2

Dependency Versions:
cffi: Not Installed
cherrypy: Not Installed
dateutil: 2.2
gitdb: Not Installed
gitpython: Not Installed
ioflo: Not Installed
Jinja2: 2.9.4
libgit2: Not Installed
libnacl: Not Installed
M2Crypto: Not Installed
Mako: Not Installed
msgpack-pure: Not Installed
msgpack-python: 0.4.2
mysql-python: 1.2.3
pycparser: Not Installed
pycrypto: 2.6.1
pygit2: Not Installed
Python: 2.7.9 (default, Jun 29 2016, 13:08:31)
python-gnupg: Not Installed
PyYAML: 3.11
PyZMQ: 14.4.0
RAET: Not Installed
smmap: Not Installed
timelib: Not Installed
Tornado: 4.2.1
ZMQ: 4.0.5

System Versions:
dist: debian 8.6
machine: x86_64
release: 4.4.26+76-ph
system: Linux
version: debian 8.6

[Ch3LL]

@disaster123 for the initial mount did you have all of those options set? I wonder if its grabbing the options you already had set?

[disaster123]

@Ch3LL no i did not. But yes it seems it grabs all those options from mount or /proc/mounts - but this is wrong. I see two reasons why this is wrong.

1.) For bind mounts /proc/mounts contains afterwards also the options of the mounpoint behind the mount point but they should never be part of the bind mount itself.
2.) /proc/mounts also contains default options from the currently bootet kernel. If salt grabs from there it may change kernel defaults when switching kernels

[Ch3LL]

@disaster123 thanks for clarifying that. Yeah agreed we definitely want to only set what you have specified. I haven't been able to replicate this using the following test case:

1. sudo mount -o bind /home/tmp /home/tmp
2. run your state file
3. /etc/fstab looks like:

/home/tmp /home/tmp none bind,nodev,noexec,nosuid,rw 0 0

Do you know what your original mount command was.

In terms of a fix for you I think the part of the code we want to focus in is https://github.com/saltstack/salt/blob/v2016.11.2/salt/states/mount.py#L211

Looks like its running __salt__['mount.active'](extended=True) and adding opts depending on that output

If you run that command on your initial mount before running the state file what does it return?

[disaster123]

To me it looks different than yours.

Here's the output:

# grep home/tmp /proc/mounts 
# 

# mount -o bind /home/tmp /home/tmp           
# grep home/tmp /proc/mounts       
/dev/dm-0 /home/tmp xfs rw,noatime,nodiratime,attr2,inode64,usrquota 0 0

# salt-call state.apply             
local:
----------
          ID: mount
    Function: 012./home/tmp
        Name: mount /home/tmp
      Result: True
     Comment: Target was already mounted. Added new entry to the fstab.
     Started: 20:05:38.695657
    Duration: 109.856 ms
     Changes:   
              ----------
              persist:
                  new
              umount:
                  Forced remount because options (nodev) changed

# grep home/tmp /proc/mounts         
/dev/dm-0 /home/tmp xfs rw,nosuid,nodev,noexec,noatime,nodiratime,attr2,inode64,usrquota 0 0
# grep home/tmp /etc/fstab 
/home/tmp               /home/tmp       none    noexec,rw,inode64,bind,attr2,nodiratime,usrquota,nosuid,noatime,nodev   0 0


Sorry don't know how to execute __salt__['mount.active'](extended=True) via salt-call
# salt-call "__salt__['mount.active'](extended=True)"
'__salt__['mount.active'](extended=True)' is not available.

[disaster123]

@Ch3LL hope this helps. If you need more information. Please ask.

[Ch3LL]

@disaster123 thanks for including all the information that really helped. Looks like indeed it is including the options on the pre-mount. I think in my test case I was looking for the extra opts you were getting, but i'm not using xfs. So thanks for helping out with that.

 ch3ll@thecakeisalie  ~/git/salt  ➦ 2786e20 <B>  grep home/tmp /proc/mounts 
/dev/mapper/fedora-home /home/tmp ext4 rw,seclabel,relatime,data=ordered 0 0

 ch3ll@thecakeisalie  ~/git/salt  ➦ 2786e20 <B>  grep home/tmp /proc/mounts
/dev/mapper/fedora-home /home/tmp ext4 rw,seclabel,nosuid,nodev,noexec,relatime,data=ordered 0 0

    - opts:
      - bind
      - nodev
      - noexec
      - nosuid
      - rw

The point of the code I pointed to earlier is indeed where it takes the current options and appends whats in the state. I think a good approach would be to add an argument that does not do this. I did not see an argument like that in the code base. So I'll approve this as a feature request.

As a side note about __salt__. I was definitely not very clear on this. If you look at our docs here you can see that salt is a dunder dictionary within the code. It gives you the ability to call an execution module. So what I was looking for was salt-call --local mount.active extended=True But I do not need that information anymore. Thanks

[disaster123]

@Ch3LL thanks for your further testing and reply. If this is a feature request how to solve this for 2016? The current state does not work for me. It includes wrong options i do not need nor want for bind.

[disaster123]

@Ch3LL any news? I still don't how how to fix this for 2016.11?

[Ch3LL]

It seems this was written with intended behavior so I added as a feature request to add a new argument to not add current options if its already mounted. If its a feature it would not be added to 2016.11 but to develop and then branched off into the next feature release. If this was intended behavior we could not change the default behavior in a bug fix release.

As a workaround does it work if you unmount before mounting using mount.unounted?

[mstarostik]

Any news on this?
I'm trying to use mount.mounted to set up a bind mount, but I think I'm bitten by this issue or a variant thereof with 2017.7.2.

If the bind mount is not mounted and not in /etc/fstab, mount.mounted works great. However, if it is already mounted before (even via the very state), it gets remounted and all the superopts appended to the line in /etc/fstab. On subsequent state runs, there will be no change.

I can see from states/mount.py how this is intended behaviour, but it's pretty much not what I'd want. I now have "relatime,rw,inode64,seclabel,bind,noquota,attr2" in fstab, of which "bind" is the only option that should be there. I simply do not want the superopts to make it into fstab.

[profihost]

+1 for 2016.11

[Ch3LL]

@mstarostik no updates on this right now. We are willing to accept PRs

ping @garethgreenaway looks like you have done some work in this code. Any chance you have time to look at this?

[garethgreenaway]

@Ch3LL need to review this issue closer but I believe it should be addressed by a PR I made to develop a few months back that will be available in the oxygen release.

[hoonetorg]

uh, oh d@mn, looks like a lineinfile is required in my formula, to workaround this
any news?

[Ch3LL]

can you clarify what you are running into @hoonetorg

are you using the oxygen release that @garethgreenaway mentioned this might be fixed in?

[disaster123]

The code is still there in 2019.2:

mount.py:
246         if 'bind' in opts and real_name in active: 
247             _device = device 
248             if active[real_name]['device'].startswith('/'): 
249                 # Find the device that the bind really points at. 
250                 while True: 
251                     if _device in active: 
252                         _real_device = active[_device]['device'] 
253                         opts = list(set(opts + active[_device]['opts'] + active[_device]['superopts'])) 
254                         active[real_name]['opts'].append('bind') 
255                         break 
256                     _device = os.path.dirname(_device) 
257                 real_device = _real_device 
258             else: 
259                 # Remote file systems act differently. 
260                 if _device in active: 
261                     opts = list(set(opts + active[_device]['opts'] + active[_device]['superopts'])) 
262                     active[real_name]['opts'].append('bind') 
263                 real_device = active[real_name]['device'] 

Whats the reason behind it? It makes absolutely no sense to copy the options from the real fs for a bind mount.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[disaster123]

no this issue is still present

[stale]

Thank you for updating this issue. It is no longer marked as stale.

[sagetherage]

@disaster123 if we take this on to do it would be in a newer release, AFAIK. I can take this up as ensuring clarification on that front, but wanting to make sure I am being transparent and clear with you. Assigning myself to investigate.

[disaster123]

@sagetherage Why? This is def. a bug not a feature. It makes absolutley no sense. Can you point to a usecase?

[sagetherage]

@disaster123 I am merely the messenger, so bare with me as I attempt to get more information and attempt to move this along.

[disaster123]

OK thx please tell me how i can help

[Ch3LL]

seems my initial investigation that this was intended behavior is incorrect, i'll change to a bug issue thanks.