Skip to content

CLOUDSTACK-8799 fixed the defalut routes #784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bvbharatk wants to merge 2 commits into from
Closed

CLOUDSTACK-8799 fixed the defalut routes #784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3effdbe
CLOUDSTACK-8799 fixed the defalut routes
bvbharat Sep 4, 2015
da17c14
CLOUDSTACK-8799 fixed for vpc networks.
bvbharat Sep 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 34 additions & 10 deletions systemvm/patches/debian/config/opt/cloud/bin/cs/CsAddress.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,18 +95,43 @@ def get_control_if(self):
return ip
return None

def check_if_link_exists(self,dev):
cmd="ip link show dev %s"%dev
result=CsHelper.execute(cmd)
if(len(result)!=0):
return True
else:
return False

def check_if_link_up(self,dev):
cmd="ip link show dev %s | tr '\n' ' ' | cut -d ' ' -f 9"%dev
Copy link
Member

@yadvr yadvr Sep 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use a better string processing here, instead of using hardcoded cut/trim limits?

Copy link
Contributor

@wido wido Sep 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed and also use the -o flag for 'ip':

ip -o link show dev

Then split the string using Python.

Copy link
Contributor Author

@bvbharatk bvbharatk Sep 9, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

the output of Ip link show command is not expected to changes so in this case i think hardcoding the limiters is not an issue.

Copy link
Contributor

@wido wido Sep 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that we should do the splitting in Python.

Execute ip with the -o flag so it is a one-liner and split with Python afterwards.

Copy link
Contributor

@remibergsma remibergsma Sep 10, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you only want the state, why not simply do this? No magic needed.
cat /sys/class/net/eth0/operstate

Copy link
Contributor Author

@bvbharatk bvbharatk Sep 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

just out of curiosity why should we do the splitting in python and what is wrong in doing it this way.

Copy link
Contributor

@wido wido Sep 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep as much logic in Python as possible. The performance difference is small, but with tr and grep you spawn subprocessess again.

But as @remibergsma says. Try to open that file in /sys and parse the contents. You can use the simple Python file functions.

No need to execute IP. The less subprocesses we execute, the better.

Copy link
Contributor Author

@bvbharatk bvbharatk Sep 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
we have used the subprocess command to find the state of the device in two more places. I have created a improvement ticket(CLOUDSTACK-8837) to track this. I will work on this later, until then
i guess the current changes will do.

result=CsHelper.execute(cmd)
if(result[0].lower()=="up"):
return True
else:
return False


def process(self):
route = CsRoute()
found_defaultroute = False

for dev in self.dbag:
if dev == "id":
continue
ip = CsIP(dev, self.config)

for address in self.dbag[dev]:
if(address["nw_type"]!="public"):
continue

#check if link is up
if (not self.check_if_link_exists(dev)):
logging.info("link %s does not exist, so not processing"%dev)
continue
if not self.check_if_link_up(dev):
cmd="ip link set %s up"%dev
CsHelper.execute(cmd)

gateway = str(address["gateway"])
network = str(address["network"])

ip.setAddress(address)
Expand All @@ -122,16 +147,15 @@ def process(self):
"Address %s on device %s not configured", ip.ip(), dev)
if CsDevice(dev, self.config).waitfordevice():
ip.configure()
route.add_route(dev, network)

if address["nw_type"] != "control":
route.add_route(dev, network)
# once we start processing public ip's we need to verify there
# is a default route and add if needed
if not route.defaultroute_exists():
cmdline=self.config.get_cmdline_instance()
if(cmdline.get_gateway()):
route.add_defaultroute(cmdline.get_gateway())

# once we start processing public ip's we need to verify there
# is a default route and add if needed
if address["nw_type"] == "public" and not found_defaultroute:
if not route.defaultroute_exists():
if route.add_defaultroute(gateway):
found_defaultroute = True


class CsInterface:
Expand Down
4 changes: 4 additions & 0 deletions systemvm/patches/debian/config/opt/cloud/bin/cs/CsDatabag.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,7 @@ def get_router_password(self):
md5 = hashlib.md5()
md5.update(passwd)
return md5.hexdigest()
def get_gateway(self):
if "gateway" in self.idata():
return self.idata()['gateway']
return False
17 changes: 3 additions & 14 deletions systemvm/patches/debian/config/opt/cloud/bin/cs/CsRedundant.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,20 +261,9 @@ def set_master(self):

self.set_lock()
logging.debug("Setting router to master")
ads = [o for o in self.address.get_ips() if o.is_public()]
dev = ''
for o in ads:
if dev == o.get_device():
continue
cmd2 = "ip link set %s up" % o.get_device()
if CsDevice(o.get_device(), self.config).waitfordevice():
CsHelper.execute(cmd2)
dev = o.get_device()
logging.info("Bringing public interface %s up" %
o.get_device())
else:
logging.error(
"Device %s was not ready could not bring it up" % o.get_device())
self.address.process()
logging.info("added default routes")

# ip route add default via $gw table Table_$dev proto static
cmd = "%s -C %s" % (self.CONNTRACKD_BIN, self.CONNTRACKD_CONF)
CsHelper.execute("%s -c" % cmd)
Expand Down