On 06/12/2014 02:37 PM, Nathaniel McCallum wrote: > On Thu, 2014-06-12 at 13:29 +0200, Tomas Babej wrote: >> On 06/12/2014 10:45 AM, Martin Kosek wrote: >>> On 06/11/2014 06:49 PM, Nathaniel McCallum wrote: >>>> On Wed, 2014-06-11 at 11:08 +0200, Tomas Babej wrote: >>>>> Hi, >>>>> >>>>> As due to possible race conditions, the preop.pin might not be >>>>> written in the CS.cfg at the time installer tries to read it. >>>>> >>>>> In case no value for preop.pin was found, retry until timeout >>>>> was reached. >>>>> >>>>> https://fedorahosted.org/freeipa/ticket/3382 >>>>> >>>>> (applies on ipa-3-0 branch) >>>> There is inconsistent spacing around '='. For instance: >>>> + f=open(filename, 'r') >>>> + data = f.read() >>>> >>>> Also, I'm fairly certain this is incorrect: >>>> + total_timeout =+ 1 >>>> >>>> Nathaniel >>> +1, this is certainly is not a deterministic, constant measuring of a >>> timeout. >> Fixed. >> >>> I would rather see something like >>> >>> op_timeout = time.time() + api.env.startup_timeout >>> >>> while preop_pin is None and time.time() < op_timeout: >>> >>> >>> Also, I do not think this will work, IOError is risen when a file is not >>> found, >>> so this code would not solve the problem if waiting on file to appear. >> Yes, but the problem was not in being unable to open the file, but that >> certain file content >> is not in the file yet. It seems that pkicreate creates the file in >> time, but the content written >> later and that causes the race condition. >> >> If you inspect the original code, and bugzilla, you can see that >> installation fails with: >> >> 2013-01-25T02:58:44Z INFO The ipa-server-install command failed, exception: >> RuntimeError: Unable to find preop.pin in /var/lib/pki-ca/conf/CS.cfg. Is >> your CA already configured? >> >> While the original code >> >> # read the config file and get the preop pin >> >> try: >> >> >> >> f=open(filename) >> >> >> except IOError, >> e: >> >> >> root_logger.error("Cannot open configuration file." + >> str(e)) >> raise >> e >> >> >> data = >> f.read() >> >> >> data = >> data.split('\n') >> >> >> pattern = re.compile("preop.pin=(.*)" >> ) >> for line in >> data: >> >> >> match = re.search(pattern, >> line) >> if >> (match): >> >> >> >> preop_pin=match.group(1) >> >> >> break >> if preop_pin is None: >> raise RuntimeError("Unable to find preop.pin in %s. Is your CA >> already configured?" % filename) >> >> does raise the IOError in case the file was not able to be opened. Since >> we get the Runtime error, >> file must exist, only the desired content is missing. >> >> That said, I have no objections to amending the patch so that it >> properly handles this race condition >> we did not encounter. Better safe than sorry, it's a simple change and >> already included in the >> current iteration of the patch. >> >> Updated patch attached. > I think I would prefer something like inotify watching for > IN_CLOSE_WRITE rather than this polling. > > Nathaniel > Wouldn't that be a little bit of an overkill for that purpose?
- we'll need to introduce an additional dependency for python-inotify - watching for IN_CLOSE_WRITE is not equivalent with the preop_pin written to CS.cfg (and we don't know that unless we inspect the code for pkicreate, which in turn would make our code dependant on code not located in our codebase), so we will still need to check if preop_pin is there, and loop over if not -- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
