Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)
andrewgaul requested changes on this pull request.
Which existing integration test exercises this code path? If none exists,
please add one. Also post the results from some real providers.
> @@ -73,7 +73,7 @@ public void testZeroLengthPutHasContentLengthHeader()
> throws IOException, Interr
RecordedRequest request = server.takeRequest();
assertEquals(request.getRequestLine(), "PUT /bucket/object HTTP/1.1");
assertEquals(request.getHeaders(CONTENT_LENGTH), ImmutableList.of("0"));
- assertEquals(request.getHeaders(EXPECT),
ImmutableList.of("100-continue"));
+ assert request.getHeaders(EXPECT).isEmpty();
Call `assertThat(request.getHeaders(EXPECT).isEmpty())`.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#pullrequestreview-50510182
Re: [jclouds/jclouds-site] Remove unsupported sections (clojure, Ant, GAE) (#199)
For what it is worth, jclouds has a GAE driver although it may have bit-rotted: https://issues.apache.org/jira/browse/JCLOUDS-836 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199#issuecomment-315960455
Re: [jclouds/jclouds-site] Remove unsupported sections (clojure, Ant, GAE) (#199)
Nice catch - thanks, @neykov! @nacx Do you think it would make sense to add "check for references in the docs" as a TODO item for contributors and/or reviewers? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199#issuecomment-315926871
Re: [jclouds/jclouds-site] Document workaround to revert to shared credential store (#200)
demobox commented on this pull request.
> @@ -548,3 +548,20 @@ The jclouds API allows many `Statements` to be built
> entirely from high-level co
without having to resort to OS-specific scripts. This enables developers to
express what they mean without having
to deal with the gory details of various OS flavors.
To see the commands that will be executed, print the result of
`Statement.render(OsFamily.UNIX)`, for example.
+
+### Sharing the credential store between ComputeService instances
+
+By default there's a separate credential store per ComputeService instance. To
change that and share the
+same credential store between instances create your ComputeService as follows:
+
+{% highlight java %}
+public static final Map SHARED_CREDENTIAL_STORE = new
ConcurrentHashMap();
+...
+Module sharedCredStore = new CredentialStoreModule(SHARED_CREDENTIAL_STORE);
[minor] Just to clarify: is there any specific reason to pull the
`SHARED_CREDENTIAL_STORE` out as a constant, or could one e.g. also have the
module as a constant?
I'm asking to avoid users e.g. inlining the declaration if they feel it's not
necessary, and ending up with unexpected behaviour.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-site/pull/200#pullrequestreview-50486268
Re: [jclouds/jclouds-site] Document workaround to revert to shared credential store (#200)
Go to http://f65b776042714e5b0791-9883cc9304202abd55a5101f881b1528.r52.cf5.rackcdn.com/ to review your changes. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/200#issuecomment-315926669
Re: [jclouds/jclouds-site] Document workaround to revert to shared credential store (#200)
@demobox pushed 1 commit. d34f70d Minor suggested text edit to the "pre-2.1.0" note -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-site/pull/200/files/c87b4acdbd9c7d679ddd9165f8f20533c9ed49be..d34f70de2c03c9013d67916d31a6d703ae17292b
Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)
> I'll also go looking a bit through the archives... Here's a few: * https://groups.google.com/forum/#!topic/jclouds-dev/H6Q5mhpCq0c * https://github.com/rackspace/jclouds/commit/d897c5e44717e3d66062cd1275033e414f6a2de2 * https://issues.apache.org/jira/browse/JCLOUDS-181 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1120#issuecomment-315926003
Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)
I recall a lot of discussion and trial and error in the past trying to get this to work across providers and versions - from what I recall, there were API bugs to work around etc. etc. As such, unless we can verify that those conditions no longer exist, I'd be very hesitant to changing more than is absolutely necessary here to get whatever is currently broken working. @andrewgaul @nacx Do either of you two happen to recall some more details? I'll also go looking a bit through the archives... -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1120#issuecomment-315925669
Re: [jclouds/jclouds-labs] Generate Azure VM password on the fly (#402)
demobox commented on this pull request.
> + * 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.
+ */
+package org.jclouds.azurecompute.arm.util;
+
+import com.google.common.io.BaseEncoding;
+
+import java.util.Random;
+
+// Seems to be a common theme between providers, perhaps should be provided by
core (see other 'Passwords' classes)
+public class Passwords {
@neykov @nacx Are there any requirements around the value produced by this
class (length, allowed characters) etc.? If so, should we add a small test to
verify those?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/402#pullrequestreview-50485264
Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)
ChaithanyaGK commented on this pull request.
> @@ -357,6 +357,22 @@ public GeneratedHttpRequest apply(Invocation invocation)
> {
if (request.getPayload() != null) {
contentMetadataCodec.fromHeaders(request.getPayload().getContentMetadata(),
headers);
}
+
+ boolean isEmptyBody = false;
+ if ("POST".equals(requestMethod) || "PUT".equals(requestMethod)) {
@nacx Mostly, I came across Expect header being set for either POST or PUT
request. But, I couldn't find any mention of that Expect header is restricted
to some Http methods. I think we can remove this check altogether. WDYT ?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#discussion_r127745941
Re: [jclouds/jclouds-site] Remove unsupported sections (clojure, Ant, GAE) (#199)
[Merged](https://git1-us-west.apache.org/repos/asf?p=jclouds-site.git;a=commit;h=94b98475f77b0ed6b9d9e3db2e05209b19c523f4) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199#issuecomment-315778282
Re: [jclouds/jclouds-labs] JCLOUDS-1255 Server API migration. (#400)
Thanks @nacx for taking a look, appreciate that you took time out to review our pull request during the 2.0.2 release. I will review the comments you have raised on this pull request and update. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/400#issuecomment-315776765
Re: [jclouds/jclouds-site] Remove unsupported sections (clojure, Ant, GAE) (#199)
Go to http://71d71ba2b69c1873f826-64134700c51c773ca3459cdb14bd1042.r2.cf5.rackcdn.com/ to review your changes. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199#issuecomment-315773508
Re: [jclouds/jclouds-site] Remove unsupported sections (clojure, Ant, GAE) (#199)
Merged #199. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199#event-1166533045
Re: [jclouds/jclouds-site] Document workaround to revert to shared credential store (#200)
Go to http://f87fb730277b7ff7bcc2-6998e5196134be870bcb4bdbdaf08877.r71.cf5.rackcdn.com/ to review your changes. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/200#issuecomment-315774092
Re: [jclouds/jclouds-site] Document workaround to revert to shared credential store (#200)
nacx approved this pull request. Thansk @neykov! -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/200#pullrequestreview-50331119
Re: [jclouds/jclouds-site] Remove unsupported sections (clojure, Ant, GAE) (#199)
nacx approved this pull request. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199#pullrequestreview-50330756
Re: [jclouds/jclouds] JCLOUDS-1321: Use separate credential stores per context (#1119)
* Jira (to be included in release notes): https://issues.apache.org/jira/browse/JCLOUDS-1321 * Heads up to the mailing lists: * [user@j.a.o](https://lists.apache.org/thread.html/c65711e3ca3b3ffe068fc6562f0db98336772984924f24857ee0bdcd@%3Cuser.jclouds.apache.org%3E) * [dev@j.a.o](https://lists.apache.org/thread.html/b3edeca6b742d122908c570dd779af4bb1c93c6405e0bf5b52c17267@%3Cdev.jclouds.apache.org%3E) * Documenting a work around in the [compute guide](https://github.com/jclouds/jclouds-site/pull/200) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1119#issuecomment-315765265
[jira] [Created] (JCLOUDS-1321) Per-compute-service credential store
Svetoslav Neykov created JCLOUDS-1321: - Summary: Per-compute-service credential store Key: JCLOUDS-1321 URL: https://issues.apache.org/jira/browse/JCLOUDS-1321 Project: jclouds Issue Type: Bug Components: jclouds-core Affects Versions: 2.0.2 Reporter: Svetoslav Neykov With a shared credential store (the current default) the configuration of one compute service leaks in all others, causing the wrong credentials to be used when not overridden. The particular problem this fixes - [Azure sets default username/password for images|https://github.com/jclouds/jclouds-labs/blob/master/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/AzureComputeProviderMetadata.java#L98] which leaks to other providers (through [GetLoginForProviderFromPropertiesAndStoreCredentialsOrReturnNull|https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/config/GetLoginForProviderFromPropertiesAndStoreCredentialsOrReturnNull.java#L51]), causing the wrong username/password to be used. Even if the credentials are scoped to a specific provider in the map, it still will cause cross-talk between differently configured compute services. The cache should only apply to the current compute service. -- This message was sent by Atlassian JIRA (v6.4.14#64029)
Re: [jclouds/jclouds-site] Document workaround to revert to shared credential store (#200)
Go to http://7980e4076c61b89e82eb-ee8aa8198f8d15624472c3c54569b0e1.r61.cf5.rackcdn.com/ to review your changes. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/200#issuecomment-315760573
Re: [jclouds/jclouds-site] Remove unsupported sections (clojure, Ant, GAE) (#199)
Go to http://84e8de0da1a2915bcbe5-02f621e7bc2c992c40c304cd38334419.r49.cf5.rackcdn.com/ to review your changes. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199#issuecomment-315760492
[jclouds/jclouds-site] Document workaround to revert to shared credential store (#200)
Depends on https://github.com/jclouds/jclouds/pull/1119 being released. You can view, comment on, or merge this pull request online at: https://github.com/jclouds/jclouds-site/pull/200 -- Commit Summary -- * Workaround to revert to shared credential store -- File Changes -- M start/compute.md (15) -- Patch Links -- https://github.com/jclouds/jclouds-site/pull/200.patch https://github.com/jclouds/jclouds-site/pull/200.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/200
Re: [jclouds/jclouds-site] Remove unsupported sections (clojure, Ant, GAE) (#199)
Done @nacx -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199#issuecomment-315760120
Re: [jclouds/jclouds-site] Remove unsupported sections (clojure, Ant, GAE) (#199)
Go to http://ee3f1ff390343e30f39b-d136f4782a6445a9196f897e7072ec57.r95.cf5.rackcdn.com/ to review your changes. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199#issuecomment-315760031
Re: [jclouds/jclouds-site] Remove unsupported sections (clojure & ant) (#199)
Can you remove the "Usage in Google AppEngine" too? -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199#issuecomment-315756600
Re: [jclouds/jclouds-site] Remove unsupported sections (clojure & ant) (#199)
Go to http://52e5085fc260ceb67e22-fa0c3b2f9d9bfd5ea649b5c10dacaba5.r41.cf5.rackcdn.com/ to review your changes. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199#issuecomment-315755686
[jclouds/jclouds-site] Remove unsupported sections (clojure & ant) (#199)
* clojure support removed from core * reference to non-existent `ApacheAntComputeGuide` You can view, comment on, or merge this pull request online at: https://github.com/jclouds/jclouds-site/pull/199 -- Commit Summary -- * Remove Clojure references, support removed from core * Remove reference to the non-existent ApacheAntComputeGuide page -- File Changes -- M start/compute.md (72) -- Patch Links -- https://github.com/jclouds/jclouds-site/pull/199.patch https://github.com/jclouds/jclouds-site/pull/199.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-site/pull/199
Re: [jclouds/jclouds-labs] Generate Azure VM password on the fly (#402)
Closed #402. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/402#event-1166369934
Re: [jclouds/jclouds-labs] Generate Azure VM password on the fly (#402)
Merged to [master](https://git1-us-west.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=3641cdb44c192be381f82d8886f966c248474f4d) and [2.0.x](https://git1-us-west.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=f27b5f944887bdefba78d7bbd339ea68db76725c) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds-labs/pull/402#issuecomment-315747187
Re: [jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)
nacx requested changes on this pull request.
Thanks @ChaithanyaGK!
@andrewgaul Mind having a look?
> @@ -357,6 +357,22 @@ public GeneratedHttpRequest apply(Invocation invocation)
> {
if (request.getPayload() != null) {
contentMetadataCodec.fromHeaders(request.getPayload().getContentMetadata(),
headers);
}
+
+ boolean isEmptyBody = false;
+ if ("POST".equals(requestMethod) || "PUT".equals(requestMethod)) {
Should we consider "PATCH" operations too?
> @@ -357,6 +357,22 @@ public GeneratedHttpRequest apply(Invocation invocation)
> {
if (request.getPayload() != null) {
contentMetadataCodec.fromHeaders(request.getPayload().getContentMetadata(),
headers);
}
+
+ boolean isEmptyBody = false;
+ if ("POST".equals(requestMethod) || "PUT".equals(requestMethod)) {
+ if (request.getPayload() != null) {
+Long length =
request.getPayload().getContentMetadata().getContentLength();
+if (length != null && length == 0) {
+ isEmptyBody = true;
+}
+ } else {
+isEmptyBody = true;
+ }
+ }
+ if (isEmptyBody) {
+ request = request.toBuilder().removeHeader("Expect").build();
+ }
Let's refactor this to a method that gets a request and returns the modified
one so it can be properly unit-tested.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1120#pullrequestreview-50308589
[jclouds/jclouds] Remove Expect header for PUT and POST reqs with content-length=0 (#1120)
Without request body, there's no point in asking for 100-continue. With [JavaUrlHttpCommandExecutor](https://github.com/jclouds/jclouds/blob/f3c3f3b30620ad62dc502cf79bf121ec1773396e/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java), HttpUrlConnection is throwing an [IOException](https://github.com/jclouds/jclouds/blob/f3c3f3b30620ad62dc502cf79bf121ec1773396e/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java#L96) if we expect 100-continue for an PUT/POST request with empty body although it is returning 201 response code, it is resulting in another network call when we do [getHeaderFields](https://github.com/jclouds/jclouds/blob/f3c3f3b30620ad62dc502cf79bf121ec1773396e/core/src/main/java/org/jclouds/http/internal/JavaUrlHttpCommandExecutorService.java#L113) on the connection. This issue can be easily replicated by invoking putBlob operation with zero-length blob. ``` ByteSource payload = ByteSource.empty(); Blob blob = blobStore.blobBuilder(blobName) .payload(payload) .contentLength(payload.size()) .build(); blobStore.putBlob(containerName, blob); ``` You can view, comment on, or merge this pull request online at: https://github.com/jclouds/jclouds/pull/1120 -- Commit Summary -- * Remove Expect header for PUT and POST reqs with content-length=0 -- File Changes -- M apis/s3/src/test/java/org/jclouds/s3/S3ClientMockTest.java (2) M core/src/main/java/org/jclouds/rest/internal/RestAnnotationProcessor.java (16) -- Patch Links -- https://github.com/jclouds/jclouds/pull/1120.patch https://github.com/jclouds/jclouds/pull/1120.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1120
[jira] [Updated] (JCLOUDS-1318) Security group created for the server is not deleted
[
https://issues.apache.org/jira/browse/JCLOUDS-1318?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Ignasi Barrera updated JCLOUDS-1318:
Labels: openstack-nova (was: )
> Security group created for the server is not deleted
>
>
> Key: JCLOUDS-1318
> URL: https://issues.apache.org/jira/browse/JCLOUDS-1318
> Project: jclouds
> Issue Type: Bug
> Components: jclouds-compute
>Affects Versions: 2.0.0, 2.0.1, 2.0.2
>Reporter: Andrea Turli
>Assignee: Andrea Turli
> Labels: openstack-nova
> Fix For: 2.1.0
>
>
> In some Openstack provider, during destroyNode the server gets deleted while
> the securityGroup created for it can't be deleted because of
> {{org.jclouds.http.HttpResponseException: command: DELETE
> https://compute.gra1.cloud.ovh.net/v2/68d8aaf362544fa78b040d7783fdad63/os-security-groups/f6957437-3035-463a-833f-253d21bc6c43
> HTTP/1.1 failed with response: HTTP/1.1 400 Bad Request; content:
> [{"badRequest": {"message": "Security Group
> f6957437-3035-463a-833f-253d21bc6c43 in use.", "code": 400}}]}}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Resolved] (JCLOUDS-1318) Security group created for the server is not deleted
[
https://issues.apache.org/jira/browse/JCLOUDS-1318?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Andrea Turli resolved JCLOUDS-1318.
---
Resolution: Fixed
> Security group created for the server is not deleted
>
>
> Key: JCLOUDS-1318
> URL: https://issues.apache.org/jira/browse/JCLOUDS-1318
> Project: jclouds
> Issue Type: Bug
> Components: jclouds-compute
>Affects Versions: 2.0.0, 2.0.1, 2.0.2
>Reporter: Andrea Turli
>Assignee: Andrea Turli
> Fix For: 2.1.0
>
>
> In some Openstack provider, during destroyNode the server gets deleted while
> the securityGroup created for it can't be deleted because of
> {{org.jclouds.http.HttpResponseException: command: DELETE
> https://compute.gra1.cloud.ovh.net/v2/68d8aaf362544fa78b040d7783fdad63/os-security-groups/f6957437-3035-463a-833f-253d21bc6c43
> HTTP/1.1 failed with response: HTTP/1.1 400 Bad Request; content:
> [{"badRequest": {"message": "Security Group
> f6957437-3035-463a-833f-253d21bc6c43 in use.", "code": 400}}]}}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[jira] [Updated] (JCLOUDS-1318) Security group created for the server is not deleted
[
https://issues.apache.org/jira/browse/JCLOUDS-1318?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Andrea Turli updated JCLOUDS-1318:
--
Flags: Important
Affects Version/s: 2.0.0
2.0.2
Fix Version/s: (was: 2.0.3)
> Security group created for the server is not deleted
>
>
> Key: JCLOUDS-1318
> URL: https://issues.apache.org/jira/browse/JCLOUDS-1318
> Project: jclouds
> Issue Type: Bug
> Components: jclouds-compute
>Affects Versions: 2.0.0, 2.0.1, 2.0.2
>Reporter: Andrea Turli
>Assignee: Andrea Turli
> Fix For: 2.1.0
>
>
> In some Openstack provider, during destroyNode the server gets deleted while
> the securityGroup created for it can't be deleted because of
> {{org.jclouds.http.HttpResponseException: command: DELETE
> https://compute.gra1.cloud.ovh.net/v2/68d8aaf362544fa78b040d7783fdad63/os-security-groups/f6957437-3035-463a-833f-253d21bc6c43
> HTTP/1.1 failed with response: HTTP/1.1 400 Bad Request; content:
> [{"badRequest": {"message": "Security Group
> f6957437-3035-463a-833f-253d21bc6c43 in use.", "code": 400}}]}}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
merged at [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/aa11765b) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315709952
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
Closed #1117. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#event-1166125075
[jira] [Commented] (JCLOUDS-1318) Security group created for the server is not deleted
[
https://issues.apache.org/jira/browse/JCLOUDS-1318?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16089559#comment-16089559
]
ASF subversion and git services commented on JCLOUDS-1318:
--
Commit aa11765bee8e5e7f2d062ba8123c0dced822f071 in jclouds's branch
refs/heads/master from [~andreaturli]
[ https://git-wip-us.apache.org/repos/asf?p=jclouds.git;h=aa11765 ]
[JCLOUDS-1318] fix based on nodeTerminatePredicate
- wait for serer deletion, before deleting the security group
- rename cleanupServer to cleanupResources
- remove keyPairCache
- better usage of tags to remove securityGroups created by jclouds
- remove keyPair after the creation of a group
- remove test for create unique keypair
- openstack nova re-adding support for existing keypair
- fix securityGroupApi check
- fix other unit tests
- remove ServerPredicates as it is now duplicated
- remove TemplateOptions.securityGroupNames as deprecated
- address commits for
ApplyNovaTemplateOptionsCreatesNodesWithGroupEncodedIntoNameAndAddToSet
- fix testCreateNodeWhileUserSpecifiesKeyPairAndUserSpecifiedGroups
> Security group created for the server is not deleted
>
>
> Key: JCLOUDS-1318
> URL: https://issues.apache.org/jira/browse/JCLOUDS-1318
> Project: jclouds
> Issue Type: Bug
> Components: jclouds-compute
>Affects Versions: 2.0.1
>Reporter: Andrea Turli
>Assignee: Andrea Turli
> Fix For: 2.1.0, 2.0.3
>
>
> In some Openstack provider, during destroyNode the server gets deleted while
> the securityGroup created for it can't be deleted because of
> {{org.jclouds.http.HttpResponseException: command: DELETE
> https://compute.gra1.cloud.ovh.net/v2/68d8aaf362544fa78b040d7783fdad63/os-security-groups/f6957437-3035-463a-833f-253d21bc6c43
> HTTP/1.1 failed with response: HTTP/1.1 400 Bad Request; content:
> [{"badRequest": {"message": "Security Group
> f6957437-3035-463a-833f-253d21bc6c43 in use.", "code": 400}}]}}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
merging to master only -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315707761
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
rebuild please -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315703315
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
rebuild please -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315703226
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
thanks @nacx let's wait for the builder as extra-check. Meanwhile I'm rebasing and squashing -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#issuecomment-315695894
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
nacx approved this pull request. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/jclouds/jclouds/pull/1117#pullrequestreview-50258245
Re: [jclouds/jclouds-labs] JCLOUDS-1255 Server API migration. (#400)
nacx requested changes on this pull request.
Thanks, @trevorflanagan! Apologies for the delay in the review. We've been
quite busy with the 2.0.2 release.
>
- public abstract int sizeGb();
+ @Nullable
+ public abstract Integer sizeGb();
Can this field have decimals? (Just asking; I'm not familiar with the API).
> + @Nullable @PayloadParam("disk") List disks, @Nullable
> CreateServerOptions options);
+
+ @Named("server:delete")
+ @POST
+ @Path("/deleteServer")
+ @Produces(MediaType.APPLICATION_JSON)
+ @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
+ @MapBinder(BindToJsonPayload.class)
+ void deleteServer(@PayloadParam("id") String id);
+
+ @Named("server:powerOff")
+ @POST
+ @Path("/powerOffServer")
+ @Produces(MediaType.APPLICATION_JSON)
+ @MapBinder(BindToJsonPayload.class)
+ @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
In state change operations I think it would be better to fail if the server
does not exist. Otherwise, users won't be able to differentiate a failed
execution than a succeeded one. The use case here is different than in get
operations or delete (where the purpose is to have the server gone). Let's
remove the 404 fallbacks from all state change operations.
> + * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.dimensiondata.cloudcontrol.predicates;
+
+import com.google.common.base.Predicate;
+import org.jclouds.dimensiondata.cloudcontrol.domain.Server;
+import org.jclouds.dimensiondata.cloudcontrol.features.ServerApi;
+import org.jclouds.logging.Logger;
+
+import javax.annotation.Resource;
+import java.text.MessageFormat;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class ServerStatus implements Predicate {
Does this duplicate the `ServerState` class?
> + protected Logger logger = Logger.NULL;
+ private final ServerApi api;
+
+ public VMToolsRunningStatus(ServerApi api) {
+ this.api = api;
+ }
+
+ @Override
+ public boolean apply(String serverId) {
+ checkNotNull(serverId, "serverId");
+ logger.trace("looking for state on Server %s", serverId);
+ final Server server = api.getServer(serverId);
+ if (server == null) {
+ throw new IllegalStateException(MessageFormat.format("Server {0} is
not found", serverId));
+ }
+ return
server.guest().vmTools().runningStatus().equals(VmTools.RunningStatus.RUNNING);
The `vmTools` field is nullable. Protect this against a NPE.
> throw new IllegalStateException(message);
}
}
+ public static void waitForVmToolsRunning(ServerApi api, String serverId,
long timeoutMillis, String message) {
+ boolean vmwareToolsRunning = retry(new VMToolsRunningStatus(api),
timeoutMillis).apply(serverId);
+ if (!vmwareToolsRunning) {
+ throw new IllegalStateException(message);
+ }
+ }
Instead of having a static class with these accessors, consider configuring all
these predicates as injectable predicates. this makes it easier to get them
injected in the jclouds classes (such as the compute service when implemented),
and also allows users to configure the timeout values, etc, via properties.
You can have a look at [the digitalocean2
provider](https://github.com/jclouds/jclouds/blob/master/providers/digitalocean2/src/main/java/org/jclouds/digitalocean2/compute/config/DigitalOcean2ComputeServiceContextModule.java#L100-L216)
for an example.
> + @Test
+ public void testDeployAndStartServer() {
+ Boolean started = Boolean.TRUE;
+ NetworkInfo networkInfo = NetworkInfo
+.create(NETWORK_DOMAIN_ID, NIC.builder().vlanId(VLAN_ID).build(),
Lists.newArrayList());
+ List disks =
ImmutableList.of(Disk.builder().scsiId(0).speed("STANDARD").build());
+ serverId = api().deployServer(deployedServerName, IMAGE_ID, started,
networkInfo, "P$$ssWwrrdGoDd!", disks, null);
+ assertNotNull(serverId);
+ waitForServerStatus(api(), serverId, true, true, 30 * 60 * 1000,
"Error");
+ waitForServerState(api(), serverId, State.NORMAL, 30 * 60 * 1000,
"Error");
+ }
+
+ @Test(dependsOnMethods = "testDeployAndStartServer")
+ public void testReconfigureServer() {
+ api().reconfigureServer(serverId, 4, CpuSpeed.HIGHPERFORMANCE.name(), 1);
+ waitForServerState(api(), serverId, State.PENDING_CHANGE, 30 * 60 *
1000, "Error");
Is it possible that the reconfigure operation is fast enough that we don't
actually see this state? Does it really make sense to wait for intermediate
states?
> + @Test(dependsOnMethods = "testStartServer")
+ public void testShutdownServer() {
+ api().shutdownServer(serverId);
+ waitForServerStatus(api(), serverId, false, true, 30 * 60 * 1000,
"Error");
+ }
+
+ @Test(dependsOnMethods = "testShutdownServer")
+ public void testCloneServer() {
+ CloneServerOptions options =
Re: [jclouds/jclouds-labs] Jclouds 46 network and image tests (#401)
nacx requested changes on this pull request.
Thanks, @btrishkin and apologies for the delay in the review! We've been quite
busy with the 2.0.2 release.
Just some minor comments to address.
> DEFAULT_PRIVATE_IPV4_PREFIX_SIZE);
assertNotNull(vlanId);
waitForVlanStatus(api(), vlanId, State.NORMAL, 30 * 60 * 1000, "Error -
unable to deploy vlan");
}
@Test
+ public void testAddPublicIPv4Block() {
+ publicIpv4BlockId = api().addPublicIpBlock(PREPARED_NETWORK_DOMAIN_ID);
+ assertNotNull(publicIpv4BlockId);
+ }
+
+ @Test(dependsOnMethods = "testAddPublicIPv4Block")
+ public void testListPublicIPv4AddressBlocks() {
+ PagedIterable ipBlockList =
api().listPublicIPv4AddressBlocks(PREPARED_NETWORK_DOMAIN_ID);
+ assertTrue(!ipBlockList.isEmpty());
+ assertEquals(ipBlockList.last().get().first().get().size(), 2);
+ assertEquals(ipBlockList.last().get().first().get().networkDomainId(),
PREPARED_NETWORK_DOMAIN_ID);
+ }
+
+ @Test(dependsOnMethods = "testListPublicIPv4AddressBlocks")
This could depend on the "add" test to maximize the chances of this test being
executed.
> +
+ @Test(dependsOnMethods = "testGetPublicIPv4AddressBlocks")
+ public void testCreateNatRule() {
+ natRuleId = api()
+.createNatRule(PREPARED_NETWORK_DOMAIN_ID,
PREPARED_PRIVATE_IPV4_ADDRESS, publicIpBlock.baseIp());
+ assertNotNull(natRuleId);
+ }
+
+ @Test(dependsOnMethods = "testCreateNatRule")
+ public void testListNatRules() {
+ PagedIterable natRulesList =
api().listNatRules(PREPARED_NETWORK_DOMAIN_ID);
+ assertTrue(!natRulesList.isEmpty());
+ assertEquals(natRulesList.last().get().first().get().networkDomainId(),
PREPARED_NETWORK_DOMAIN_ID);
+ }
+
+ @Test(dependsOnMethods = "testListNatRules")
Also depend on the "add" test.
> +
+ @Test(dependsOnMethods = "testCreateNatRule")
+ public void testListNatRules() {
+ PagedIterable natRulesList =
api().listNatRules(PREPARED_NETWORK_DOMAIN_ID);
+ assertTrue(!natRulesList.isEmpty());
+ assertEquals(natRulesList.last().get().first().get().networkDomainId(),
PREPARED_NETWORK_DOMAIN_ID);
+ }
+
+ @Test(dependsOnMethods = "testListNatRules")
+ public void testGetNatRule() {
+ NatRule natRule = api().getNatRule(natRuleId);
+ assertNotNull(natRule);
+ assertEquals(natRule.networkDomainId(), PREPARED_NETWORK_DOMAIN_ID);
+ }
+
+ @Test(dependsOnMethods = "testGetNatRule")
Annotate cleanup methods with `alwaysRun = true` to make sure they're executed
even the tests they depend on have failed. This way we avoid leaking resources
used in tests.
> @@ -123,4 +213,13 @@ private NetworkApi api() {
return api.getNetworkApi();
}
+ private FirewallRule findById(List collection, String id) {
+ FirewallRule result = null;
+ for (FirewallRule rule : collection) {
+ if (rule.id().equals(id)) {
+result = rule;
Just `return rule`.
> @@ -217,4 +225,15 @@ protected void assertBodyContains(RecordedRequest
> recordedRequest, String expect
throw Throwables.propagate(e);
}
}
+
+ private List parsePath(String fullPath) {
+ return URLEncodedUtils.parse(fullPath, Charset.defaultCharset(), '?',
'&');
+ }
+
+ private void assertParameters(List parsedPath,
List parsedRequest) {
+ assertEquals(parsedPath.size(), parsedRequest.size());
+ for (NameValuePair parameter : parsedPath) {
+ assertTrue(parsedRequest.contains(parameter));
+ }
+ }
I don't really get the value in comparing the requests like this. Can't just we
compare the strings and avoid the need of adding an additional dependency just
for this?
Also, this check does not guarantee paths are the same; it does not check the
order of the value pairs. Consider just comparing the requests path strings, as
we do in all other providers.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/401#pullrequestreview-50242150
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
andreaturli commented on this pull request.
>
- boolean keyPairExtensionPresent =
novaApi.getKeyPairApi(region).isPresent();
+ List inboundPorts =
Ints.asList(templateOptions.getInboundPorts());
+ if (!templateOptions.getGroups().isEmpty() && !inboundPorts.isEmpty()) {
Ops, yes, thanks
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#discussion_r127638294
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
andreaturli commented on this pull request.
> }
+ } else if (templateOptions.getKeyPairName() != null &&
templateOptions.getLoginPrivateKey() != null) {
I was a bit unsure as well, I'll remove the extra check
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#discussion_r127638157
Re: [jclouds/jclouds] [JCLOUDS-1318] fix based on nodeTerminatePredicate (#1117)
nacx requested changes on this pull request.
I've just looked at the last changes. I'll run a local build too and see what
happens with that slow test.
> }
+ } else if (templateOptions.getKeyPairName() != null &&
templateOptions.getLoginPrivateKey() != null) {
Why checking also the private key here? If users configure a key pair we don't
care if they also configure the private key. Creating a node and not accessing
it is a valid use case. Perhaps users access it in another connection (where
the private key will be configured). I'd remove the private key check from here
and always set the keypair when its name is set.
>
- boolean keyPairExtensionPresent =
novaApi.getKeyPairApi(region).isPresent();
+ List inboundPorts =
Ints.asList(templateOptions.getInboundPorts());
+ if (!templateOptions.getGroups().isEmpty() && !inboundPorts.isEmpty()) {
This should be `||`.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1117#pullrequestreview-50241023
