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

Remove the renewal lock file upon uninstall #229

Closed
wants to merge 1 commit into from

Conversation

flo-renaud
Copy link
Contributor

@flo-renaud flo-renaud commented Nov 10, 2016

Make sure that the file /var/run/ipa/renewal.lock is deleted upon
uninstallation, in order to avoid subsequent installation issues.

Part of the refactoring effort, certificates sub-effort.

https://fedorahosted.org/freeipa/ticket/6433

@HonzaCholasta
Copy link
Contributor

The file is owned by the server, not the client, so it should be deleted in ipa-server-install --uninstall, not in ipa-client-install --uninstall.

@flo-renaud
Copy link
Contributor Author

You are right, I updated the PR to put the code at the end of server uninstallation.

@frasertweedale
Copy link
Contributor

Works as expected.

pre_command, post_command)
state = wait_for_request(reqId, timeout=60)
passwd_fname, dns, ca, profile)
state = wait_for_request(reqId, timeout=180)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please put the timeout increase into a separate commit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it might not even be necessary with the other fixes?

try:
os.remove(paths.IPA_RENEWAL_LOCK)
except Exception:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be better (off the top of my head):

    try:
        os.remove(paths.IPA_RENEWAL_LOCK)
    except OSError as e:
        if e.errno != errno.ENOENT:
            root_logger.warning("Failed to remove %s: %s", paths.IPA_RENEWAL_LOCK, e)


# Add presave and postsave commands
# This is not done earlier to avoid running the cmds
# during the initial request
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this, I would rather not restart the services in renew_ra_cert, restart_httpd and restart_dirsrv if they were not previously running.

Copy link
Contributor

Choose a reason for hiding this comment

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

Speaking of which, I don't think the restart in renew_ra_cert is necessary at all - httpd itself does not use the RA cert in any way, it is used only in the framework.

- Make sure that the file /var/run/ipa/renewal.lock is deleted upon
uninstallation, in order to avoid subsequent installation issues.

- Modify certmonger renewal script: restart the http/dirsrv services
only if they were already running

- Cleanup certmonger ra renewal script: no need to restart httpd

- Reorder during http install: request the SSL cert before adding
ipa-service-guard
Rationale: when a CA helper is modified, certmonger launches the helper
with various operations (FETCH_ROOTS, ...) If the CA helper is once again
modified, the on-going helper is killed. This can lead to
ipa-service-guard being killed and not releasing the renew lock.

If the SSL cert is requested with IPA helper before ipa-service-guard is added,
we avoid this locking issue.

Part of the refactoring effort, certificates sub-effort.

https://fedorahosted.org/freeipa/ticket/6433
@flo-renaud
Copy link
Contributor Author

Hi,
I implemented @jcholast suggestions and finally found the origin of the lock.

@MartinBasti
Copy link
Contributor

Works for me on both domain levels, I'd ACK this if nobody is against

@MartinBasti MartinBasti added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Nov 15, 2016
@MartinBasti
Copy link
Contributor

@flo-renaud flo-renaud deleted the renewallock branch November 16, 2016 10:07
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
4 participants