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

ipa-replica-install: properly use the file store #2332

Closed
wants to merge 2 commits into from

Conversation

flo-renaud
Copy link
Contributor

ipa-replica-install: properly use the file store

In ipa-replica-install, many components use their own instance of the FileStore to backup configuration files to the pre-install state. This causes issues when the calls are mixed, like for instance:
ds.do_task1_that_backups_file (using ds.filestore)
http.do_task2_that_backups_file (using http.filestore)
ds.do_task3_that_backups_file (using ds.filestore)

because the list of files managed by ds.filestore does not include the files managed by http.filestore, and the 3rd call would remove any file added on 2nd call.

The symptom of this bug is that ipa-replica-install does not save /etc/httpd/conf.d/ssl.conf and subsequent uninstallation does not restore the file, leading to a line referring to ipa-rewrite.conf
that prevents httpd startup.

The installer should consistently use the same filestore.

Fixes https://pagure.io/freeipa/issue/7684

Test: scenario replica install/uninstall should restore ssl.conf

Test that the scenario ipa-replica-install/ uninstall correctly restores the file /etc/httpd/conf.d/ssl.conf

Related to https://pagure.io/freeipa/issue/7684

@flo-renaud flo-renaud added prioritized Pull Request has higher priority for PR-CI needs review Pull Request is waiting for a review ipa-4-7 ipa-4-6 Mark for backport to ipa 4.6 labels Sep 5, 2018
@rcritten
Copy link
Contributor

rcritten commented Sep 5, 2018

We ran into something similar in the past. It was fixed in 082c55f

Is that the same issue?

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

The patch doesn't add fstore argument to bindinstance.BindInstance(api=remote_api).

In ipa-replica-install, many components use their own instance
of the FileStore to backup configuration files to the pre-install
state. This causes issues when the calls are mixed, like for
instance:
ds.do_task1_that_backups_file (using ds.filestore)
http.do_task2_that_backups_file (using http.filestore)
ds.do_task3_that_backups_file (using ds.filestore)

because the list of files managed by ds.filestore does not include
the files managed by http.filestore, and the 3rd call would remove
any file added on 2nd call.

The symptom of this bug is that ipa-replica-install does not save
/etc/httpd/conf.d/ssl.conf and subsequent uninstallation does not
restore the file, leading to a line referring to ipa-rewrite.conf
that prevents httpd startup.

The installer should consistently use the same filestore.

Fixes https://pagure.io/freeipa/issue/7684
Test that the scenario ipa-replica-install/ uninstall correctly
restores the file /etc/httpd/conf.d/ssl.conf

Related to https://pagure.io/freeipa/issue/7684
@flo-renaud
Copy link
Contributor Author

flo-renaud commented Sep 6, 2018

@rcritten

We ran into something similar in the past. It was fixed in 082c55f

Is that the same issue?

082c55f was about the class StateFile (usually linked to sysrestore.state which contains sections and key=value), but this one is related to the class FileStore (sysrestore.index which contains a list of backup files with their permissions uid and gid).
Do you think I should rather follow the same strategy for the fix, i.e. reload the list of files at the beginning of every public method?

@tiran
thanks for the review, I added the fstore to the bindinstance call.

@tiran tiran added ack Pull Request approved, can be merged and removed needs review Pull Request is waiting for a review labels Sep 6, 2018
@flo-renaud flo-renaud added the re-run Trigger a new run of PR-CI label Sep 6, 2018
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Sep 6, 2018
@tiran tiran added the pushed Pull Request has already been pushed label Sep 6, 2018
@tiran
Copy link
Member

tiran commented Sep 6, 2018

master:

  • 6ad11d8 ipa-replica-install: properly use the file store
  • b2ce20c Test: scenario replica install/uninstall should restore ssl.conf

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 ipa-4-6 Mark for backport to ipa 4.6 prioritized Pull Request has higher priority for PR-CI pushed Pull Request has already been pushed
Projects
None yet
4 participants