You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by miguelaferreira <gi...@git.apache.org> on 2015/08/24 16:28:57 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-8758: Handle redirects in comm...
GitHub user miguelaferreira opened a pull request:
https://github.com/apache/cloudstack/pull/737
CLOUDSTACK-8758: Handle redirects in communication with NXS controller (a.k.a. Nicira NVP)
When an NSX controller node is part of a cluster it will redirect API calls to the master controller. Because the ACS management server does not follow such redirects, if there is a change of master within a NSX controller, the NSX device (a.k.a. NiciraNvp) needs to be reconfigured (via the management server DB).
The goal of this PR is to enable ACS management server to follow HTTP redirects sent by NSX controllers. However, other changes were made to the cloud-utils module that provides the REST client that the NSX plugin uses.
Cosmetic changes:
* Upgrade maven module structure for cloud-utils and cloud-plugin-netowkr-nvp to comply with maven default
* Several refactorings on both modules to consistently format the code, remove unused code, declare final when possible, remove auto generated comments, etc
Other changes:
* Upgrade HTTP library used in REST client to latest version of Apache HTTP Components
* Implement NSX specific REST client that support following HTTP redirects
* Simplify NSX api implementation
* Previously existing unit tests for both the REST client and NSX api were either maintained in the same test classes, moved to new test classes (because code under tests also moved), or removed (because code under tests was also removed)
* New Marvin tests for NSX controllers
Testing:
* Ran all unit tests present in the project
* Ran Java Integration tests for NSX api targeting both a master and a slave controller
* Ran new Marvin test for NSX controller
* Manual inspection of logs to confirm redirection is taking place
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/miguelaferreira/cloudstack feature/mferreira/ncx-follow-redirects-gardened-rebase
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/737.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 #737
----
commit 31679f7dbcb861277b03849f5ffa7a805192f3ec
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-05T14:03:28Z
Create Nicira NVP devices and enable plugin at deploy
commit fcd39e8586ae367e946b327409c0390ac23b0cb5
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-21T15:39:02Z
Add Marvin test for Nicira NVP plugin
commit 357a223cc210a27696eada3abfa37df33b574739
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-23T14:11:06Z
Fix unit-test library dependencies
- XML formatting
- Fix license header
- Reorder hamcrest, junit, mockito and powermock dependencies
* Since several libraries include a subset of hamcrest classes,
hamcrest should be declared first in the pom, inorder for its
classes to be loaded first by the JVM
- Depdend on hamcrest-all and exclude hamcrest classes from other
libraries
commit a92c419231f1383f66929e59a8c9e2de2e3e70f7
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-23T14:03:11Z
Refactor cloud-utils project into Maven default structure
commit ebe27281651ac7a0e8959c48aa41948a25006e0d
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-22T18:48:27Z
Refactor cloud-plugin-network-nvp project into Maven default structure
commit 9018ab4be818c418779d44bcde0a2b9b8e1ea366
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-22T18:53:12Z
Code clean up in cloud-utils project
- Code formatting
- Remove unused methods
commit fafc33c7bf2ca65eafc9a0ee78ee8ae6781dd32e
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-21T15:44:57Z
Add basic RestClient implentation based on HTTP Components 4.5
- Upgrade version of HTTP Components to 4.5
- Add helper class to create Http clients
- Add helper class to build http requests
- Add enum with the different Http Methods
- Add constants class for HTTP related values
commit 87e5852edb22910b57d756a656f361638810f0ae
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-22T18:53:28Z
Code clean up in cloud-plugin-network-nvp project
- Code formatting
- Declare final where possible
- Remove unused methods
- Remove throws declarations where not needed
- Remove generated comments (e.g. "TODO Auto-generated method stub")
commit 151ed6e032ba5a7a0bb0e8b89fbe48fea71664ac
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-23T13:11:29Z
Delegate HTTP protocol activity in RESTServiceConnector to RestClient
- All HTTP protocol activities are now handled by RestClient
- This service is now only responsible for creating requests, and
dispatching them to the client
- Provides a Simple API for creating, updating, retrieving and deleting
objects
commit 4af32bcf13ad3f30a4f30a24a1c86bb0eb4031a0
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-22T14:12:12Z
Refactor NSX api implementation (NiciraNvpApi)
- Make internal method private
- Remove unused methods
- Refactor type deserialization adapter classes out
commit 06450d95d6410575f25d56a3ff2fb1ad65c00bf0
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-22T15:05:30Z
Add NSX specific RestClient implementation
- Add -noverify JVM arg to surefire plugin, to allow Powermockito to
de-encapsulate private methods
- Add dependency on cloud-utils test-jar to use custom HttpRequest
matchers
commit f6708866f2e6af6d06cf658c04574aac659011d5
Author: Miguel Ferreira <mi...@me.com>
Date: 2015-08-22T15:30:58Z
Use NSX specific RestClient in API implementation (NiciraNvpApi)
- Simplify public API to return Lists instead of NiciraNvpLists
----
---
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-8758: Handle redirects in comm...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/737#discussion_r37976062
--- Diff: pom.xml ---
@@ -1,12 +1,23 @@
-<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
- license agreements. See the NOTICE file distributed with this work for additional
- information regarding copyright ownership. The ASF licenses this file to you under
- the Apache License, Version 2.0 (the "License"); you may not use this file except
- in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
- Unless required by applicable law or agreed to in writing, software distributed under
- the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
- OF ANY KIND, either express or implied. See the License for the specific language
- governing permissions and limitations under the License. -->
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
--- End diff --
@pdion891 good fix. I would very much more rather relying on the xml tag than a line number.
Before the change I got the mycila license plugin complaining during `mvn clean install`, so I changed the header to make it happy. Then the rats build was also complaining, and I made more changes to make that one happy.
I wonder how was it that before this change, neither the mycila plugin or the rats build complained?
---
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-8758: Handle redirects in comm...
Posted by ustcweizhou <gi...@git.apache.org>.
Github user ustcweizhou commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/737#discussion_r37968584
--- Diff: pom.xml ---
@@ -1,12 +1,23 @@
-<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
- license agreements. See the NOTICE file distributed with this work for additional
- information regarding copyright ownership. The ASF licenses this file to you under
- the Apache License, Version 2.0 (the "License"); you may not use this file except
- in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
- Unless required by applicable law or agreed to in writing, software distributed under
- the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
- OF ANY KIND, either express or implied. See the License for the specific language
- governing permissions and limitations under the License. -->
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
--- End diff --
@pdion891 @miguelaferreira This change broke the debian packaging , because debian/rules read the line 22 of pom.xml to get the version. now it should be line 33.
---
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-8758: Handle redirects in comm...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/737#issuecomment-134579465
Force pushed again to add the missing license headers.
---
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-8758: Handle redirects in comm...
Posted by snuf <gi...@git.apache.org>.
Github user snuf commented on the pull request:
https://github.com/apache/cloudstack/pull/737#issuecomment-134558902
Wow, great code and tests!. Love what you did with the Restclient, will be using that for sure soon :).
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-8758: Handle redirects in comm...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/737#issuecomment-134576306
Force pushed to fix typos in license header of file `utils/src/test/java/com/cloud/utils/rest/RESTServiceConnectorTest.java`
---
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-8758: Handle redirects in comm...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/737#issuecomment-134695303
LGTM - Deployed a full stack Nicira environment: 3 controllers, a manager, a service node and 2 KVM hypervisors, all inside my development VM:
```
# virsh list
Id Name State
----------------------------------------------------
42 nsxmgr1 running
43 nsxcon1 running
44 nsxsvc1 running
48 kvm1 running
49 nsxcon2 running
50 nsxcon3 running
52 kvm2 running
54 cs1 running
```
Then I ran the Marvin tests:
```
# nosetests --with-marvin --marvin-config=/data/shared/marvin/mct-zone1-kvm1-NVP.cfg -s -a tags=advanced,required_hardware=true test/integration/smoke/test_nicira_controller.py
test_01_nicira_controller (integration.smoke.test_nicira_controller.TestNiciraContoller) ... === TestName: test_01_nicira_controller | Status : SUCCESS ===
ok
Nicira clusters will redirect clients (in this case ACS) to the master node. ... === TestName: test_02_nicira_controller_redirect | Status : SUCCESS ===
ok
----------------------------------------------------------------------
Ran 2 tests in 766.910s
OK
```
BTW: The Marvin data center config file is available here: https://github.com/schubergphilis/MCT-shared/blob/master/marvin/mct-zone1-kvm1-NVP.cfg
---
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-8758: Handle redirects in comm...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/737#issuecomment-134699346
All builds are passing and we have 2xLGTM. Will merge it. Thanks @miguelaferreira !
---
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-8758: Handle redirects in comm...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/737#issuecomment-134595872
Force pushed again for two more license headers
---
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-8758: Handle redirects in comm...
Posted by pdion891 <gi...@git.apache.org>.
Github user pdion891 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/737#discussion_r37973943
--- Diff: pom.xml ---
@@ -1,12 +1,23 @@
-<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
- license agreements. See the NOTICE file distributed with this work for additional
- information regarding copyright ownership. The ASF licenses this file to you under
- the Apache License, Version 2.0 (the "License"); you may not use this file except
- in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
- Unless required by applicable law or agreed to in writing, software distributed under
- the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
- OF ANY KIND, either express or implied. See the License for the specific language
- governing permissions and limitations under the License. -->
+<!--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+-->
--- End diff --
Quick hotfix is in here:
https://github.com/apache/cloudstack/pull/746
On Wed, Aug 26, 2015 at 6:48 AM, ustcweizhou <no...@github.com>
wrote:
> In pom.xml
> <https://github.com/apache/cloudstack/pull/737#discussion_r37968584>:
>
> > + distributed with this work for additional information
> > + regarding copyright ownership. The ASF licenses this file
> > + to you under the Apache License, Version 2.0 (the
> > + "License"); you may not use this file except in compliance
> > + with the License. You may obtain a copy of the License at
> > +
> > + http://www.apache.org/licenses/LICENSE-2.0
> > +
> > + Unless required by applicable law or agreed to in writing,
> > + software distributed under the License is distributed on an
> > + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> > + KIND, either express or implied. See the License for the
> > + specific language governing permissions and limitations
> > + under the License.
> > +
> > +-->
>
> @pdion891 <https://github.com/pdion891> @miguelaferreira
> <https://github.com/miguelaferreira> This change broke the debian
> packaging , because debian/rules read the line 22 of pom.xml to get the
> version. now it should be line 33.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/cloudstack/pull/737/files#r37968584>.
>
---
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-8758: Handle redirects in comm...
Posted by miguelaferreira <gi...@git.apache.org>.
Github user miguelaferreira commented on the pull request:
https://github.com/apache/cloudstack/pull/737#issuecomment-134632894
Force pushed again to fix NiciraNvp module structure refactoring that left out the resources configuration in pom.xml
---
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-8758: Handle redirects in comm...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/737
---
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.
---