Skip to content
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

Restarting DBus before install certmonger #20

Closed
wants to merge 4 commits into from

Conversation

arxcruz
Copy link

@arxcruz arxcruz commented May 28, 2018

If DBus is updated before run this module and the machine not get
restarted, the certmonger fails (among other services that relies
on dbus service).

If DBus is updated before run this module and the machine not get
restarted, the certmonger fails (among other services that relies
on dbus service).
@JAORMX
Copy link
Collaborator

JAORMX commented May 28, 2018

@locknut @earsdown could you take a look?

@locknut
Copy link

locknut commented May 29, 2018

Not sure if managing the dbus service is within the scope of this module. Even if we do this, I think we should have a $manage_dbus_service parameter at a minimum?

@locknut
Copy link

locknut commented May 29, 2018

Also, if this problem appears only when upgrading from dbus 7.4 to 7.5, then is it really necessary for this module to handle that specific condition?

Adding an option to restart dbus service
@arxcruz
Copy link
Author

arxcruz commented May 29, 2018

Added manage_dbus_service option

Copy link
Collaborator

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, and it's reasonably turned off by default. Which we can enable for such cases as that update.

service { 'dbus':
ensure => running,
enable => true,
restart => '',

This comment was marked as resolved.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be good to properly define the dbus service - for example, there's this module available on the puppet forge, and the service definition:
https://github.com/bodgit/puppet-dbus/blob/master/manifests/service.pp

service { 'dbus':
ensure => running,
enable => true,
restart => '',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really usefuly to set this, according to the doc: "Specify a restart command manually. If left unspecified, the service will be stopped and then started"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What options to we have for this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, you can actually define the proper restart command if you want. But such a service, there's no use doing so I guess.

@JAORMX
Copy link
Collaborator

JAORMX commented Jun 1, 2018

I think the commit is ready, @locknut what do you think?

@JAORMX
Copy link
Collaborator

JAORMX commented Jun 5, 2018

@locknut @earsdown could you take a look at this if you have some time?

@locknut
Copy link

locknut commented Jun 7, 2018

Sorry it's taken a few days to get to this. Had a look. Don't think this PR can be accepted (yet?) for a number of reasons, and maybe we've misunderstood, so help us understand if we've got this wrong.

  1. The issue only presents itself during the process of upgrading from dbus 7.4 to 7.5? Or does this issue occur when trying to install certmonger on a system running dbus >=7.5? Does it occur at runtime (e.g. during system boot)? Can someone link me to the actual bug?
  2. Will this commit actually result in a restart of the dbus service after installing certmonger? Shouldn't the dbus service subscribe to the certmonger package? Or should the dbus service subscribe to the certmonger service? Why is the dbus service notifying the certmonger service? Why isn't the proper solution a fix to either certmonger's or dbus's systemd unit file?
  3. Isn't it better to just document dbus's (broken?) behaviour and let users implement their workarounds at the profile level or rely on a separate dbus module?

Sorry about bringing up all of these issues. We haven't experienced the bug (yet?) so we're relying on you guys on this PR more than usual.

@JAORMX
Copy link
Collaborator

JAORMX commented Nov 22, 2018

Seems we won't need this anymore.

@JAORMX JAORMX closed this Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants