On Thu, Feb 20, 2014 at 11:51 AM, Dimitris Aragiorgis <[email protected]> wrote: > * Michele Tartara <[email protected]> [2014-02-19 15:51:56 +0100]: > >> 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 >> > > How about this interdiff: > > diff --git a/lib/objects.py b/lib/objects.py > index 6aca6fc..8b38e81 100644 > --- a/lib/objects.py > +++ b/lib/objects.py > @@ -2190,7 +2190,12 @@ class > SerializableConfigParser(ConfigParser.SafeConfigParser, object): > if value.lower() == constants.VALUE_NONE: > value = None > except ConfigParser.NoOptionError: > - pass > + r = re.compile(r"(disk|nic)\d+_name") > + match = r.match(option) > + if match: > + pass > + else: > + raise > > return value > > > > >> > >> >> 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
LGTM, 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
