You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by bvbharatk <gi...@git.apache.org> on 2015/08/13 14:59:00 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8725 RVR functionality is brok...
GitHub user bvbharatk opened a pull request:
https://github.com/apache/cloudstack/pull/692
CLOUDSTACK-8725 RVR functionality is broken in case of isolated netwo…
CLOUDSTACK-8725 RVR functionality is broken in case of isolated networks, conntrackd fails to start.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bvbharatk/cloudstack CLOUDSTACK-8725
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/692.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 #692
----
commit fbebfd76ca774281e80c72e9141cd179a9d31bed
Author: Bharat Kumar <bh...@citrix.com>
Date: 2015-08-13T12:28:15Z
CLOUDSTACK-8725 RVR functionality is broken in case of isolated networks, conntrackd fails to start.
----
---
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-8725 RVR functionality is brok...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/692#issuecomment-134952038
@bvbharatk The rebase is needed due to the 'PEP8' style fix that has been done on CsRedundant.py Let me know if you need help :-)
---
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-8725 RVR functionality is brok...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/692#issuecomment-134876392
@DaanHoogland OK, thanks!
---
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-8725 RVR functionality is brok...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/692#issuecomment-134707371
@bvbharatk Had a quick look and it seems to solve the problem. Can you please rebase against latest master so we can have a clean merge? Thanks!
@DaanHoogland Any more questions on this one?
---
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-8725 RVR functionality is brok...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/692#discussion_r37056270
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsRedundant.py ---
@@ -96,7 +96,7 @@ def _redundant_on(self):
d = s.replace(".templ", "")
CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, s), "%s/%s" % (self.CS_ROUTER_DIR, d))
CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, "keepalived.conf.templ"), self.KEEPALIVED_CONF)
- CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, "conntrackd.conf.templ"), self.CONNTRACKD_CONF)
+ CsHelper.copy("%s/%s" % (self.CS_TEMPLATES_DIR, "conntrackd.conf.templ"), self.CONNTRACKD_CONF)
--- End diff --
then wouldn't the keepalived.conf always be needed as well? the use of copy_if_needed seemed very intentional.
---
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-8725 RVR functionality is brok...
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/692#discussion_r37269538
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsRedundant.py ---
@@ -96,7 +96,7 @@ def _redundant_on(self):
d = s.replace(".templ", "")
CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, s), "%s/%s" % (self.CS_ROUTER_DIR, d))
CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, "keepalived.conf.templ"), self.KEEPALIVED_CONF)
- CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, "conntrackd.conf.templ"), self.CONNTRACKD_CONF)
+ CsHelper.copy("%s/%s" % (self.CS_TEMPLATES_DIR, "conntrackd.conf.templ"), self.CONNTRACKD_CONF)
--- End diff --
Hi Daan,
The intent of copy_if_needed is to prevent overwriting the config file if it is preset already. In case of conntrackd, the config file is already present and so it dose not copy the default cloudstack conntrackd template and as a result conntrackd fails to start. For this(copy_if_needed) to work we should make sure that this file is not present when this code runs for the first time. There may be similar errors related to rvr. I was trying to see if i can address them as a part of larger fix.
---
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-8725 RVR functionality is brok...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/692
---
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-8725 RVR functionality is brok...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/692#discussion_r36970870
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsRedundant.py ---
@@ -96,7 +96,7 @@ def _redundant_on(self):
d = s.replace(".templ", "")
CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, s), "%s/%s" % (self.CS_ROUTER_DIR, d))
CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, "keepalived.conf.templ"), self.KEEPALIVED_CONF)
- CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, "conntrackd.conf.templ"), self.CONNTRACKD_CONF)
+ CsHelper.copy("%s/%s" % (self.CS_TEMPLATES_DIR, "conntrackd.conf.templ"), self.CONNTRACKD_CONF)
--- End diff --
would it make sense to make sure it is marked as needed, or is this always needed (even in non redundant routers)?
---
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-8725 RVR functionality is brok...
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/692#discussion_r37048693
--- Diff: systemvm/patches/debian/config/opt/cloud/bin/cs/CsRedundant.py ---
@@ -96,7 +96,7 @@ def _redundant_on(self):
d = s.replace(".templ", "")
CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, s), "%s/%s" % (self.CS_ROUTER_DIR, d))
CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, "keepalived.conf.templ"), self.KEEPALIVED_CONF)
- CsHelper.copy_if_needed("%s/%s" % (self.CS_TEMPLATES_DIR, "conntrackd.conf.templ"), self.CONNTRACKD_CONF)
+ CsHelper.copy("%s/%s" % (self.CS_TEMPLATES_DIR, "conntrackd.conf.templ"), self.CONNTRACKD_CONF)
--- End diff --
Hi Daan,
This will be needed only in case of redundant router. The current code tries to copy this file only if it is a rvr. It will not be copied for normal routers.
---
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-8725 RVR functionality is brok...
Posted by michaelandersen <gi...@git.apache.org>.
Github user michaelandersen commented on the pull request:
https://github.com/apache/cloudstack/pull/692#issuecomment-134932710
verified that conntrackd starts on redundant vpc routers.
LGTM in that respect.
Design wise i feel we need to refactor this, naming of copy_if_needed is ambiguous.
Also we're running a lot of boilerplate code which can be replaced by more widely used and maintained python packages.
---
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-8725 RVR functionality is brok...
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/692#issuecomment-134863538
@remibergsma no, answer is clear and though my question expresses a reservation on design level it should not stop this PR; => 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.
---