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

Replace DS replication plugin name #5739

Closed
wants to merge 2 commits into from

Conversation

mreynolds389
Copy link
Contributor

In efforts to remove problematic language, like master and slave,
DS is replacing "master" with "supplier" in the replication plugin
entry under cn=config.

DS should be able handle upgrades correctly as well.

relates: https://pagure.io/freeipa/issue/8799

In efforts to remove problematic language, like master and slave,
DS is replacing "master" with "supplier" in the replication plugin
entry under cn=config.

One problem here is that DS will not be backwards compatible since
we are not supposed to have the word "master" in our source code.
So upgrades would not work at this time.

relates: https://pagure.io/freeipa/issue/8799
@flo-renaud
Copy link
Contributor

Adding re-run to trigger PRCI tests (@mreynolds389 is not a member of the allowed list)

@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Apr 26, 2021
@rcritten
Copy link
Contributor

389-ds isn't starting after restart. The journal had this:

Apr 26 21:30:48 ipa.example.test ns-slapd[447343]: [26/Apr/2021:21:30:48.118714150 +0000] - ERR - plugin_dependency_startall - Failed to resolve plugin dependencies
Apr 26 21:30:48 ipa.example.test ns-slapd[447343]: [26/Apr/2021:21:30:48.187004606 +0000] - ERR - plugin_dependency_startall - object plugin IPA Topology Configuration is not started

@mreynolds389
Copy link
Contributor Author

389-ds isn't starting after restart. The journal had this:

Apr 26 21:30:48 ipa.example.test ns-slapd[447343]: [26/Apr/2021:21:30:48.118714150 +0000] - ERR - plugin_dependency_startall - Failed to resolve plugin dependencies
Apr 26 21:30:48 ipa.example.test ns-slapd[447343]: [26/Apr/2021:21:30:48.187004606 +0000] - ERR - plugin_dependency_startall - object plugin IPA Topology Configuration is not started

Anything else in the errors log? And how did it fail? Just during an install?

@rcritten
Copy link
Contributor

I didn't notice anything else out of the ordinary. You can see a sample full log at http://freeipa-org-pr-ci.s3-website.eu-central-1.amazonaws.com/jobs/df8fb0ce-a696-11eb-94e5-5254008d364b/test_integration-test_commands.py-TestIPACommand-install/master.ipa.test/var/log/dirsrv/slapd-IPA-TEST/errors.gz

It reproducibly fails during installation. It failed for me locally for me as well.

@mreynolds389
Copy link
Contributor Author

I didn't notice anything else out of the ordinary. You can see a sample full log at http://freeipa-org-pr-ci.s3-website.eu-central-1.amazonaws.com/jobs/df8fb0ce-a696-11eb-94e5-5254008d364b/test_integration-test_commands.py-TestIPACommand-install/master.ipa.test/var/log/dirsrv/slapd-IPA-TEST/errors.gz

It reproducibly fails during installation. It failed for me locally for me as well.

I don't see /etc/dirsrv/ from that test run. Any chance the dse.ldif is still available? No biggie if it's not...

@mreynolds389
Copy link
Contributor Author

How can I test IPA with my proposed PR? Can a COPR build be made for me?

Also, just to confirm, was this PR tested against the nightly COPR build of DS (not the RPM that ships on the OS)?
Thanks,
Mark

@rcritten
Copy link
Contributor

You can get a nightly build of freeipa at https://copr.fedorainfracloud.org/coprs/g/freeipa/freeipa-master-nightly/builds/

This PR was tested against the released version of DS.

@mreynolds389
Copy link
Contributor Author

You can get a nightly build of freeipa at https://copr.fedorainfracloud.org/coprs/g/freeipa/freeipa-master-nightly/builds/

This PR was tested against the released version of DS.

Ok, can we get this PR tested against a DS nightly COPR build (https://copr.fedorainfracloud.org/coprs/g/389ds/389-ds-base-nightly/)? This PR should succeed with our latest DS build. Once verified we can figure out how to sync up the builds so no one breaks once this is officially built upstream.

@flo-renaud
Copy link
Contributor

Hi @mreynolds389
if you want to run this PR against the nightly 389ds build, you need to do the following on top of this PR:

ln -sf ipatests/prci_definitions/nightly_latest_389ds.yaml .freeipa-pr-ci.yaml
git add .freeipa-pr-ci.yaml
git commit

and push the additional commit to your branch mreynolds389:issue8799
This will use the test definition specified in nightly_latest_389ds.yaml, which runs on images with the @389ds/389-ds-base-nightly copr enabled.

@flo-renaud flo-renaud added the re-run Trigger a new run of PR-CI label Apr 27, 2021
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Apr 27, 2021
@flo-renaud
Copy link
Contributor

The azure failures are expected since azure is not using @389ds nightly copr repo.

@rcritten
Copy link
Contributor

In general this looks ok for new installations. Are you proposing to change the values in existing installs as well? I don't believe that will be necessary given these are all in cn=config, just checking on your plans.

@mreynolds389
Copy link
Contributor Author

In general this looks ok for new installations. Are you proposing to change the values in existing installs as well?

In DS we do have an upgrade mechanism in place that occurs at server startup. So DS should handle upgrades seamlessly.

Yeah so I wanted to confirm, the tests passed when using the DS COPR build?

@rcritten
Copy link
Contributor

Yeah so I wanted to confirm, the tests passed when using the DS COPR build?

That is correct.

@flo-renaud
Copy link
Contributor

Hi @mreynolds389
There is one issue with the new name in 389ds, though. Let's imagine the following situation:

  • ipa package on the machine is the current version (4.9.3), and contains code using "Multimaster Replication Plugin"
  • 389-ds on the machine is the version with the patch, i.e. using "Multisupplier Replication Plugin"
    (We can force new ipa version to depend on new 389ds version but the reverse is not true and an admin could possibly update 389ds but keep an old IPA).
    A new installation is likely to fail when ipa-server-install tries to configure anything referring to "Multimaster Replication Plugin", unless 389ds has special code to handle this case.

@tbordaz
Copy link
Contributor

tbordaz commented May 18, 2021

Hi @mreynolds389
There is one issue with the new name in 389ds, though. Let's imagine the following situation:

* ipa package on the machine is the current version (4.9.3), and contains code using "Multimaster Replication Plugin"

* 389-ds on the machine is the version with the patch, i.e. using "Multisupplier Replication Plugin"
  (We can force new ipa version to depend on new 389ds version but the reverse is not true and an admin could possibly update 389ds but keep an old IPA).
  A new installation is likely to fail when ipa-server-install tries to configure anything referring to "Multimaster Replication Plugin", unless 389ds has special code to handle this case.

@flo-renaud, how IPA is dealing with 'cn=Multimaster Replication Plugin,cn=plugins,cn=config' entry ? Does it update dse.ldif or do only ldap operations ? If it is ldap operation we may register callbacks to catch the search/read and redirect it to the right entry.

@mreynolds389
Copy link
Contributor Author

Hi @mreynolds389
There is one issue with the new name in 389ds, though. Let's imagine the following situation:

* ipa package on the machine is the current version (4.9.3), and contains code using "Multimaster Replication Plugin"

* 389-ds on the machine is the version with the patch, i.e. using "Multisupplier Replication Plugin"
  (We can force new ipa version to depend on new 389ds version but the reverse is not true and an admin could possibly update 389ds but keep an old IPA).
  A new installation is likely to fail when ipa-server-install tries to configure anything referring to "Multimaster Replication Plugin", unless 389ds has special code to handle this case.

This is a problem that can not be solved. 389 and IPA must be dependent on each other. But in RHEL I didn't think you could just update 389 and not IPA. Is it really possible? If it is possible, I don't know how to fix it and still be compliant with the "Problematic language" requirement.

@abbra
Copy link
Contributor

abbra commented May 18, 2021

I think in this specific case 389-ds has to keep recognition of the both names. I think it is OK for compatibility reasons.

@mreynolds389
Copy link
Contributor Author

I think in this specific case 389-ds has to keep recognition of the both names. I think it is OK for compatibility reasons.

My understanding is that it is not okay to keep "master" text in the source code (for any reason). I hope I'm wrong, but I was under the impression this was pretty strict...

@abbra
Copy link
Contributor

abbra commented May 19, 2021

I have reviewed existing materials, externally and internally, and while there is a strong push, there is no definitive answer for compatibility/legacy support. Most of the intended changes concern user visible content or content within the code that does not affect communication with other tools. There is simply no answer for our case of a legacy configuration dependency.

I would strongly suggest to keep 389-ds support for recognizing previous plugin name and aliasing it into the new one. Without it 389-ds cannot be used for upgrades. We cannot guarantee that every RHEL user, for example, would not do individual package upgrade. In fact, we do see this quite often, according to our support organization.

@abbra
Copy link
Contributor

abbra commented May 19, 2021

Another observation is that the suggestion I see is to avoid pairing 'master' and 'slave' in IT context. In this particular case there is no such pairing as we are dealing with the multimaster plugin name. Combined with the previous argumentation, this is definitely not the case where a change is strictly required. If anything, keeping the old 'multimaster' name is definitely a practical solution.

@tbordaz
Copy link
Contributor

tbordaz commented May 19, 2021

I have reviewed existing materials, externally and internally, and while there is a strong push, there is no definitive answer for compatibility/legacy support. Most of the intended changes concern user visible content or content within the code that does not affect communication with other tools. There is simply no answer for our case of a legacy configuration dependency.

I would strongly suggest to keep 389-ds support for recognizing previous plugin name and aliasing it into the new one. Without it 389-ds cannot be used for upgrades. We cannot guarantee that every RHEL user, for example, would not do individual package upgrade. In fact, we do see this quite often, according to our support organization.

389DS server can be compatible with the two naming. For example, we are aliasing the RUV read with the two entries 'nsuniqueid=ffffff-ffff...,' and 'cn=replica,cn=,cn=mapping tree,cn=config'. Here we should support read/write but I think it is doable. That means to extend the original 'problematic language' patch that @mreynolds389 did or to prepare a new patch. This is not an immediate fix. It will remain some changes to do on client application (like freeipa), if some script are directly digging in dse.ldif.

@abbra
Copy link
Contributor

abbra commented May 19, 2021

FreeIPA does have one specific update of dse.ldif that is done directly: when installing a replica, ipa-replica-install accepts an LDIF file to add to dse.ldif:

    --dirsrv-config-file=FILE
                        The path to LDIF file that will be used to modify
                        configuration of dse.ldif during installation of the
                        directory server instance

The same can be done in ipa-server-install. This is not used in a default installation but is employed in IPA tests where we add replication debugging:

        # replication debugging
        dn: cn=config
        changetype: modify
        replace: nsslapd-errorlog-level
        nsslapd-errorlog-level: {log_level}

        # server writes all access log entries directly to disk
        dn: cn=config
        changetype: modify
        replace: nsslapd-accesslog-logbuffering
        nsslapd-accesslog-logbuffering: off

Basically, none of the changes we do touch the code that would be affected by the problematic language patches.

@mreynolds389
Copy link
Contributor Author

mreynolds389 commented May 19, 2021

Another observation is that the suggestion I see is to avoid pairing 'master' and 'slave' in IT context. In this particular case there is no such pairing as we are dealing with the multimaster plugin name.

I would respectfully disagree. In "replication" the term Master also implies Slave: master and slaves replicas. That pairing has been used for a very long time in the LDAP community. Just because we don't use the term Slave in our config doesn't mean that Master is off the hook.

Your suggestion about mapping multimaster to multisuppler internally is not going to be a trivial change. In fact it makes me very nervous creating some "ghost entry" just for this mapping, or trying to catch an error 32 and then use some mechanism to process it after the fact is also overkill and adds risk. I would prefer to have a specfile requirement forcing a minimum IPA version if this must be enforced.

But since this is all supposed to land at the same time in the next major version of RHEL is there really a risk of an IPA version that does not support this? I don't see how unless you refuse to merge this PR.

@abbra
Copy link
Contributor

abbra commented May 19, 2021

@mreynolds389 I don't see how that combined update would solve the problem @flo-renaud pointed out, namely, when a user updates 389-ds-base but not IPA. This is really what we see happening all the time with RHEL support where customers choose to run yum install/upgrade <package> rather than yum upgrade to allow all packages to upgrade. If they'd choose to yum install 389-ds-base instead of allowing to upgrade IPA as well, they'll end up with non-working deployment.

Perhaps, you would need to add a bit dangerous 'Conflicts: ipa-server < 4.9.4' or similar statement in 389-ds-base spec file to prevent it installing on the systems with older FreeIPA. I am not sure how this would work upgrade-wise, this needs to be tested a lot.

Note also that FreeIPA is not going to get rid of 'master' word for quite some time. We have cn=masters,cn=ipa,cn=etc,$SUFFIX group, for example, that cannot be easily renamed in a multi-supplier replicated environment. We have MIT Kerberos ABI symbols that include master in the name and are part of KDC database layer definitions that is not going to be modified. We also have certain technologies where a word 'master' is codified like /etc/auto.master in autofs. There are other elements that cannot be easily changed, so it will take a lot of time before we'd see a light without 'master'.

A separate question I have is whether upgrade code 389-ds adds for this case handles backup/restore of an instance data?

@mreynolds389
Copy link
Contributor Author

@mreynolds389 I don't see how that combined update would solve the problem @flo-renaud pointed out, namely, when a user updates 389-ds-base but not IPA. This is really what we see happening all the time with RHEL support where customers choose to run yum install/upgrade <package> rather than yum upgrade to allow all packages to upgrade. If they'd choose to yum install 389-ds-base instead of allowing to upgrade IPA as well, they'll end up with non-working deployment.

Perhaps, you would need to add a bit dangerous 'Conflicts: ipa-server < 4.9.4' or similar statement in 389-ds-base spec file to prevent it installing on the systems with older FreeIPA. I am not sure how this would work upgrade-wise, this needs to be tested a lot.

Note also that FreeIPA is not going to get rid of 'master' word for quite some time. We have cn=masters,cn=ipa,cn=etc,$SUFFIX group, for example, that cannot be easily renamed in a multi-supplier replicated environment. We have MIT Kerberos ABI symbols that include master in the name and are part of KDC database layer definitions that is not going to be modified. We also have certain technologies where a word 'master' is codified like /etc/auto.master in autofs. There are other elements that cannot be easily changed, so it will take a lot of time before we'd see a light without 'master'.

A separate question I have is whether upgrade code 389-ds adds for this case handles backup/restore of an instance data?

DS, at every server start up, checks if multiSupplier is being used, if its not then it renames it. This is only an issue with dse.ldif, nothing in the database uses the config names. So DS can handle restores of dse.ldif.

Regarding your other comments I will take that offline since I don't know if we can talk about it publicly...

@abbra abbra added the ipa-4-9 Mark for backport to ipa 4.9 label May 24, 2021
@freeipa-pr-ci freeipa-pr-ci added the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Jun 1, 2021
@flo-renaud
Copy link
Contributor

@mreynolds389
Since @abbra committed https://pagure.io/freeipa/c/b4b2c10e234315042b54a4556bf264d55ecd07c9 we can close this PR, right?

@mreynolds389
Copy link
Contributor Author

@mreynolds389
Since @abbra committed https://pagure.io/freeipa/c/b4b2c10e234315042b54a4556bf264d55ecd07c9 we can close this PR, right?

It would appear so. Closing PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ipa-4-9 Mark for backport to ipa 4.9 needs rebase Pull Request cannot be automatically merged - needs to be rebased
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants