[SSSD] [PATCH] CONFDB: Assume config file version 2 if missing

Jakub Hrozek jhrozek at redhat.com
Mon Sep 14 09:10:53 UTC 2015


On Thu, Sep 03, 2015 at 02:29:16PM +0200, Michal Židek wrote:
> On 09/03/2015 09:58 AM, Lukas Slebodnik wrote:
> >On (03/09/15 09:35), Jakub Hrozek wrote:
> >>On Wed, Sep 02, 2015 at 02:52:42PM +0200, Lukas Slebodnik wrote:
> >>>On (01/09/15 12:55), Michal Židek wrote:
> >>>>On 09/01/2015 11:11 AM, Jakub Hrozek wrote:
> >>>>>On Tue, Sep 01, 2015 at 11:00:15AM +0200, Lukas Slebodnik wrote:
> >>>>>>On (01/09/15 10:51), Jakub Hrozek wrote:
> >>>>>>>On Tue, Aug 11, 2015 at 07:47:00AM +0200, Lukas Slebodnik wrote:
> >>>>>>>>On (10/08/15 17:18), Michal Židek wrote:
> >>>>>>>>>On 08/06/2015 01:39 PM, Lukas Slebodnik wrote:
> >>>>>>>>>>On (05/08/15 13:41), Michal Židek wrote:
> >>>>>>>>>>>On 07/30/2015 06:08 PM, Lukas Slebodnik wrote:
> >>>>>>>>>>>>>>>-    } else if (version < CONFDB_VERSION_INT) {
> >>>>>>>>>>>>>>>-        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>>>>>>>>>>>>-                "Config file is an old version. "
> >>>>>>>>>>>>>>>-                 "Please run configuration upgrade script.\n");
> >>>>>>>>>>>>>>>-        ret = EINVAL;
> >>>>>>>>>>>>>>>-        goto done;
> >>>>>>>>>>>>>>>-    } else if (version > CONFDB_VERSION_INT) {
> >>>>>>>>>>>>>>>-        DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>>>>>>>>>>>>-                "Config file version is newer than confdb\n");
> >>>>>>>>>>>>>>>-        ret = EINVAL;
> >>>>>>>>>>>>>>>-        goto done;
> >>>>>>>>>>>>>>>+        /* No known version. Use default. */
> >>>>>>>>>>>>>>>+        DEBUG(SSSDBG_CONF_SETTINGS,
> >>>>>>>>>>>>>>>+              "Value of config_file_version option not found. "
> >>>>>>>>>>>>>>>+              "Assumed to be version %d.\n", CONFDB_DEFAULT_CFG_FILE_VER);
> >>>>>>>>>>>>>>>+    } else {
> >>>>>>>>>>>>>>>+        version = sss_ini_get_int_config_value(init_data,
> >>>>>>>>>>>>>>>+                                               CONFDB_DEFAULT_CFG_FILE_VER,
> >>>>>>>>>>>>>>>+                                               -1, &ret);
> >>>>>>>>>>>>>>>+        if (ret != EOK) {
> >>>>>>>>>>>>>>>+            DEBUG(SSSDBG_FATAL_FAILURE,
> >>>>>>>>>>>>>>>+                    "Config file version could not be determined\n");
> >>>>>>>>>>>>                       ^^
> >>>>>>>>>>>>I do not prefer nested "if"s. If you decided to do it in this way then
> >>>>>>>>>>>
> >>>>>>>>>>>Me neither, but sss_ini_get_int_config_value() has to be
> >>>>>>>>>>>skipped conditionally. It is just call to the function
> >>>>>>>>>>>plus error checking that is nested. I think it is not
> >>>>>>>>>>>too bad in this case.
> >>>>>>>>>>>
> >>>>>>>>>>>>you shoudl have proper indentation.
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>Fixed in the new version.
> >>>>>>>>>>>
> >>>>>>>>>>It works because integration tests passed.
> >>>>>>>>>>http://sssd-ci.duckdns.org/logs/job/20/68/summary.html
> >>>>>>>>>>But ...
> >>>>>>>>>>
> >>>>>>>>>>I tested new version with ipa-client-install
> >>>>>>>>>>and "config_file_version = 2" is still added to sssd.conf
> >>>>>>>>>>even though it is a default value.
> >>>>>>>>>>
> >>>>>>>>>>ipa-client-install uses our python API (python-sssdconfig)
> >>>>>>>>>>and it does not try to add this option itself.
> >>>>>>>>>
> >>>>>>>>>I often change version of SSSD with git checkout sssd<version>.
> >>>>>>>>>If I generate the config with realmd or ipa-client-install
> >>>>>>>>realmd do not use python module SSSDConfig.
> >>>>>>>>
> >>>>>>>>>with latest version then the config_version_file
> >>>>>>>>>would need to be added manually (and I am pretty
> >>>>>>>>>sure it would be after I looked into logs to see why sssd
> >>>>>>>>>is not starting). I know this will probably only hit
> >>>>>>>>>testers/developers, but I would prefer not to add unnecessary
> >>>>>>>>just developers and developers useually join machine
> >>>>>>>>to AD or IPA just once.
> >>>>>>>>
> >>>>>>>>>little inconveniences.
> >>>>>>>>>
> >>>>>>>>I do not try old sssd version very often. Just in case of bisect.
> >>>>>>>>and ther is nice/simple workarount for "little inconvenience"
> >>>>>>>>
> >>>>>>>>Just manually add "config_version_file = 2" to sssd.conf
> >>>>>>>>
> >>>>>>>>The main point of this ticket is to simplify sssd.conf
> >>>>>>>>and our python sssdconfig API should do the same.
> >>>>>>>>Otherwise we do not need to do such change.
> >>>>>>>>
> >>>>>>>>I still see "config_file_version = 2" in sssd.conf
> >>>>>>>>It was generated by python module SSSDConfig (via ipa-client-install)
> >>>>>>>>
> >>>>>>>>LS
> >>>>>>>
> >>>>>>>This thread has stalled, let's try to restart it.
> >>>>>>>
> >>>>>>>What versions are normally still being worked on by developers? I
> >>>>>>>suspect it's master and sssd-1-12. What about pushing the patch to
> >>>>>>>sssd-1-12 as well?
> >>>>>>I'm fine with sssd-1-12.
> >>>>>>The python sssdconfig need to be updated.
> >>>>>>
> >>>>>>LS
> >>>>>
> >>>>>Michal, if you agree, please update the patches so that we can push them
> >>>>>in time for sssd-1.13.1
> >>>>
> >>>>New patch attached. I tested ipa-client-install and the
> >>>>config_file_version is no longer added.
> >>>>
> >>>>Michal
> >>>>
> >>>
> >>>>From f61ca88d69b3a56645b7473928b0674ac4bceff8 Mon Sep 17 00:00:00 2001
> >>>>From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek at redhat.com>
> >>>>Date: Tue, 7 Jul 2015 15:15:32 +0200
> >>>>Subject: [PATCH] CONFDB: Assume config file version 2 if missing
> >>>>
> >>>>Default to config file version 2 if the version
> >>>>is not specified explicitly.
> >>>>
> >>>>Ticket:
> >>>>https://fedorahosted.org/sssd/ticket/2688
> >>>>---
> >>>* integration tests passed
> >>>* ipa-client generated config without config_file_version and sssd worked.
> >>>
> >>>ACK
> >>>
> >>>LS
> >>
> >>* master: 175613be0cfb0890174d12d941e634d833b63dd9
> >>
> >>Michal, can you rebase the patch for sssd-1-12 ?
> >The conflict might me caused by missing integration tests in 1.12
> >
> >LS
> 
> Indeed it was the tests.
> 
> Patch for 1.12 is attached.
> 
> Michal
> 

CI had some issues, but I don't think those are related to your patch:
http://sssd-ci.duckdns.org/logs/job/26/52/summary.html

ACK for sssd-1-12


More information about the sssd-devel mailing list