FEATURE :: Make some freebsd-specific modules jail and/or chroot aware

[mamalos]

Description of Issue/Question

freebsdservice is't aware of jail and/or chroot. Moreover, a freebsd-update module seems to be missing.

Details

I have a few servers running multiple jails each and within these jails most filesystems are mounted ro and only those that need to be mounted rw are mounted rw. In real life, when I want to upgrade their base and/or userland, I just keep a nullfs rw mountpoint of those filesystems available on the host and work on it. This procedure is well documented in paragraph 14.5 of the FreeBSD handbook:

I wanted to try using salt for automating the management of these servers, and I realised that if I want to do it, I will need to write custom code for upgrading my jails' packages, even though in reality I'd just use the same commands that I've been using for the host, only this time I'd just provide the appropriate arguments (this holds for pkg and freebsd-update, for restarting services one has to use jexec). I think that adding this functionality on the existing functions won't be that difficult.

More specifically, I gave a quick look in freebsdpkg's code and I think it wouldn't be that difficult for most functions to accept a couple of additional positional arguments -like jailid, jailname or chroot- and change their behaviour accordingly. In fact, I would be very happy to contribute this code, but before doing so I would like to know that: (1) you agree, and (2) that I won't be breaking any specific interfaces when doing so. For example, freebsdpkg's *file_list(packages) accepts 0 or more packages as its arguments; if changing it to accept an additional 0 or more keyword arguments is not that bad (eg. **file_list(*packages, kwargs)), then I think I'd be happy to contribute! :)

Any comments and thoughts are more than welcome!

Thanks in advance,

George.

EDIT

Sorry, it seems that pkgng module, which replaces freebsdpkg in FreeBSD versions >=9.0, is aware of chroots and jails. So, what remains is to add jail awareness in freebsdservice (most probably by using jexec wrappers and/or providing jails' paths where appropriate) and then the discussion about adding a freebsd-update module that's also jail and chroot aware (if it doesn't exist already).

[gtmanfred]

Thanks for reporting this George.

It looks like you have a handle on what needs to be changed, would you be able to provide a PR to fix the behavior that you would like to see here? Otherwise this is tagged with our core team.

Thanks
Daniel

[mamalos]

Hi!

Yes, I'd be glad to give it a try!

The only thing is that I'm very new to salt so I'm wondering if I may be missing things that are already implemented (as you saw from my initial message, at first I thought that pkg was not behaving right, just because I looked at a previous FreeBSD package-management module's code). Moreover, I think I should get a bit more well acquainted with the modules' structure. Is there maybe a guide for developers? OK, I can understand most stuff by reading the code, but if there's a quick guide explaining salt's modules architecture, I'd be glad to read it before providing a PR :).

So, architecturally for example, should I deduce that a freebsd-update equivalent module is missing because Linux doesn't have the notion of "world" and "userland" and everything that involves an update or upgrade is usually through packages? And if so, would adding a module for this functionality also affect some other module that for example is supposed to "update the whole system"?

Whatever your answer is, I'll start giving it a try from Monday on (starting with the service module which seems easier) and see how it goes.

PS. In case I need help with the code, what is the best place to discuss about it? IRC (and if so, which channel? saltstack or salt?)? The mailing list? Github issues list?

[gtmanfred]

The best place to discuss this issue is here, so that it can be recorded and we can pass it around to different developers to get input.

There is a salt-users mailing list that is also good. And #salt on freenode is good for getting feedback/help/ideas

As for why freebsd-update doesn't exist, I am not sure what that would entail, but I would say write it and submit a PR with it, and see what happens :)

Here is our contributing guide for salt https://docs.saltstack.com/en/latest/topics/development/contributing.html

[mamalos]

OK, I've made a pull request (link provided just above this thread) for freebsdservice module. Sorry, I haven't written any tests, but I will do so shortly; nonetheless, I have tested each function on a real system and nothing seems to be broken (both for a jail and for the host system). I've tried to update docstrings as well.

I will come back to this issue again when I'll have more news, hope it's OK :).

[gtmanfred]

Looks good, I have added closes #36675 to the pr to close this issue when it is merged to develop

[mamalos]

Hmm...Shouldn't the issue remain open until I submit code for freebsd-update as well? Or should I open a new one?

[gtmanfred]

That works, it can wait until after the update module is there too.

[abednarik]

Hi Guys

We are testing v2017.7.1 and look like cc2b85c introduced a bug.

Running a simple state to ensure a service is running inside FreeBSD fails because after the above change looks like salt assumes that we are running that inside a jail when is not the case.

[INFO    ] Executing command '/usr/sbin/jexec None /usr/sbin/service -l' in directory '/home/vagrant'
       [ERROR   ] Command '/usr/sbin/jexec None /usr/sbin/service -l' failed with return code: 1
       [ERROR   ] output: jexec: jail "None" not found
       [ERROR   ] The named service sshd is not available
       [INFO    ] Completed state [sshd] at time 12:28:16.619401 duration_in_ms=16.581

[root@cronmaster-freebsd-103 /usr/local]# service sshd onestatus
sshd is running as pid 639.

I think here None means this is not a jail bot a jail name.

We get the same errors in all services. I think this issue is critical if this is confirmed.

@mamalos @gtmanfred

thanks in advance.

[woodsb02]

I am having the same issue as @abednarik has reported here.

Troubleshooting by adding some additional "log.info" lines in the freebsdservice.py code has revealed that

• the "jail" variable has the type NoneType in the "get_all" function
• when the "get_all" function calls the "_cmd" function, the "jail" variable has the type str within the "_cmd" function

After a lot of troubleshooting, I have established that this is caused by the following line in freebsdservice.py:
@decorators.memoize
https://github.com/saltstack/salt/blob/v2017.7.1/salt/modules/freebsdservice.py#L42

Removing this line fixes the issue.

This can be repeated with a simple standalone python program.

#!/usr/local/bin/python2.7
import salt.utils.decorators as decorators

@decorators.memoize
def _cmd(jail=None):
    print "Value of jail:",jail
    print "Type of jail:",type(jail)
    if jail:
        print "if jail = true"
    if jail is None:
        print "if jail is None = true"
    if jail is not None:
        print "if jail is not None = true"

def test(jail=None):
    print "Value of jail:",jail
    print "Type of jail:",type(jail)
    if jail:
        print "if jail = true"
    if jail is None:
        print "if jail is None = true"
    if jail is not None:
        print "if jail is not None = true"
    jail=jail
    _cmd(jail)

print "Hello World!"
test()

So the question which remains is how this should be fixed. Should we be removing the decorators.memoize and the decorators import line from freebsdservice.py or should we be fixing the memoize code to not convert a NullType to a str?

[mamalos]

Hi guys,

And sorry for the delayed answer, but unfortunately (or fortunately? :) ) I'm on vacation with limited Internet access. On Tuesday morning (EEST) I'll be back in my office, so I'll try correcting it then, if it's not that critical. I already see some workarounds, but it's a long time since I've written this code so it'd be much better if I gave it a more thorough look before submitting a corrcection.

In short, I see no reason why memoize decorator should be removed, We'll just have to check what goes "wrong" with that decorator and returns a string instead of None. But, again, it'd be much safer to give it a closer look before submitting a PR.

Thanks both for informing!
George.

[mamalos]

OK, the problems arose after issue #40937 (with pr #41014). Even though I was asked as a reviewer if anything might break, I didn't see it coming, hence this bug. I'll ask for issue #40937 to be reopened and will suggest/discuss a cleaner solution.

[abednarik]

Thanks for the update @mamalos.

Cheers.

[mamalos]

Hmmm...after looking into my code (and memoize's code) a little deeper, I think that the memoize decorator is not needed in our case -or better say I think that using might be wrong. I'll give it a more thorough look tomorrow morning with a cleaner head and will submit the PR.

The good thing, on the other hand, with this bug is that it disclosed a few other issues about decorator memoize that should be corrected, so thanks again for the help!

[mamalos]

Ah, this bug has been removed from branch 2017.7 with @cro's fix in a260e91 about 3 weeks ago. I suppose that 2017.7.1 should merge this fix?

[gtmanfred]

2017.7.1 was a small bug fix release of specific bugs, so it doesn't look like this one was included in it. It should be in 2017.7.2

…

On Wed, Aug 23, 2017 at 7:35 AM, George Mamalakis ***@***.***> wrote: Ah, this bug has been removed from branch *2017.7* with @cro <https://github.com/cro>'s fix in a260e91 <a260e91> about 3 weeks ago. I suppose that *2017.7.1* should merge this fix? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36675 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAssoePUG65h2iyUFRZfR7bkiPotbOg5ks5sbCqjgaJpZM4KKBDM> .

[abednarik]

Thamks for the info.

[mamalos]

Guys, from what I understand we should close the issue, since this bug is only found in 2017.7.1 branch and is not present in the main 2017.7 branch.