You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by wilderrodrigues <gi...@git.apache.org> on 2015/07/14 15:04:19 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPR with both ...
GitHub user wilderrodrigues opened a pull request:
https://github.com/apache/cloudstack/pull/587
CLOUDSTACK-8616: Redundant VPR with both routers as Master
This PR contains some refactoring of the Python code used by the redundant routers and also a fix for the intermittent problem when running the rVPC component tests.
To summarise it:
* If the KeepaloiveD configuration file changes, restart the service instead of reloading it.
* Since we are configuring KeepaliveD/VRRP in no-preemptive mode, we no longer need priorities. As a matter of fact, the Management Server was not sending priorities to the routers anymore. The value used in the old configuration was defaulted to 99 in the Python code.
* KeepaliveD and ConntractD, once configured in a router, will have a cronjob that will run on reboot. So, the services will be restarted without the need to wait for the management server to send some configuration and force a restart.
* Installing KeepaliveD from Wheezy-Backports in order to have a newer version available.
I already squashed few commits of this PR so we wouldn't have to go through simple fixes/typos that happened during the tryouts. When opening the commits for review please note that the commit messages also contain the messages of the squashed commits.
Adding the cronjob to restart the KeepaliveD service on reboot helped to get a 60% success rate with the tests. Before that, the tests were failing very often: 4 out of 5 times.
I then added the "restart" when configuration changes instead of "reload". Once the change was applied, I successfully executed the tests 13 times. That gives confidence.
Tests can be executed with the following command:
nosetests --with-marvin --marvin-config=[your_configuration_file] -s -a tags=advanced,required_hardware=true component/test_vpc_redundant.py
Since there were changes on marvin/base.py - in the previous PR, you will need to build/upgrade your Marvin installation.
@DaanHoogland @bhaisaab @remibergsma, could you please have a look at this PR?
Cheers,
Wilder
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/schubergphilis/cloudstack fix/CLOUDSTACK-8616
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/587.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #587
----
commit c35c6661696ab3c3c1ddfb6794bd293a76b2463b
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-07-08T05:24:35Z
CLOUDSTACK-8616 - Removing the Priority form KeepaliveD configuration
- We use no preempt mode with state set as EQUAL to both nodes, no need to have Priotities setup
- Do not add IPs as comments to the configuration. If a new guest interface is added, the file will change anyway.
- This was used in the past when keepalived would restart for each new interface added
- Removed the long sleep form the tests: we now sleep 5 seconds per PF rule added
CLOUDSTACK-8616 - Fix keepalived.ts/2 files comparison
- Add call to set_fault() in case of router transits to that state
- Removing commented out code
CLOUDSTACK-8616 - Fixing check_heartbeat.sh.templ
CLOUDSTACK-8616 - Call set_fault from the check_heartbeat.sh script
commit c975185318cbfd00e9d5e346b4fc9ea2c76e8098
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-07-09T09:40:32Z
CLOUDSTACK-8616 - Add keepalived start on reboot
- Runs check_heartbeat.sh every 30 seconds
CLOUDSTACK-861 - Copy/Paste error
- Paste the wrong command in the crontab line.
commit c20b5f3ff1e56b4db296bd2ec46f0cd8ed538b29
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-07-10T06:41:28Z
CLOUDSTACK-8616 - Installing KeepaliveD from Debian Wheezy backports
- preempt delay reverted on version 1.2.13 - from the backports
- vrrp : Revert "Honor preempt_delay setting on startup.".
- See changelog: http://www.keepalived.org/changelog.html
- Refactoring some variable names to avoid misunderstanding
commit 118d7b79f4f5f15a9931ff1cc6e2cc91a562ee11
Author: wilderrodrigues <wr...@schubergphilis.com>
Date: 2015-07-13T17:29:41Z
CLOUDSTACK-8616 - Add a cron job to restart ConntrackD on reboot
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/587#discussion_r34578120
--- Diff: systemvm/patches/debian/buildsystemvm.sh ---
@@ -375,8 +375,10 @@ packages() {
chroot . apt-get --no-install-recommends -q -y --force-yes install open-vm-tools
#xenstore utils
chroot . apt-get --no-install-recommends -q -y --force-yes install xenstore-utils libxenstore3.0
- #keepalived and conntrackd
- chroot . apt-get --no-install-recommends -q -y --force-yes install keepalived conntrackd ipvsadm libnetfilter-conntrack3 libnl1
+ #keepalived - install version 1.2.13 from wheezy backports
+ chroot . apt-get --no-install-recommends -q -y --force-yes -t wheezy-backports install keepalived
--- End diff --
Any package related change needs to go into: tools/appliance/definitions/systemvmtemplate/install_systemvm_packages.sh
AFAIK, this buildsystemvm script is not used or maintained by anyone
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/587#discussion_r34755507
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsFile.py ---
@@ -35,10 +35,9 @@ def load(self):
self.new_config.append(line)
except IOError:
logging.debug("File %s does not exist" % self.filename)
- return
else:
logging.debug("Reading file %s" % self.filename)
- self.config = copy.deepcopy(self.new_config)
+ self.config = list(self.new_config)
--- End diff --
Alright, as I mentioned in my comment if the list contains (references to) other objects then only copy.deepcopy should be used. I was not clear about the usage, after reading the code I think it would work since we're working with list of strings (read from file), followed by append/insert operations; list() would be indeed faster than copy.deepcopy in this case as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/587#discussion_r34579273
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsFile.py ---
@@ -35,10 +35,9 @@ def load(self):
self.new_config.append(line)
except IOError:
logging.debug("File %s does not exist" % self.filename)
- return
else:
logging.debug("Reading file %s" % self.filename)
- self.config = copy.deepcopy(self.new_config)
+ self.config = list(self.new_config)
--- End diff --
copy.deepcopy may be a necessary evil, if the list new_config contains objects. I wrote an example to test this: https://gist.github.com/bhaisaab/fe85fb02e1a7fda2fd2d
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/587#discussion_r34770678
--- Diff: systemvm/patches/debian/buildsystemvm.sh ---
@@ -375,8 +375,10 @@ packages() {
chroot . apt-get --no-install-recommends -q -y --force-yes install open-vm-tools
#xenstore utils
chroot . apt-get --no-install-recommends -q -y --force-yes install xenstore-utils libxenstore3.0
- #keepalived and conntrackd
- chroot . apt-get --no-install-recommends -q -y --force-yes install keepalived conntrackd ipvsadm libnetfilter-conntrack3 libnl1
+ #keepalived - install version 1.2.13 from wheezy backports
+ chroot . apt-get --no-install-recommends -q -y --force-yes -t wheezy-backports install keepalived
--- End diff --
discussed with @wilderrodrigues : will add a note in this PR that this file is being phased out
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/587#discussion_r34586102
--- Diff: systemvm/patches/debian/buildsystemvm.sh ---
@@ -375,8 +375,10 @@ packages() {
chroot . apt-get --no-install-recommends -q -y --force-yes install open-vm-tools
#xenstore utils
chroot . apt-get --no-install-recommends -q -y --force-yes install xenstore-utils libxenstore3.0
- #keepalived and conntrackd
- chroot . apt-get --no-install-recommends -q -y --force-yes install keepalived conntrackd ipvsadm libnetfilter-conntrack3 libnl1
+ #keepalived - install version 1.2.13 from wheezy backports
+ chroot . apt-get --no-install-recommends -q -y --force-yes -t wheezy-backports install keepalived
--- End diff --
That's a good point, @bhaisaab . I blindly trusted the script and did not check the version before. I just created a rVPC and checked the router to confirm that the version was not updated. I will fix that one, test and push to the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/587#discussion_r34771180
--- Diff: systemvm/patches/debian/buildsystemvm.sh ---
@@ -375,8 +375,10 @@ packages() {
chroot . apt-get --no-install-recommends -q -y --force-yes install open-vm-tools
#xenstore utils
chroot . apt-get --no-install-recommends -q -y --force-yes install xenstore-utils libxenstore3.0
- #keepalived and conntrackd
- chroot . apt-get --no-install-recommends -q -y --force-yes install keepalived conntrackd ipvsadm libnetfilter-conntrack3 libnl1
+ #keepalived - install version 1.2.13 from wheezy backports
+ chroot . apt-get --no-install-recommends -q -y --force-yes -t wheezy-backports install keepalived
--- End diff --
Added the following not to the file and will push it to the PR:
#
# IMPORTANT REMARK
# Package intallation is no longer done via this script. We are not removing the code yet, but we want to
# make sure that everybody willing to install/update packages should refer to the file:
# ==> cloud-tools/appliance/definitions/systemvmtemplate/install_systemvm_packages.sh
#
Is it good enough?
Cheers,
Wilder
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/587#discussion_r34589895
--- Diff: systemvm/patches/debian/config/opt/cloud/templates/check_heartbeat.sh.templ ---
@@ -47,13 +47,14 @@ then
if [ $s -gt 2 ]
then
echo Keepalived process is dead! >> $ROUTER_LOG
- $ROUTER_BIN_PATH/services.sh stop >> $ROUTER_LOG 2>&1
- $ROUTER_BIN_PATH/disable_pubip.sh >> $ROUTER_LOG 2>&1
- $ROUTER_BIN_PATH/primary-backup.sh fault >> $ROUTER_LOG 2>&1
service keepalived stop >> $ROUTER_LOG 2>&1
service conntrackd stop >> $ROUTER_LOG 2>&1
- pkill -9 keepalived >> $ROUTER_LOG 2>&1
- pkill -9 conntrackd >> $ROUTER_LOG 2>&1
+
+ #Set fault so we have the same effect as a KeepaliveD fault.
+ python /opt/cloud/bin/master.py --fault
--- End diff --
Yep... it will block.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/587#discussion_r34579656
--- Diff: systemvm/patches/debian/config/opt/cloud/templates/check_heartbeat.sh.templ ---
@@ -47,13 +47,14 @@ then
if [ $s -gt 2 ]
then
echo Keepalived process is dead! >> $ROUTER_LOG
- $ROUTER_BIN_PATH/services.sh stop >> $ROUTER_LOG 2>&1
- $ROUTER_BIN_PATH/disable_pubip.sh >> $ROUTER_LOG 2>&1
- $ROUTER_BIN_PATH/primary-backup.sh fault >> $ROUTER_LOG 2>&1
service keepalived stop >> $ROUTER_LOG 2>&1
service conntrackd stop >> $ROUTER_LOG 2>&1
- pkill -9 keepalived >> $ROUTER_LOG 2>&1
- pkill -9 conntrackd >> $ROUTER_LOG 2>&1
+
+ #Set fault so we have the same effect as a KeepaliveD fault.
+ python /opt/cloud/bin/master.py --fault
--- End diff --
will running the script here block this shell script until it exists, or will it launch as a daemon? If it's blocking then it's alright.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/587#issuecomment-121928006
lgtm
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by wilderrodrigues <gi...@git.apache.org>.
Github user wilderrodrigues commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/587#discussion_r34588816
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsFile.py ---
@@ -35,10 +35,9 @@ def load(self):
self.new_config.append(line)
except IOError:
logging.debug("File %s does not exist" % self.filename)
- return
else:
logging.debug("Reading file %s" % self.filename)
- self.config = copy.deepcopy(self.new_config)
+ self.config = list(self.new_config)
--- End diff --
In the way you put it, accessing the indexes, it changes both probably due to some reference. However, if one adds some other cases - mainly the uso of append(), as we have - you will see that it works. Check below:
>>> a = 1
>>> b = [1,2,3]
>>> c = "some random string :)"
>>> z = [a,b,c]
>>> x = list(z)
>>> x[1][1] = 100
>>> print x
[1, [1, 100, 3], 'some random string :)']
>>> print z
[1, [1, 100, 3], 'some random string :)']
>>> z = []
>>> print z
[]
>>> print x
[1, [1, 100, 3], 'some random string :)']
>>> z.append(1)
>>> print x
[1, [1, 100, 3], 'some random string :)']
>>> print z
[1]
>>>
Now, to show our complete use-case here:
for l in file:
new_config.append(l)
config = list(new_config)
Laters, for every new config item, we do:
new_config.append(x)
or
new_config.insert(index, item)
By using those operations we won't change the "config" variable.
Another example:
>>> x = [1,2,3]
>>> z = list(x)
>>> z.insert(1,5)
>>> z
[1, 5, 2, 3]
>>> x
[1, 2, 3]
>>> z.append(10)
>>> z
[1, 5, 2, 3, 10]
>>> x
[1, 2, 3]
>>>
What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/587#issuecomment-121827701
LGTM, though did not test it due to lack of time and test infrastructure.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8616: Redundant VPC with both ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/587
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---