On Thu, Nov 14, 2013 at 1:37 PM, Dimitris Aragiorgis <[email protected]> wrote: > * Dimitris Aragiorgis <[email protected]> [2013-11-07 10:20:02 +0200]: > >> Hi! >> >> * Dimitris Aragiorgis <[email protected]> [2013-11-01 18:01:11 +0200]: >> >> > * Guido Trotter <[email protected]> [2013-11-01 15:57:07 +0100]: >> > >> > > As long assigning node or the cluster IP is still disallowed ok. Is it >> > > possible for the admin to both specify reserved and externally reserved >> > > IPS? It'd be great if it didn't become too simple to make mistakes. Also >> > > if >> > > this series could come with extra tests and documentation updates perhaps >> > > it might be less problematic to have the change in behaviour. >> > > >> > >> > As far as I can see currently one can assing the master ip to a VM >> > via --net ip=... and as a result the VerifyConfig() will complain >> > but ganeti will not abort. >> > >> > With this patch, in order to assign the master ip (that is externally >> > reserved) to a VM we would additionally need to pass >> > --no-conflicts-check option. In this sense it is even harder than before >> > to assign the master's IP to a VM, thus making it not so simple to make >> > mistakes. >> > >> > As far as the documentation is concerned I think I already changed the >> > corresponding parts. I changed a test as well, that was failing silently >> > before. >> > >> > Additionally in case we tweak cluster/node related opcodes to change >> > those reserved IPs in master, this patch does not affect the execution >> > workflow at all. >> > >> >> Any news on that? >> > > Sorry for asking again but is there any blocking reason for these two > patches not to get merged? >
Hi, Yes, I was on vacation and then at LISA and the team waited for me to reply. LGTM, sorry for the delay. Thanks, Guido > Thanks, > dimara > >> > Thanks, >> > dimara >> > >> > > Thanks, >> > > >> > > Guido >> > > On 1 Nov 2013 15:24, "Dimitris Aragiorgis" <[email protected]> wrote: >> > > >> > > > The administrator should be able to assign an externally reserved IP >> > > > to a Ganeti instance manually, if desired. Currently this is not >> > > > supported. External reservations should act as holes in the pool and >> > > > not just as IPs already used by someone outside of Ganeti. >> > > > Automatic allocation with ip=pool will continue to exclude those IPs >> > > > as happens now. >> > > > >> > > > To allow such functionality the administrator needs to pass explicitly >> > > > the desired IP along with the ``--no-conflicts-check`` option, or else >> > > > an error will be produced as happens now. >> > > > >> > > > The aforementioned require the following changes: >> > > > >> > > > - Make IsReserved() to look either in reservations or external ones. >> > > > - Make Reserve() and Release() to use IsReserved() with external >> > > > argument True or False. >> > > > - Pass extra option to ReserveIp() to bypass checking of external >> > > > reservations >> > > > - Update man pages and design doc for this change. >> > > > >> > > > Furthermore, a side effect of this patch is that it fixes the >> > > > following problem: >> > > > Currently, one could not mark an IP as external if it was already >> > > > reserved (i.e. belonged to an instance). The code would produce a >> > > > warning >> > > > and fail silently. >> > > > >> > > > Fix config_mock.py so that if network and ip is given then reserve it >> > > > in >> > > > the pool. >> > > > >> > > > Signed-off-by: Dimitris Aragiorgis <[email protected]> >> > > > --- >> > > > doc/design-network.rst | 23 >> > > > +++++++++++++++-------- >> > > > lib/cmdlib/instance.py | 6 ++++-- >> > > > lib/cmdlib/network.py | 10 ++-------- >> > > > lib/config.py | 9 ++++++--- >> > > > lib/network.py | 25 >> > > > +++++++++++++++++++------ >> > > > man/gnt-instance.rst | 3 +++ >> > > > test/py/cmdlib/testsupport/config_mock.py | 4 ++++ >> > > > 7 files changed, 53 insertions(+), 27 deletions(-) >> > > > >> > > > diff --git a/doc/design-network.rst b/doc/design-network.rst >> > > > index 837924c..8b52f62 100644 >> > > > --- a/doc/design-network.rst >> > > > +++ b/doc/design-network.rst >> > > > @@ -88,21 +88,24 @@ give IP pool management capabilities. A network's >> > > > pool >> > > > is defined by two >> > > > bitfields, the length of the network size each: >> > > > >> > > > ``reservations`` >> > > > - This field holds all IP addresses reserved by Ganeti instances, as >> > > > - well as cluster IP addresses (node addresses + cluster master) >> > > > + This field holds all IP addresses reserved by Ganeti instances. >> > > > >> > > > ``external reservations`` >> > > > This field holds all IP addresses that are manually reserved by the >> > > > - administrator, because some other equipment is using them outside >> > > > the >> > > > - scope of Ganeti. >> > > > + administrator (external gateway, IPs of external servers, etc) or >> > > > + automatically by ganeti (the network/broadcast addresses, >> > > > + Cluster IPs (node addresses + cluster master)). These IPs are >> > > > excluded >> > > > + from the IP pool and cannot be assigned automatically by ganeti to >> > > > + instances (via ip=pool). >> > > > >> > > > The bitfields are implemented using the python-bitarray package for >> > > > space efficiency and their binary value stored base64-encoded for JSON >> > > > compatibility. This approach gives relatively compact representations >> > > > even for large IPv4 networks (e.g. /20). >> > > > >> > > > -Ganeti-owned IP addresses (node + master IPs) are reserved >> > > > automatically >> > > > -if the cluster's data network itself is placed under pool management. >> > > > +Cluster IP addresses (node + master IPs) are reserved automatically >> > > > +as external if the cluster's data network itself is placed under >> > > > +pool management. >> > > > >> > > > Helper ConfigWriter methods provide free IP address generation and >> > > > reservation, using a TemporaryReservationManager. >> > > > @@ -129,10 +132,14 @@ node-specific underlying infrastructure. >> > > > >> > > > We also introduce a new ``ip`` address value, >> > > > ``constants.NIC_IP_POOL``, >> > > > that specifies that a given NIC's IP address should be obtained using >> > > > -the IP address pool of the specified network. This value is only valid >> > > > +the first available IP address inside the pool of the specified >> > > > network. >> > > > +(reservations OR external_reservations). This value is only valid >> > > > for NICs belonging to a network. A NIC's IP address can also be >> > > > specified manually, as long as it is contained in the network the NIC >> > > > -is connected to. >> > > > +is connected to. In case this IP is externally reserved, Ganeti will >> > > > produce >> > > > +an error which the user can override if explicitly requested. Of >> > > > course >> > > > +this IP will be reserved and will not be able to be assigned to >> > > > another >> > > > +instance. >> > > > >> > > > >> > > > Hooks >> > > > diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py >> > > > index 3dee0e3..7c739d5 100644 >> > > > --- a/lib/cmdlib/instance.py >> > > > +++ b/lib/cmdlib/instance.py >> > > > @@ -1039,7 +1039,8 @@ class LUInstanceCreate(LogicalUnit): >> > > > self.LogInfo("Chose IP %s from network %s", nic.ip, >> > > > nobj.name >> > > > ) >> > > > else: >> > > > try: >> > > > - self.cfg.ReserveIp(net_uuid, nic.ip, >> > > > self.proc.GetECId()) >> > > > + self.cfg.ReserveIp(net_uuid, nic.ip, >> > > > self.proc.GetECId(), >> > > > + check=self.op.conflicts_check) >> > > > except errors.ReservationError: >> > > > raise errors.OpPrereqError("IP address %s already in >> > > > use" >> > > > " or does not belong to >> > > > network >> > > > %s" % >> > > > @@ -2624,7 +2625,8 @@ class LUInstanceSetParams(LogicalUnit): >> > > > # Reserve new IP if in the new network if any >> > > > elif new_net_uuid: >> > > > try: >> > > > - self.cfg.ReserveIp(new_net_uuid, new_ip, >> > > > self.proc.GetECId()) >> > > > + self.cfg.ReserveIp(new_net_uuid, new_ip, >> > > > self.proc.GetECId(), >> > > > + check=self.op.conflicts_check) >> > > > self.LogInfo("Reserving IP %s in network %s", >> > > > new_ip, new_net_obj.name) >> > > > except errors.ReservationError: >> > > > diff --git a/lib/cmdlib/network.py b/lib/cmdlib/network.py >> > > > index 01bd948..8bfd0b4 100644 >> > > > --- a/lib/cmdlib/network.py >> > > > +++ b/lib/cmdlib/network.py >> > > > @@ -365,10 +365,7 @@ class LUNetworkSetParams(LogicalUnit): >> > > > if self.op.add_reserved_ips: >> > > > for ip in self.op.add_reserved_ips: >> > > > try: >> > > > - if self.pool.IsReserved(ip): >> > > > - self.LogWarning("IP address %s is already reserved", ip) >> > > > - else: >> > > > - self.pool.Reserve(ip, external=True) >> > > > + self.pool.Reserve(ip, external=True) >> > > > except errors.AddressPoolError, err: >> > > > self.LogWarning("Cannot reserve IP address %s: %s", ip, err) >> > > > >> > > > @@ -378,10 +375,7 @@ class LUNetworkSetParams(LogicalUnit): >> > > > self.LogWarning("Cannot unreserve Gateway's IP") >> > > > continue >> > > > try: >> > > > - if not self.pool.IsReserved(ip): >> > > > - self.LogWarning("IP address %s is already unreserved", ip) >> > > > - else: >> > > > - self.pool.Release(ip, external=True) >> > > > + self.pool.Release(ip, external=True) >> > > > except errors.AddressPoolError, err: >> > > > self.LogWarning("Cannot release IP address %s: %s", ip, err) >> > > > >> > > > diff --git a/lib/config.py b/lib/config.py >> > > > index 1f07d29..56e4c96 100644 >> > > > --- a/lib/config.py >> > > > +++ b/lib/config.py >> > > > @@ -384,7 +384,7 @@ class ConfigWriter(object): >> > > > _, address, _ = self._temporary_ips.Generate([], gen_one, ec_id) >> > > > return address >> > > > >> > > > - def _UnlockedReserveIp(self, net_uuid, address, ec_id): >> > > > + def _UnlockedReserveIp(self, net_uuid, address, ec_id, check=True): >> > > > """Reserve a given IPv4 address for use by an instance. >> > > > >> > > > """ >> > > > @@ -392,22 +392,25 @@ class ConfigWriter(object): >> > > > pool = network.AddressPool(nobj) >> > > > try: >> > > > isreserved = pool.IsReserved(address) >> > > > + isextreserved = pool.IsReserved(address, external=True) >> > > > except errors.AddressPoolError: >> > > > raise errors.ReservationError("IP address not in network") >> > > > if isreserved: >> > > > raise errors.ReservationError("IP address already in use") >> > > > + if check and isextreserved: >> > > > + raise errors.ReservationError("IP is externally reserved") >> > > > >> > > > return self._temporary_ips.Reserve(ec_id, >> > > > (constants.RESERVE_ACTION, >> > > > address, net_uuid)) >> > > > >> > > > @locking.ssynchronized(_config_lock, shared=1) >> > > > - def ReserveIp(self, net_uuid, address, ec_id): >> > > > + def ReserveIp(self, net_uuid, address, ec_id, check=True): >> > > > """Reserve a given IPv4 address for use by an instance. >> > > > >> > > > """ >> > > > if net_uuid: >> > > > - return self._UnlockedReserveIp(net_uuid, address, ec_id) >> > > > + return self._UnlockedReserveIp(net_uuid, address, ec_id, check) >> > > > >> > > > @locking.ssynchronized(_config_lock, shared=1) >> > > > def ReserveLV(self, lv_name, ec_id): >> > > > diff --git a/lib/network.py b/lib/network.py >> > > > index d78b717..291b0a8 100644 >> > > > --- a/lib/network.py >> > > > +++ b/lib/network.py >> > > > @@ -153,8 +153,6 @@ class AddressPool(object): >> > > > def Validate(self): >> > > > assert len(self.reservations) == self._GetSize() >> > > > assert len(self.ext_reservations) == self._GetSize() >> > > > - all_res = self.reservations & self.ext_reservations >> > > > - assert not all_res.any() >> > > > >> > > > if self.gateway is not None: >> > > > assert self.gateway in self.network >> > > > @@ -188,25 +186,40 @@ class AddressPool(object): >> > > > """ >> > > > return self.all_reservations.to01().replace("1", "X").replace("0", >> > > > ".") >> > > > >> > > > - def IsReserved(self, address): >> > > > + def IsReserved(self, address, external=False): >> > > > """Checks if the given IP is reserved. >> > > > >> > > > """ >> > > > idx = self._GetAddrIndex(address) >> > > > - return self.all_reservations[idx] >> > > > + if external: >> > > > + return self.ext_reservations[idx] >> > > > + else: >> > > > + return self.reservations[idx] >> > > > >> > > > def Reserve(self, address, external=False): >> > > > """Mark an address as used. >> > > > >> > > > """ >> > > > - if self.IsReserved(address): >> > > > - raise errors.AddressPoolError("%s is already reserved" % >> > > > address) >> > > > + if self.IsReserved(address, external): >> > > > + if external: >> > > > + msg = "IP %s is already externally reserved" % address >> > > > + else: >> > > > + msg = "IP %s is already used by an instance" % address >> > > > + raise errors.AddressPoolError(msg) >> > > > + >> > > > self._Mark(address, external=external) >> > > > >> > > > def Release(self, address, external=False): >> > > > """Release a given address reservation. >> > > > >> > > > """ >> > > > + if not self.IsReserved(address, external): >> > > > + if external: >> > > > + msg = "IP %s is not externally reserved" % address >> > > > + else: >> > > > + msg = "IP %s is not used by an instance" % address >> > > > + raise errors.AddressPoolError(msg) >> > > > + >> > > > self._Mark(address, value=False, external=external) >> > > > >> > > > def GetFreeAddress(self): >> > > > diff --git a/man/gnt-instance.rst b/man/gnt-instance.rst >> > > > index d48adb1..c133a72 100644 >> > > > --- a/man/gnt-instance.rst >> > > > +++ b/man/gnt-instance.rst >> > > > @@ -135,6 +135,9 @@ ip >> > > > connected to the said network. ``--no-conflicts-check`` can be >> > > > used >> > > > to override this check. The special value **pool** causes Ganeti >> > > > to >> > > > select an IP from the the network the NIC is or will be connected >> > > > to. >> > > > + One can pick an externally reserved IP of a network along with >> > > > + ``--no-conflict-check``. Note that this IP cannot be assigned to >> > > > + any other instance until it gets released. >> > > > >> > > > mode >> > > > specifies the connection mode for this NIC: routed, bridged or >> > > > diff --git a/test/py/cmdlib/testsupport/config_mock.py >> > > > b/test/py/cmdlib/testsupport/config_mock.py >> > > > index 3c8812d..4579cb9 100644 >> > > > --- a/test/py/cmdlib/testsupport/config_mock.py >> > > > +++ b/test/py/cmdlib/testsupport/config_mock.py >> > > > @@ -28,6 +28,7 @@ import uuid as uuid_module >> > > > from ganeti import config >> > > > from ganeti import constants >> > > > from ganeti import objects >> > > > +from ganeti.network import AddressPool >> > > > >> > > > import mocks >> > > > >> > > > @@ -507,6 +508,9 @@ class ConfigMock(config.ConfigWriter): >> > > > if mac is None: >> > > > mac = "aa:00:00:aa:%02x:%02x" % (nic_id / 0xff, nic_id % 0xff) >> > > > if isinstance(network, objects.Network): >> > > > + if ip: >> > > > + pool = AddressPool(network) >> > > > + pool.Reserve(ip) >> > > > network = network.uuid >> > > > if nicparams is None: >> > > > nicparams = {} >> > > > -- >> > > > 1.7.10.4 >> > > > >> > > > >> >> > > > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.10 (GNU/Linux) > > iQEcBAEBAgAGBQJShMQdAAoJEHFDHex6CBG9RU8IALElWK0WsGJcpoJHU6OwhHfa > VHk3wgzuXxyremoe3IY5w7INwL9bWq1g+ovLxc32mJcKhM1OqHdQaEIGXcs46L5t > uzU5Ns1RtHFPNGvXJQ+TIbQvW9FJ+3R+vS3PKr7rsm2WJEXG1nuaLJWk1oLJi3Hh > mbGr0bKE21MEtm+ZvN50rFS7afrIt9xjq27XgKledYiigFyVh10tMEH0bcfcVizV > 6CiHfuXdSGmYKlkhbjATXj//uHC1FBUKiTnZ3TzQA0eb4iixgIUm1ILg+tl404T/ > i/XkmotRy4OfUT2Hpe3h8bmDZIElMse9qw4nBZU4L48ApkXx2Lyc7OgxE2mxjsc= > =vjWS > -----END PGP SIGNATURE----- > -- Guido Trotter Ganeti Engineering 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 Steuernummer: 48/725/00206 Umsatzsteueridentifikationsnummer: DE813741370
