With FreeIPA in a container (https://github.com/adelton/docker-freeipa), upgrades are done by merely starting new container from new image and give it the volume with configuration and data. The whole /etc/dirsrv lives in volume because it container instance-specific setup but that means that /etc/dirsrv/schema lives in the container as well.
However, when running upgrade for dirsrv, it needs /etc/dirsrv/schema matching the version of 389 server.
In the container we have workarounded in
https://github.com/adelton/docker-freeipa/blob/master/Dockerfile
by moving it to /usr/share to live in image and not in volume and symlinking it from /etc/dirsrv but if this content has to match the version of dirsrv installed, it really should not be in /etc in the first place.
We have discussed this, and decided to expand on the work in this ticket.
First, schema will be moved from /etc/dirsrv/schema to /usr/lib64/dirsrv/schema. There will be no symlink from /etc/dirsrv/schema created, it will remain as a directory.
Files that are unmodified and belong to ds schema will be removed from /etc/dirsrv/schema. Files that have been modified will remain (requires a posttrans hack)
Setup-ds.pl will no longer copy schema to /etc/dirsrv/slapd-inst/schema. -u will not either.
ds will be modified to read it's schema in, and overlay them in the following order:
/usr/lib64/dirsrv/schema /etc/dirsrv/schema /etc/dirsrv/slapd-inst/schema
This allows:
Replying to [comment:3 firstyear]:
Can you please elaborate on the posttrans hack? Checking rpm -q --scripts 389-ds-base-1.3.4.4-1.fc23.1.x86_64 does not show anything posttrans-ish.
Please note that any scriptlet modifications to the instance-specific data (like /etc/dirsrv should be) complicate things for containerized deployments because with containers, there is no rpm upgrade -- it's always fresh installation of the packages when there is no data available.
Replying to [comment:4 adelton]:
Replying to [comment:3 firstyear]: Files that are unmodified and belong to ds schema will be removed from /etc/dirsrv/schema. Files that have been modified will remain (requires a posttrans hack) Can you please elaborate on the posttrans hack? Checking rpm -q --scripts 389-ds-base-1.3.4.4-1.fc23.1.x86_64 does not show anything posttrans-ish.
If the user has edited files in /etc/dirsrv/schema, rpm upgrade will automatically rename them with a .rpmsave extension. What we want to do is to rename them back without the .rpmsave extension so that they will continue to work. We have to do that after the rpm rename has happened, which I believe we can do during the posttrans phase.
Right, but I want to make sure that whatever we do works both for "plain" installation/upgrade as well as container installation.
Replying to [comment:5 rmeggins]:
Can't we use noreplace?
Great.
Replying to [comment:6 adelton]:
Replying to [comment:5 rmeggins]: If the user has edited files in /etc/dirsrv/schema, rpm upgrade will automatically rename them with a .rpmsave extension. What we want to do is to rename them back without the .rpmsave extension so that they will continue to work. We have to do that after the rpm rename has happened, which I believe we can do during the posttrans phase. Can't we use noreplace?
Maybe.
Right, but I want to make sure that whatever we do works both for "plain" installation/upgrade as well as container installation. Great.
config(noreplace) is what causes them to become .rpmsave when we remove them.
attachment 0001-Ticket-48163-Read-schema-from-multiple-locations.patch
This doesn't try to solve any of our schema replication issues. In fact, by moving the system schema to /usr, we actually just retain the current situation with schema repl, where all changes land in 99user.ldif.
Where this aids us is:
Looks good, a few minor issues:
{{{ path_wrapper_free(struct path_wrapper p) { 58 if (p != NULL) { 59 if (p->path) { 60 slapi_ch_free((void *)&(p->path)); 61 } 62 if (p->filename) { }}}
No need to check "path" and "filename" for NULL, this is already done in slapi_ch_free()
add_file_to_list()
--> indentation issues --> Comment about strdup being taken care of my "free_string", but free_string() is removed and replaced by your free wrapper function. The comment should reflect this.
ldap/servers/slapd/schema.c
--> several indentation issues
I'm wondering if there is no extra work needed for the upgrade?
In case the new version of schema has some updates, if you run setup-ds.pl -u", does it apply the fix to each instance properly? Probably, you could double-check this script -- 60upgradeschemafiles.pl? Thanks.
Fix marks comments 0001-Ticket-48163-Read-schema-from-multiple-locations.2.patch
Fix marks whitespace comment, by re-tabbing the file. 0002-Ticket-48163-Re-space-schema.c.patch
Replying to [comment:11 nhosoi]:
I'm wondering if there is no extra work needed for the upgrade? In case the new version of schema has some updates, if you run setup-ds.pl -u", does it apply the fix to each instance properly? Probably, you could double-check this script -- 60upgradeschemafiles.pl? Thanks.
Ohhh, there will definetly be extra work needed for the upgrade.
In NEW installs, it makes the upgrade easier: We don't need to bother touching the schema files, as they update from the rpm. Done. Magic!
However, existing installs, we need to determine that they do have the existing schema in /etc/dirsrv/slapd-inst/schema, and either:
Replying to [comment:12 firstyear]:
Replying to [comment:11 nhosoi]: I'm wondering if there is no extra work needed for the upgrade? In case the new version of schema has some updates, if you run setup-ds.pl -u", does it apply the fix to each instance properly? Probably, you could double-check this script -- 60upgradeschemafiles.pl? Thanks. Ohhh, there will definetly be extra work needed for the upgrade. In NEW installs, it makes the upgrade easier: We don't need to bother touching the schema files, as they update from the rpm. Done. Magic! However, existing installs, we need to determine that they do have the existing schema in /etc/dirsrv/slapd-inst/schema, and either: remove it, and let the new process work Do the update procedure as normal.
Another question... Do we need to some changes to the schema reload task for this change? It mainly uses the schema code, so the fix may be automatically applied to it, but we need to test it...
Replying to [comment:13 nhosoi]:
Ohhh probably. I think testing it would be the first step. Provided that schema reload just calls init_schema_dse_ext() at some point it should "just work".
Okay, with regards to the schema task. This will "just work".
The schema task calls slapi_reload_schema_files(td->schemadir); This schemadir is set to the instance schema dir, or "default".
Then slapi_reload_schema_files(char path) calls init_schema_dse_ext(). Because of the way the change to init_schema_dse_ext(char path) was made, it reads from {SYSTEMSCHEMADIR, path}.
In other words, it will work exactly as expected!
Good to hear that. Thanks for trying it.
With ldap/admin/src/scripts/60upgradeschemafiles.pl we have a choice.
Moving between 1.3.4 -> 1.3.5 obviously we will gain schema in /usr.
So we need to choose how we handle this upgrade case, as the schema for an existing instance will be in /etc/dirsrv/inst/schema.
We have two options.
First, we can continue to upgrade the schema in /etc/dirsrv/inst/schema, but we just use the source files from /usr instead of /etc/dirsrv/schema.
Alternately, we can remove the schema files from /etc/dirsrv/inst/schema (except 99user.ldif, or other custom schema), and then just rely on the new behaviour. Obviously, we should back these up in this case.
In both cases we still need the upgrade task that removes duplicate elements from 99user.ldif.
I think that given the way the upgrade task is written, the second case may already be the way it's handled.
Fix a crash during upgrade 0001-Ticket-48163-Read-schema-from-multiple-locations.3.patch
I'm leaning toward to push this ticket's milestone to 1.3.6 or even 1.4 since...
Replying to [comment:18 nhosoi]:
I'm leaning toward to push this ticket's milestone to 1.3.6 or even 1.4 since... This is not a customer bug. I understand it is required by Docker, but we have higher priorities in the 1.3.5 timeframe.
Agreed, but the work is almost done.
There are quite a number of upgrade projects from RHDS8 to 10.x happening and it usually fails in the schema upgrade or migration. I'm afraid adding another big change might confuse the field engineers.
This is a very valid concern. I think this alone might be reason to push out to 1.4. Perhaps there is a way we can merge the code, but have it if-defed out, so that we can test it ourselves in private?
There is a schema replication ticket 47749 which is in the FUTURE bucket. #47749 - All standard schema definitions are present in 99user.ldif when the schema is updated in MMR This could be yet another unknown factor on the schema change.
I'm not sure this change would impact that ticket regardless.
So I think that I agree to push this out. How about 1.3.6, because this gives us time to test, and make an informed decision if we want to push out to 1.4 instead?
Semi related, but I think this issue highlights something I have had in my mind for a while, which is that it would be good to have an --with-experimental flag to configure, so that we can have major changes in our code, ifdefed by an EXPERIMENTAL flag. This way we can write code, have it in master, we can test it over a long period, but we also know that it won't go to a release? Probably a topic for email discussion ...
Thank you for understanding, William.
Here's my proposal. 1. Set the target milestone to 1.3.6. 2. Open a ticket to establish a schema migration scenario from 8.2 (that could have custom schema) to the latest schema in the new location. It could be a script or detailed doc. 3. Once we branch 389-ds-base-1.3.5, push your patch only to the master branch.
What do you think?
Yes, I'm happy with that course of action.
Does this mean we are happy to ack the patch as it is, but wait until the 1.3.5 branch point to apply to master?
Replying to [comment:21 firstyear]:
Yes, I'm happy with that course of action. Does this mean we are happy to ack the patch as it is, but wait until the 1.3.5 branch point to apply to master? Correct. Thanks for your patience. :)
Correct. Thanks for your patience. :)
As we mentioned a few months ago, I'm going to apply this to 1.3.6. I'll do this in the next few days to clean it out of trac.
Rebased to latest master. 0001-Ticket-48163-Read-schema-from-multiple-locations.4.patch
Rebased to latest master. 0002-Ticket-48163-Re-space-schema.c.2.patch
Please remove java style comments. Indentation issues and a memset() in init_schema_dse_ext() Indentation issues in slapi_entry_schema_check_ext (not covered by second patch)
Minor issue you probably were not aware of is that we are trying to replace all the slapi_ch_free() calls for strings with slapi_ch_free_string(). This is not a strict rule though, and I think the reason for doing it might no longer apply. So I'll leave it completely up to your discretion - just wanted to make you aware though.
Replying to [comment:25 mreynolds]:
Please remove java style comments.
Fixed
Indentation issues and a memset() in init_schema_dse_ext()
Indentation issues in slapi_entry_schema_check_ext (not covered by second patch)
Fixed!
I think that's a different discussion. For now, I've changed to slapi_ch_free_string, but I think we should bring the topic up and discuss it's relevance? How does that sound.
Updated based on Mark's comments. 0002-Ticket-48163-Re-space-schema.c.3.patch
Updated based on Mark's comments. 0001-Ticket-48163-Read-schema-from-multiple-locations.5.patch
Replying to [comment:26 firstyear]:
Minor issue you probably were not aware of is that we are trying to replace all the slapi_ch_free() calls for strings with slapi_ch_free_string(). This is not a strict rule though, and I think the reason for doing it might no longer apply. So I'll leave it completely up to your discretion - just wanted to make you aware though. I think that's a different discussion. For now, I've changed to slapi_ch_free_string, but I think we should bring the topic up and discuss it's relevance? How does that sound.
Fine by me. Like I said, I think the reason for doing it doesn't matter anymore, but do we keep doing it for consistency? Personally, for purely aesthetic reasons, I like _free_string(char), it's not as ugly as ((void **)&char).
More indentation issues. probably missed these the first time (my bad):
schema.c line - 787, 2210-2216, ...
But it's really hard trying to review two patches that are intertwined. I think there might be more indentation issues, or they might also be fixed? This is why I prefer multiple patches to be merged into a single patch, but I know others on the team strongly disagree with that though.
Either way your patches do not apply to the master branch so I can not properly review them.
I kept them seperate because the patches address different things. One the change in code, one the whitespace. It would be hard to look at what code changed in the middle of the retab. That's why I did that.
I'll rebase to master today, and will repost. I'll look at those lines you are mentioning now too.
Rebase to master, fix a few more spacing issues 0001-Ticket-48163-Read-schema-from-multiple-locations.6.patch
Rebase to master, fix a few more spacing issues 0002-Ticket-48163-Re-space-schema.c.4.patch
At this point, I think any remaining spacing issues were there before. Can we clean these up in another patch?
Looks good
commit 6941dbd commit 848bd6b Writing objects: 100% (25/25), 21.42 KiB | 0 bytes/s, done. Total 25 (delta 19), reused 0 (delta 0) To ssh://git.fedorahosted.org/git/389/ds.git eb358a5..848bd6b master -> master
Metadata Update from @nhosoi: - Issue assigned to firstyear - Issue set to the milestone: 1.3.6 backlog
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/1494
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.