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

Handle NTP configuration in a replica server installation #2451

Closed
wants to merge 1 commit into from

Conversation

rcritten
Copy link
Contributor

There were two separate issues:

  1. If not enrolling on a pre-configured client then the ntp-server and
    ntp-pool options are not being passed down to the client installer
    invocation.
  2. If the client is already enrolled then the ntp options are ignored
    altogether.

So basically reverse those. Detect the ntp options and add them to
the ipa-client-install invocation if the client is not pre-enrolled.

If it is pre-enrolled then call out to the time configuration
methods to setup the servers or pool. The changes will be recorded
in the client sysrestore.

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

Signed-off-by: Rob Crittenden rcritten@redhat.com

@flo-renaud flo-renaud self-assigned this Oct 16, 2018
Copy link
Member

@Tiboris Tiboris left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. Just had a quick look at it :)
LGTM but not tested i will leave it to assignee

Copy link
Contributor

@flo-renaud flo-renaud left a comment

Choose a reason for hiding this comment

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

Hi @rcritten
thanks for the PR. Please find inline comments.

options.ntp_pool,
client_fstore,
client_statestore)

Copy link
Contributor

Choose a reason for hiding this comment

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

At first I was not comfortable with configuring chrony in promote_check, but since this method is doing more than checks (for instance if client is not already installed, it calls ensure_enrolled that calls ipa-client-install), it's OK to do this part here. Moreover both branches of if not client_fstore.has_files(): end with the client installed and NTP configured.

Copy link
Contributor

Choose a reason for hiding this comment

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

If client was installed with -N and replica without -N, chronyd is not configured on the replica. I am not sure what would be expected in this case (would you expect the admin to explicitely provide -N to both client and replica-install commands if he doesn't want chrony?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So for the promote_check that's ok then? I didn't quite follow the branches, is that a problem?

Nice catch on -N, I hadn't thought of that! And there are some more edge cases there too.

client with -N and replica with options, client with options and replica with -N... I can only hope to not make a mess of this.

How about this. If the client is pre-installed then we have to honor whatever NTP configuration is already there and ipa-replica-install errors out if NTP options are passed in.

For a pure install from ipa-replica-install the NTP options are honored. I update the man page to reflect this behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

So for the promote_check that's ok then?

Yes.

How about this. If the client is pre-installed then we have to honor whatever NTP configuration is already there and ipa-replica-install errors out if NTP options are passed in.

OK, seems a good proposal.

For a pure install from ipa-replica-install the NTP options are honored. I update the man page to reflect this behavior.

OK, thanks.

There were two separate issues:

1. If not enrolling on a pre-configured client then the ntp-server and
   ntp-pool options are not being passed down to the client installer
   invocation.
2. If the client is already enrolled then the ntp options are ignored
   altogether.

In the first case simply pass down the options to the client
installer invocation.

If the client is pre-enrolled and NTP options are provided then
raise an exception.

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

Signed-off-by: Rob Crittenden <rcritten@redhat.com>
@flo-renaud
Copy link
Contributor

Hi @rcritten,
thanks for the updated patch. The new behavior is working as expected, I tested:

  • 2-step replica install, ipa-replica-install using ntp options: refused as expected
  • 2-step replica install, ipa-replica-install without ntp options: working, chrony config as specified in client-install. Uninstall reverts to the pre-client-install state.
  • 1-step replica install with ntp options: working, chrony config as specified in replica-install options. Un-install reverts to the pre-replica-install state.

@flo-renaud flo-renaud added the ack Pull Request approved, can be merged label Oct 19, 2018
@rcritten rcritten added the pushed Pull Request has already been pushed label Oct 19, 2018
@rcritten
Copy link
Contributor Author

master:

  • 2fba5ac Handle NTP configuration in a replica server installation

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