(Partially Fixed) Emerge config reading badly broken

[kaithar]

Kinda-regression for #8252

2014-01-15 21:55:25,648 [salt.minion                                 ][WARNING ] The minion function caused an exception
Traceback (most recent call last):
  File "/usr/lib64/python2.7/site-packages/salt/minion.py", line 747, in _thread_return
    return_data = func(*args, **kwargs)
  File "/usr/lib64/python2.7/site-packages/salt/modules/state.py", line 383, in sls
    ret = st_.state.call_high(high_)
  File "/usr/lib64/python2.7/site-packages/salt/state.py", line 1689, in call_high
    ret = self.call_chunks(chunks)
  File "/usr/lib64/python2.7/site-packages/salt/state.py", line 1411, in call_chunks
    running = self.call_chunk(low, running, chunks)
  File "/usr/lib64/python2.7/site-packages/salt/state.py", line 1615, in call_chunk
    running = self.call_chunk(chunk, running, chunks)
  File "/usr/lib64/python2.7/site-packages/salt/state.py", line 1615, in call_chunk
    running = self.call_chunk(chunk, running, chunks)
  File "/usr/lib64/python2.7/site-packages/salt/state.py", line 1529, in call_chunk
    self._mod_init(low)
  File "/usr/lib64/python2.7/site-packages/salt/state.py", line 565, in _mod_init
    mret = self.states[minit](low)
  File "/usr/lib64/python2.7/site-packages/salt/states/pkg.py", line 924, in mod_init
    ret = __salt__['pkg.ex_mod_init'](low)
  File "/usr/lib64/python2.7/site-packages/salt/modules/ebuild.py", line 174, in ex_mod_init
    __salt__['portage_config.enforce_nice_config']()
  File "/usr/lib64/python2.7/site-packages/salt/modules/portage_config.py", line 72, in enforce_nice_config
    _order_all_package_confs()
  File "/usr/lib64/python2.7/site-packages/salt/modules/portage_config.py", line 89, in _order_all_package_confs
    _package_conf_ordering(conf_file)
  File "/usr/lib64/python2.7/site-packages/salt/modules/portage_config.py", line 188, in _package_conf_ordering
    append_to_package_conf(conf, string=line)
  File "/usr/lib64/python2.7/site-packages/salt/modules/portage_config.py", line 255, in append_to_package_conf
    atom = _p_to_cp(atom)
  File "/usr/lib64/python2.7/site-packages/salt/modules/portage_config.py", line 54, in _p_to_cp
    ret = _porttree().dbapi.xmatch("match-all", p)
  File "/usr/lib64/python2.7/site-packages/portage/dbapi/porttree.py", line 825, in xmatch
    mydep = dep_expand(origdep, mydb=self, settings=self.settings)
  File "/usr/lib64/python2.7/site-packages/portage/proxy/objectproxy.py", line 31, in __call__
    return result(*args, **kwargs)
  File "/usr/lib64/python2.7/site-packages/portage/dbapi/dep_expand.py", line 35, in dep_expand
    mydep = Atom(mydep, allow_repo=True)
  File "/usr/lib64/python2.7/site-packages/portage/dep/__init__.py", line 1255, in __init__
    raise InvalidAtom(self)
InvalidAtom: #

I threw an exception throw in just above append_to_package_conf(conf, string=line) in salt/modules/portage_config.py:188:_package_conf_ordering to determine what's going on...

  File "/usr/lib64/python2.7/site-packages/salt/modules/portage_config.py", line 186, in _package_conf_ordering
    raise(Exception(repr(rearrange)))
Exception: ['# required by salt (argument)\n', 'app-admin/salt **\n', '# required by app-admin/salt-0.16.4\n', '# required by salt (argument)\n', '=dev-python/pyyaml-3.10-r1 ~amd64\n', ...]

To be sure, I went down a frame:

    if conf in SUPPORTED_CONFS:
        if not string:
            # Snipped
        else:
            raise(Exception(string))
            atom = string.strip().split()[0]
            new_flags = portage.dep.strip_empty(string.strip().split(' '))[1:]
            if '/' not in atom:
                atom = _p_to_cp(atom)
                string = '{0} {1}'.format(atom, ' '.join(new_flags))
                if not atom:
                    return

And sure enough:

  File "/usr/lib64/python2.7/site-packages/salt/modules/portage_config.py", line 188, in _package_conf_ordering
    append_to_package_conf(conf, string=line)
  File "/usr/lib64/python2.7/site-packages/salt/modules/portage_config.py", line 252, in append_to_package_conf
    raise(Exception(string))
Exception: # required by salt (argument)

I'm calling it a regression as I wasn't seeing this on 0.17.4, but the patch applied in PR #8924 doesn't actually address this particular case. I'd guess that some other change has resurrected this bug.
After a lot of digging, I've traced a large part of the issue to this section of salt/modules/portage_config.py (around L157):

        for triplet in os.walk(path):
            for file_name in triplet[2]:
                file_path = '{0}/{1}'.format(triplet[0], file_name)
                cp = triplet[0][len(path) + 1:] + '/' + file_name

                shutil.copy(file_path, file_path + '.bak')
                backup_files.append(file_path + '.bak')

                if cp[0] == '/' or cp.split('/') > 2:
                    rearrange.extend(list(salt.utils.fopen(file_path)))
                    os.remove(file_path)

So, if you have straight files, os.walk won't yield and this entire block gets skipped. If you have directories like me:
path == "/etc/portage/package.accept_keywords"
triplet == ('/etc/portage/package.accept_keywords', [], ['salt'])
file_path = "/etc/portage/package.accept_keywords/salt"
triplet[0][len(path) + 1:] == ""
cp == "/whatever_keywords_file"

Meaning, by my reading, that that last comparison won't fail unless you have nested folders.

Not to mention that the os.remove call deletes the original file and a further os.remove further down deletes the backup ... but the call to recreate the original isn't called so far as I can see.
So as is, it looks like emerge support is broken and actively nuking config files?

[kaithar]

To clarify, I'm seeing this on a minion after trying both 0.17.4 and 2014.1.0-379-ga2de041 ... I would imagine the official RC also has it.

I reproduced it on another minion also running 2014.1.0-379-ga2de041

The master is 0.17.4, though I don't think that's relevant in this case.

[kaithar]

Having studied the code further and figured out the interaction, I'm noting my original post has some errors.

I found the writing code down in append_to_package_conf at line 280, seeing the purpose is for merging configs in to appropriately named files (which explains the cp comparison I noted).
I can't figured out how this could actually handle comments at all... it derives the file to write to from the atom, comments don't have an atom so where would it write to?

I don't know what's the proper way to fix this is, but for a temp fix I'm going to inject a config option in to ebuild.py's ex_mod_init and default it to off. This mechanic could well annoy people when it totally rewrites their /etc/portage

Edit: Now I see why I've not encountered it before... it was in the 0.17 release but not in the 0.16 release.
I can see the PR is #5824 ... I'm really not happy that this was merged as a default behaviour; It's extremely invasive, it kicks in even if you don't want it to manage those files, as I've discovered it's very fragile... I'm not sure what the official stance is, but my opinion is that software shouldn't make radical changes unless asked to.
I would like some feedback on this.

[cachedout]

Hi @kaithar. Thanks for reporting this and for the excellent detective work here!

I'm not a Gentoo user so I don't have an enlightened view of what the correct behavior should be here. I am going to move this issue into the list of open bugs so that we can start to get some feedback from others on how this might work.

[basepi]

@kaithar None of the core devs are gentoo users, so we're not very opinionated on the subject. Perhaps that was too invasive and should not have been merged. Seems like the invasive behavior could probably be hidden behind a configuration option and that would allow everyone to be happy. Thoughts?

[kaithar]

@basepi that's what I'm intending, yes, defaulting to off (though it obviously won't revert previous merges), and properly documented.

The only analogy I could come up with is a media player deciding to silently reorganise your mp3 collection when you only asked it to play one of the tracks... that's not the best analogy but I think it conveys the right notion.

One of the main principles of Gentoo is that of offering choices where ever possible, even to the point of having you choose what crontab package to use. That's part of why it's such a horrible learning curve unfortunately. As a result, I'm not aware of an "official" layout for the folders. The site docs mention they can be files or folders and suggest you could use one file per package. It also suggests checking the man-page. Thus the portage man-page is probably the most authoritative (and detailed) source and has this to say:

       /etc/portage/
              Any file in this directory that begins with "package." can be more than just a flat file.  If  it  is  a
              directory,  then  all the files in that directory will be sorted in ascending alphabetical order by file
              name and summed together as if it were a single file.

              Example:
              /etc/portage/package.accept_keywords/common
              /etc/portage/package.accept_keywords/e17
              /etc/portage/package.accept_keywords/kde

So basically, every user does what feels right to them. For my salt controlled nodes I've gone the route of having a file for each state named after the state that created it.

As noted elsewhere, portage automatically generates comments along with new atoms when the user uses the --autounmask-write option. One of the more extreme examples looks like this:

# required by sys-libs/libselinux-2.1.13-r4
# required by sys-process/psmisc-22.20
# required by sys-apps/openrc-0.11.8
# required by virtual/service-manager-0
# required by @system
>=dev-libs/libpcre-8.33 static-libs

[kaithar]

Should I make a separate PR for 2014.1, or can this be cherry picked over?

[kaithar]

Sorted :)

[kaithar]

I've directed @faust here, as the original author of the code involved, as I'd like the original problem resolved.
To clarify, there's no bad feelings from my side, the intent is good even if the config and docs were lacking :)

[faust]

Thank you for pointing me here. Actually there were some doubts about the original PR (#5824), but then it was merged before finding out a better solution. The main problem is that Gentoo allows users to build up silly directory trees for configuration files. Reading values is not a problem, but when you have to modify or to add something it can be a little bit difficult to find out what file you have to modify. For this reason I choose to "enforce" a "nice" (for salt) configuration. I know that it is not the best solution, but I based my decision on the assumption that if you are using salt it means that you won't manage your server manually. So I thought that it doesn't matter if your configuration tree structure is reorganized as a far as it is hidden behind salt states. Of course this assumption does not have general validity.
I'll try the take a look to the new code to find out a way to obviate the "enforce nice config" thing.

[ribx]

Just as a small update:

'portage_config.enforce_nice_config' still aborts with errors if there are

1. empty lines
2. comments starting with '#'
   in files in /etc/portage/package.use/

I don't know yet about the other dirs, but I will dig into that (and hopefully provide a solution soon).

[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.