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

restore: restart/reload gssproxy after restore #748

Closed
wants to merge 1 commit into from

Conversation

pvoborni
Copy link
Member

@pvoborni pvoborni commented Apr 28, 2017

So that gssproxy picks up new configuration and therefore related
usages like authentication of CLI against server works

https://pagure.io/freeipa/issue/6902

So that gssproxy picks up new configuration and therefore related
usages like authentication of CLI against server works

https://pagure.io/freeipa/issue/6902
@pvoborni
Copy link
Member Author

Obsoletes PR #738

def restart(self, instance_name="", capture_output=True, wait=True):
ipautil.run([paths.SYSTEMCTL, "restart",
self.service_instance(instance_name)],
def _restart_base(self, instance_name, operation, capture_output=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd either set operation="restart" as default or rename the method to something more descriptive.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's better to pass explicit operation to the function to prevent surprise behavior. The public functions are then more readable.

Feel free to suggest better name for the base function.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was meant as a base class for: "reload", "restart", "try-restart", "reload-or-restart" and "try-reload-or-restart". The name could be reload_restart_base, but given that it is private/protect function then it can be renamed at any time in the future without any trouble when new methods are introduced as needed..

Copy link
Contributor

Choose a reason for hiding this comment

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

The method as is could be used for any systemctl command and I think the name should reflect that.

How about _systemctl_exec or _systemctl_command?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, since the method also waits for open ports, the name shouldn't suggest it just executes a systemctl command.

def restart(self, instance_name="", capture_output=True, wait=True):
ipautil.run([paths.SYSTEMCTL, "restart",
self.service_instance(instance_name)],
def _restart_base(self, instance_name, operation, capture_output=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, since the method also waits for open ports, the name shouldn't suggest it just executes a systemctl command.

@tkrizek tkrizek self-assigned this Apr 28, 2017
@tkrizek
Copy link
Contributor

tkrizek commented Apr 28, 2017

How is this patch going to work for Debian? Shouldn't we also implement reload_or_restart for DebianSysvService?

@pvoborni
Copy link
Member Author

Should work:

def debian_service_class_factory(name, api=None):
    if name == 'dirsrv':
        return redhat_services.RedHatDirectoryService(name, api)
    if name == 'domainname':
        return DebianNoService(name, api)
    if name == 'ipa':
        return redhat_services.RedHatIPAService(name, api)
    if name == 'messagebus':
        return DebianNoService(name, api)
    if name == 'ntpd':
        return DebianSysvService("ntp", api)
    return DebianService(name, api)

so it's DebianService

class DebianService(redhat_services.RedHatService):
    system_units = debian_system_units

then

class RedHatService(base_services.SystemdService):

I.e. it is not DebianSysvService

@tkrizek
Copy link
Contributor

tkrizek commented Apr 28, 2017

Ok, everything looks good then.

@tkrizek tkrizek added the ack Pull Request approved, can be merged label Apr 28, 2017
@tkrizek
Copy link
Contributor

tkrizek commented Apr 28, 2017

master:

  • 3a4c8e3 restore: restart/reload gssproxy after restore

ipa-4-5:

  • 04ed1fa restore: restart/reload gssproxy after restore

@tkrizek tkrizek added the pushed Pull Request has already been pushed label Apr 28, 2017
@tkrizek tkrizek closed this Apr 28, 2017
Copy link
Contributor

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

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

Late to the party, but I'm much happier with this approach, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
3 participants