-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[openSUSE] Rock-on service does not stop correctly #2088
Comments
I thinks that's just fine. We are customising a tad anyway and if your sure of the KillMode modification I say we go for it. As for regressions we are dealing with 3 moving targets, but at least we have the pending testing channel to help with just this kind of think: just need to get one or two more things lined up ready. |
@FroggyFlox Nice find and proposed fix by the way. And again, quite timely. |
I've never messed with that option before and only found out about it while trying to debug that issue. That made me learn a lot about a service file configuration, actually... I'll need to test more of course and see if it breaks anything else down the line but it seems to be OK so far; this is the KillMode we use currently in CentOS, after all. I just need to make sure there's not a very good reason why the package comes with the The other option would be to do without our docker-wrapper but that seems like a more risky route, especially given I'm not particularly familiar with its reasons and history. I'll try to go on with changing the KillMode and test that out, then. |
@phillxnet , a quick update after a brief investigation: As I was trying to see why the default systemd
If we keep It seems to me that its only purpose is to pass along our Rockstor-specific customizations (
Interestingly, if we simply use this file, we may fix our current issue we have in both openSUSE flavors re-docker settings being defined in both the
We still have customizations to make in the |
@FroggyFlox Re:
Yes, I was considering this, if the quick fix you suggested earlier is a no go. Which it looks to be here.
Exactly, that way we go with the flow, and edit that file instead for our logging, driver, etc. Docker has moved on some since Rockstor first adopted it and maybe, as you suggest it's time we adapted to that via embracing the daemon.json. But we may still need to set dependencies up as we are responsible for mounting the Rock-ons root and then starting docker. So we just have to ensure we add our appropriate service as a dependency to the docker service file so that it waits for us to mount it's required subvols.
Yes, again going with the flow of change. But I'd rather do this only with our openSUSE offerings.
I think you have it. I've only dabbled with that element of the code of late so the wrapper will need to be fully understood but I definitely think you are on the right track. If your game to steam in we can check the results. Again if done in openSUSE only then we only affect the testing channel anyway, which of course is what it's for. Let me know if you are game to take this one on as it looks like you are far further along than I anyway. I definitely think we can pass more work back to systemd here and go the default config route for docker. Nice call. I think our current wrapper was initially, as you say, just to pass our customisation as docker was a lot younger at the time (in computer years). So looks like we are all turned around on this one, but it also looks to be a good thing. We may end up with a code simplification which would be nice, at least on the openSUSE side. Note here if you are game to continue tackling this one as it's a key one for our soon to be release installer / testing channel. |
Thanks a lot for a prompt, yet exhaustive feedback, @phillxnet! I will of course try to tackle that one, especially considering it is fresh in my head. I'll still need to make sure I'm not missing one of the functions of the docker_wrapper, but what you described related to its use actually fits what I thought so it makes a lot of sense. I'll thus make sure to keep the systemd customizations to their minimum and switch as much as I can to the Going back to this issue, I will, of course, only make change to openSUSE flavors as I'm not even sure our docker version in CentOS would support this way of config anyway. Thanks again, and hopefully I'll have a PR ready very soon. |
Just a quick mention about this: I was thinking of a use of such feature for users who need to use a different runtime than the default one, such as the nvidia one, which has come up a couple of time in the forum. That's a rather advanced feature, but it could be of use for some and thus worth considering in the future. |
I now have an implementation seemingly working in Leap15.1 (see https://github.com/FroggyFlox/rockstor-core/tree/WIP_Docker_KillMode):
Both this issue and #2032 thus seem fixed with this branch. Now that we have proof of concept, I will proceed with refining, writing tests, and further testing the new logic. |
@phillxnet, may I have your feedback on the following? In this new branch, I currently load the default daemon.json file (located at The risk, however, is to also carry a change in this config that may be incompatible with the options we need to set. An example of that actually already happened with the default config file having In this context, I see two options:
The more I think about it, the more I prefer option 2, as I see reliability has having priority over being up-to-date. Also, I would be surprised if this default config file changes that often (if at all). Moreover, as mention above, we can even implement the possibility for users to add their own custom settings in the future if we'd like. Thanks in advance for your feedback! |
@FroggyFlox I favour number 1. We update the current config file. Post initial install any package updates should leave it be anyway is my understanding. Although a package update may end up dumping another ineffectual file their for reference but I don't think it will change our config, once established.
Given the above assumption of mine that a package update won't change this file if it has already been changed, do we not avoid a potential package breakage by this means?
I'd shy away from this one myself as once we've changed this file I'm assuming the package manager will not change it in an update.
I'm currently favouring 1, but with the caveat that once we have made our changed the file is ours anyway. And if we want to support custom changes by users then with 2 will overwrite those changes: where changes are made directly to the file that our not ones we inline edit. So rather unusually we have different selections. Could it be that once we establish that a package update will leave our edited json unchanged we may be in agreement. As you say this file may not change a howl lot anyway and is unlikely to receive critical functional updates. A point on 1:
Your call on this as you are doing it and most likely more up on this but I think we need to establish if this file, once updated by us, remains as we left it. I.e. akin to package files installed in /etc that will not be overwritten by a package manager (normally) but will be written if the file don't exist. Such as @Hooverdan96 reported in #2032. This may help to clarify our choices.
Yes, unless docker arbitrarily decides that a missing config element (or repeat as we've just had) is a breaking change (given precedence was apparently impossible). If we edit what we find a fix is to copy over the backup created by the package (hopefully) and cycle our Rock-ons service. It will then edit what we need to an leave all other options as is, including custom ones edited by other programs / users. Sorry that's not all that clear but I think you are assuming this file will be updated by the package manger post our changes and I am assuming otherwise. So if we first clear that up then we know for sure and can take it form their. Plus the nuke and pave approach on config rather than edit has left us previously with really old config arrangements and an inline edit such as in 1. would not do that as at least each time we roll an iso we edit what we find. Let double check that files treatment over an upgrade post modification and take it from their. Personally I think it 'should' be left unaltered and the update should dump a copy of what it would have put their but not in play. This then means docker updates don't break existing config such as all other updates generally. However I also think that precedence should existing in options and warning not breakage should result if their are mulltiple competing options. This was not what we found so lets get more info on how this file is treated and take it form their. Hope thats of use and when we have more info we can take it from their. But again, your call and this will be in testing channel first (assuming we are still openSUSE only on this one). But nice to do right first time if we can. Plus we may loose either way :) |
Thanks a lot for your feedback, @phillxnet!
Very good idea, indeed... I'll try to figure that out by testing after a docker-ce update. The "nice" thing is that dockerd doesn't seem to care about the existence of the file as it's only there to optionally customize dockerd configuration. If no value for a given option is specified in daemon.json, the dockerd default will then apply.
Correct.
I should have mentioned that in my current PR preparation, I'm using a distinct json file for configuration and propose to write it in
It may also be that I failed to give you all the information, so we're not making our reasoning from the same resources. This is also why I agree with you on the fact that I need to figure out what a package update will or will not do to this default json file. It will for sure leave our edited json alone as it has no way to even be aware of its existence (or so I believe, at least). I personally highly doubt that this file will receive critical updates as it isn't its purpose (well... it shouldn't), Its purpose is only to customize and not define dockerd, As mentioned above, if a setting is not defined in this json file, the "hardcoded" dockerd default will apply.
Agreed... and that's actually pretty much what I coded so far: This way,
Yes, I am... in an effort to think of a worst-case scenario but that probably leads me to give too much importance to something that may very well never happen. Like you said, then, I'd better figure out exactly what a docker-ce update will do to this file (if any). Thanks again for spending some of your time on this, that is very helpful! |
I had some time to play around with docker update via zypper and observe the fate of In Leap 15.1, using
In 1 and 2, the changes were kept after the docker package install. In 3, however, I observed the same as @Hooverdan96 and It thus appears that the file won't be updated after package install no matter what we do to it, as long as we don't rename/remove it. |
[openSUSE] configure docker daemon using json configuration file. Fixes #2032 #2088 Source given daemon.json to create docker-daemon.json to facilitate Rockstor required configuration. Replaces, in openSUSE variants only, the prior docker Python wrapper which recently broke re systemd docker process management.
On openSUSE Leap15.1 (expected to be the case in Tumbleweed as well, but untested), while the Rock-on service turns ON without problem the first time, it errors out on subsequent starts (after being turned OFF, of course). The error is:
Full error
The
journalctl
log shows more details:Full error
As we can see in the error above, Docker fails to start because it is found to still be running. Indeed, we have:
After a little investigation, I believe the problem is linked to how systemd is stopping the docker service. Indeed, in the
docker.service
files we source directly from the package (since #2046), the docker service is meant to be stopped by terminating thedockerd
daemon process--which is thus the way it's meant to be stopped. This is illustrated in its service file by the following line:Full file
Using the standard docker "setup", this works well as the main process being killed is the
dockerd
daemon. Indeed:The main PID actually is the
dockerd
daemon, and because theKillMode
is set toprocess
in the service file, the docker daemon is actually killed when stopping the docker service.Using the Rockstor's customized docker service file, however, we see the difference:
As we can see above, the main PID refers here to our docker-wrapper and not directly the dockerd daemon. In this context, the
KillMode
set toprocess
understandably kills only thedocker-wrapper
and not the docker daemon.As a result, setting the
KillMode
tocontrol-group
(default in systemd) seems to resolve the problem. This was tested after starting the Rock-on service (thus re-setting thedocker.service
file to Rockstor's customized version), and then:We could thus change the KillMode setting in the service file while we're customizing it:
rockstor-core/src/rockstor/smart_manager/views/docker_service.py
Lines 85 to 101 in ec3903d
@phillxnet, what do you think of this approach? It seems it's a regression I introduced in #2046 and deeply apologize about it... I'm just surprised I didn't notice that when testing that PR and that it hasn't emerged since then...
The text was updated successfully, but these errors were encountered: