You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Trevor Flanagan <no...@github.com> on 2017/11/29 18:13:40 UTC
[jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/422
-- Commit Summary --
* Implementing BaseImageToHardware and CleanupServer functions.
-- File Changes --
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/compute/functions/BaseImageToHardware.java (65)
A dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/compute/functions/CleanupServer.java (153)
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/FirewallRule.java (6)
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/NatRule.java (6)
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/PublicIpBlock.java (6)
M dimensiondata/src/main/java/org/jclouds/dimensiondata/cloudcontrol/domain/State.java (4)
A dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/compute/functions/BaseImageToHardwareTest.java (113)
A dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/compute/functions/CleanupServerTest.java (239)
M dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/parse/FirewallRulesParseTest.java (3)
M dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/parse/NatRulesParseTest.java (5)
M dimensiondata/src/test/java/org/jclouds/dimensiondata/cloudcontrol/parse/PublicIpBlocksParseTest.java (3)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/422.patch
https://github.com/jclouds/jclouds-labs/pull/422.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-labs/pull/422
Re: [jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
Thanks, @trevorflanagan! Just some minor comments!
> +import static java.lang.String.format;
+import static org.jclouds.dimensiondata.cloudcontrol.utils.DimensionDataCloudControlResponseUtils.generateFirewallRuleName;
+import static org.jclouds.dimensiondata.cloudcontrol.utils.DimensionDataCloudControlResponseUtils.waitForServerState;
+
+@Singleton
+public class CleanupServer implements Function<String, Boolean> {
+
+ @Resource
+ @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+ protected Logger logger = Logger.NULL;
+
+ private final DimensionDataCloudControlApi api;
+ private final Timeouts timeouts;
+
+ @Inject
+ public CleanupServer(final DimensionDataCloudControlApi api, final Timeouts timeouts) {
Remove the public modifier
> + String networkDomainId = server.networkInfo().networkDomainId();
+ final String internalIp = server.networkInfo().primaryNic().privateIpv4();
+
+ // delete nat rules associated to the server, if any
+ final NetworkApi networkApi = api.getNetworkApi();
+ List<NatRule> natRulesToBeDeleted = networkApi.listNatRules(networkDomainId).concat()
+ .filter(new Predicate<NatRule>() {
+ @Override
+ public boolean apply(NatRule natRule) {
+ return natRule.internalIp().equals(internalIp);
+ }
+ }).toList();
+
+ for (final NatRule natRule : natRulesToBeDeleted) {
+ if (natRule.state().isNormal()) {
+ networkApi.deleteNatRule(natRule.id());
Surround with a try/catch and log a warning, for a best-effort cleanup? Otherwise, it will stop deleting stuff here if this call fails. (Apply the same criteria to the other cleanup calls).
> + }
+
+ @Override
+ public Boolean apply(final String serverId) {
+ final ServerApi serverApi = api.getServerApi();
+ Server server = serverApi.getServer(serverId);
+
+ if (server == null) {
+ return true;
+ }
+
+ if (server.state().isFailed()) {
+ rollbackOperation(format("Server(%s) not deleted as it is in state(%s).", serverId, server.state()));
+ }
+
+ if (server.state().isNormal()) {
Just return false if it is not normal and continue the rest of the method "outside" the conditional. It is easier to read.
--
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/422#pullrequestreview-80498909
Re: [jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + }
+ }
+
+ private void attemptDeleteNatRule(final String serverId, final NetworkApi networkApi, final NatRule natRule) {
+ try {
+ if (natRule.state().isNormal()) {
+ networkApi.deleteNatRule(natRule.id());
+ } else {
+ logger.warn(format("Server(%s) has an associated NatRule(%s) that was not deleted as it was in state(%s).",
+ serverId, natRule.id(), natRule.state()));
+ }
+ } catch (Throwable t) {
+ logger.warn(
+ format("Failed to delete NatRule(%s) associated with Server(%s). Due to - (%s)", natRule.id(), serverId,
+ t.getMessage()));
+ }
Better pass the `Throwable` as the first parameter to the logger to have the complete stacktrace logged. Often just the exception message does not give enough contex.
--
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/422#pullrequestreview-80893593
Re: [jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
Posted by Trevor Flanagan <no...@github.com>.
@trevorflanagan pushed 1 commit.
4b3a037 Implementing BaseImageToHardware and CleanupServer functions. PR updates.
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/422/files/9d7855515bcdf4d752dc6ff3d3527d71c5df2aa4..4b3a0377b40398b048cbe1bacd30c75f736e79b4
Re: [jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
Posted by Trevor Flanagan <no...@github.com>.
trevorflanagan commented on this pull request.
> + String networkDomainId = server.networkInfo().networkDomainId();
+ final String internalIp = server.networkInfo().primaryNic().privateIpv4();
+
+ // delete nat rules associated to the server, if any
+ final NetworkApi networkApi = api.getNetworkApi();
+ List<NatRule> natRulesToBeDeleted = networkApi.listNatRules(networkDomainId).concat()
+ .filter(new Predicate<NatRule>() {
+ @Override
+ public boolean apply(NatRule natRule) {
+ return natRule.internalIp().equals(internalIp);
+ }
+ }).toList();
+
+ for (final NatRule natRule : natRulesToBeDeleted) {
+ if (natRule.state().isNormal()) {
+ networkApi.deleteNatRule(natRule.id());
Makes sense to cleanup as much as possible without failing the full operation, I will update to have this, but I am not sure about the need for a try/catch.
--
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/422#discussion_r154491834
Re: [jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
Posted by Trevor Flanagan <no...@github.com>.
@trevorflanagan pushed 1 commit.
9d78555 Implementing BaseImageToHardware and CleanupServer functions. PR updates.
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/422/files/f04c95db74621e32aea2e6ad9f6996f7d10dc6bb..9d7855515bcdf4d752dc6ff3d3527d71c5df2aa4
Re: [jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
Posted by Trevor Flanagan <no...@github.com>.
@trevorflanagan pushed 1 commit.
f04c95d Implementing BaseImageToHardware and CleanupServer functions. PR updates.
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/422/files/08d902d63383da37b3fdc6b2024d0a623ac49deb..f04c95db74621e32aea2e6ad9f6996f7d10dc6bb
Re: [jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
Posted by Trevor Flanagan <no...@github.com>.
trevorflanagan commented on this pull request.
> + }
+
+ @Override
+ public Boolean apply(final String serverId) {
+ final ServerApi serverApi = api.getServerApi();
+ Server server = serverApi.getServer(serverId);
+
+ if (server == null) {
+ return true;
+ }
+
+ if (server.state().isFailed()) {
+ rollbackOperation(format("Server(%s) not deleted as it is in state(%s).", serverId, server.state()));
+ }
+
+ if (server.state().isNormal()) {
Much tidier, I will 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/422#discussion_r154491504
Re: [jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + String networkDomainId = server.networkInfo().networkDomainId();
+ final String internalIp = server.networkInfo().primaryNic().privateIpv4();
+
+ // delete nat rules associated to the server, if any
+ final NetworkApi networkApi = api.getNetworkApi();
+ List<NatRule> natRulesToBeDeleted = networkApi.listNatRules(networkDomainId).concat()
+ .filter(new Predicate<NatRule>() {
+ @Override
+ public boolean apply(NatRule natRule) {
+ return natRule.internalIp().equals(internalIp);
+ }
+ }).toList();
+
+ for (final NatRule natRule : natRulesToBeDeleted) {
+ if (natRule.state().isNormal()) {
+ networkApi.deleteNatRule(natRule.id());
Calls to the API can fail for a variety of reasons, say there is a conflict, a transient, error, whatever unexpected error that makes this raise an `HttpResponseException`. The `CleanupServer` is likely to be used when users want to destroy nodes and associated resources to "stop paying for them". We should be as defensive as possible and try to continue deleting stuff even if an unexpected exception occurred when trying to delete intermediate resources.
--
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/422#discussion_r154615490
Re: [jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
Posted by Ignasi Barrera <no...@github.com>.
Merged to [master](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/172d6f34) and [2.0.x](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/caf4c19a). Thanks @trevorflanagan!
I had to remove [an unnecessary call to `EasyMockSupport.injectMocks`](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/b4998732) to make it compile in 2.0.x (probably due to the easyMock version), and I've also applied it to master.
--
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/422#issuecomment-349232310
Re: [jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
Posted by Trevor Flanagan <no...@github.com>.
trevorflanagan commented on this pull request.
> + String networkDomainId = server.networkInfo().networkDomainId();
+ final String internalIp = server.networkInfo().primaryNic().privateIpv4();
+
+ // delete nat rules associated to the server, if any
+ final NetworkApi networkApi = api.getNetworkApi();
+ List<NatRule> natRulesToBeDeleted = networkApi.listNatRules(networkDomainId).concat()
+ .filter(new Predicate<NatRule>() {
+ @Override
+ public boolean apply(NatRule natRule) {
+ return natRule.internalIp().equals(internalIp);
+ }
+ }).toList();
+
+ for (final NatRule natRule : natRulesToBeDeleted) {
+ if (natRule.state().isNormal()) {
+ networkApi.deleteNatRule(natRule.id());
Understood, thanks for clarifying. I will make that change.
--
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/422#discussion_r154627697
Re: [jclouds/jclouds-labs] Implementing BaseImageToHardware and
CleanupServer functions. (#422)
Posted by Ignasi Barrera <no...@github.com>.
Closed #422.
--
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/422#event-1372130453