On Sun, Feb 16, 2014 at 12:33 AM, Dimitris Aragiorgis <[email protected]> wrote: > * Michele Tartara <[email protected]> [2014-02-13 16:20:22 +0100]: > >> On Thu, Feb 13, 2014 at 3:46 PM, Dimitris Aragiorgis <[email protected]> wrote: >> > During backup import/export SafeConfigParser() is used to >> > save/restore instance's configuration. There is a possibility if an >> > export is done with a different Ganeti version, a specific value not >> > to be saved during export (e.g. the NIC/Disk name) but still >> > requested during import. >> > >> > With this patch we override the get() method of SafeConfigParser() >> > and catch NoOptionError if raised and return None. Additionally we >> > translate "None" values read from .ini file into None. >> > >> > Signed-off-by: Dimitris Aragiorgis <[email protected]> >> > --- >> > lib/objects.py | 15 ++++++++++++++- >> > 1 file changed, 14 insertions(+), 1 deletion(-) >> > >> > diff --git a/lib/objects.py b/lib/objects.py >> > index 358f108..6aca6fc 100644 >> > --- a/lib/objects.py >> > +++ b/lib/objects.py >> > @@ -2159,7 +2159,8 @@ class Network(TaggableObject): >> > return obj >> > >> > >> > -class SerializableConfigParser(ConfigParser.SafeConfigParser): >> > +# need to inherit object in order to use super() >> > +class SerializableConfigParser(ConfigParser.SafeConfigParser, object): >> > """Simple wrapper over ConfigParse that allows serialization. >> > >> > This class is basically ConfigParser.SafeConfigParser with two >> > @@ -2181,6 +2182,18 @@ class >> > SerializableConfigParser(ConfigParser.SafeConfigParser): >> > cfp.readfp(buf) >> > return cfp >> > >> > + def get(self, section, option, **kwargs): >> > + value = None >> > + try: >> > + value = super(SerializableConfigParser, self).get(section, option, >> > + **kwargs) >> > + if value.lower() == constants.VALUE_NONE: >> > + value = None >> > + except ConfigParser.NoOptionError: >> > + pass >> > + >> > + return value >> > + >> > >> > class LvmPvInfo(ConfigObject): >> > """Information about an LVM physical volume (PV). >> > -- >> > 1.7.10.4 >> > >> >> I think we should at least log that an error happened. Silently >> accepting it seems to me extremely dangerous. >> > > Sure. Maybe "extremely" is too much here, but ok :) > >> Also, I'm not confident enough about this part of the code to say that >> all the values can be optional if not found. So, what about >> specifically checking whether we have been looking for the disk name >> and nick name and ONLY in that case returning None instead of the >> exception? > > Well I think this can get kinda ugly. What if another option gets added or > removed in the future? Are we going to add another if: then: else?
Yes. Actually I was thinking more of a: if option in ["disk", "nic", ...]: pass else: raise again the exception > >> This way, we know that we are modifying only the behavior related to >> the new configuration fields. >> > > What is actually new? Name is a NIC/Disk parameter since 2.7... Of course. I meant it as "newly exported". It's not a new parameter per se. > > How about adding just the logging? Right now we have a really strict checking on the options. Allowing a specific set of them to be optional is ok, but suddenly turning off any check on the options is not a good thing. Also, it's not like we are going to have hundreds of optional ones, so the list is still going to be really small and updated only rarely. > > Thanks, > dimara > >> Cheers, >> Michele >> Thanks, Michele -- Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores
