fix(provider/openstack): corrects behavior of status checkers for ope…

…nstack operations (#2482) Co-authored-by: Gardnem6 <12258900+Gardnem6@users.noreply.github.com> Co-authored-by: msilpala <msilpala@users.noreply.github.com> Co-authored-by: esengstrom <37663918+esengstrom@users.noreply.github.com>
spinnaker · Apr 6, 2018 · c260577 · c260577
1 parent 834fe13
commit c260577
Show file tree

Hide file tree

Showing 43 changed files with 603 additions and 314 deletions.
diff --git a/...in/groovy/com/netflix/spinnaker/clouddriver/openstack/client/BlockingStatusChecker.groovy b/...in/groovy/com/netflix/spinnaker/clouddriver/openstack/client/BlockingStatusChecker.groovy
@@ -18,6 +18,7 @@ package com.netflix.spinnaker.clouddriver.openstack.client
 
 import com.netflix.spinnaker.clouddriver.openstack.deploy.exception.OpenstackProviderException
 import groovy.transform.PackageScope
+import lombok.SneakyThrows
 
 import java.util.concurrent.TimeUnit
 
@@ -50,28 +51,21 @@ class BlockingStatusChecker {
   }
 
   @PackageScope
+  @SneakyThrows // used for the Thread.sleep(pollInterval)
   <T> T execute(Closure<T> closure) {
     long startTime = System.currentTimeMillis()
-    boolean isDone = false
+    T result
 
-    T result = closure.call()
-
-    while(!isDone) {
+    while(true) {
+      result = closure.call()
       if (statusChecker.isReady(result)) {
-        isDone = true
-      } else {
-        try {
-          sleep(pollInterval)
-        } catch (Exception e) {
-          //Do nothing
-        }
-        if ((System.currentTimeMillis() - startTime) > timeout) {
-          throw new OpenstackProviderException('Operation timed out')
-        }
+        return result
+      }
+      if ((System.currentTimeMillis() - startTime) > timeout) {
+        throw new OpenstackProviderException('Operation timed out')
       }
+      sleep(pollInterval)
     }
-
-    result
   }
 
   @PackageScope

diff --git a/...oovy/com/netflix/spinnaker/clouddriver/openstack/client/OpenstackComputeV2Provider.groovy b/...oovy/com/netflix/spinnaker/clouddriver/openstack/client/OpenstackComputeV2Provider.groovy
@@ -166,13 +166,9 @@ class OpenstackComputeV2Provider implements OpenstackComputeProvider, OpenstackR
 
   @Override
   SecGroupExtension getSecurityGroup(String region, String id) {
-    SecGroupExtension securityGroup = handleRequest {
+    handleRequest {
       client.useRegion(region).compute().securityGroups().get(id)
     }
-    if (!securityGroup) {
-      throw new OpenstackResourceNotFoundException("Unable to find security group ${id}")
-    }
-    securityGroup
   }
 
   @Override
@@ -184,13 +180,9 @@ class OpenstackComputeV2Provider implements OpenstackComputeProvider, OpenstackR
 
   @Override
   Server getServerInstance(String region, String instanceId) {
-    Server server = handleRequest {
+    handleRequest {
       client.useRegion(region).compute().servers().get(instanceId)
     }
-    if (!server) {
-      throw new OpenstackProviderException("Could not find server with id ${instanceId}")
-    }
-    server
   }
 
   @Override
@@ -203,6 +195,9 @@ class OpenstackComputeV2Provider implements OpenstackComputeProvider, OpenstackR
   @Override
   String getIpForInstance(String region, String instanceId) {
     Server server = getServerInstance(region, instanceId)
+    if (!server) {
+      throw new OpenstackResourceNotFoundException("unable to find instance: $instanceId in region: $region")
+    }
     /* TODO
       For now just get the first ipv4 address found. Openstack does not associate an instance id
       with load balancer membership, just an ip address. An instance can have multiple IP addresses.
@@ -220,6 +215,9 @@ class OpenstackComputeV2Provider implements OpenstackComputeProvider, OpenstackR
   @Override
   List<? extends Address> getIpsForInstance(String region, String instanceId) {
     Server server = getServerInstance(region, instanceId)
+    if (!server) {
+      throw new OpenstackResourceNotFoundException("unable to find instance: $instanceId in region: $region")
+    }
     server.addresses?.addresses?.collect { n -> n.value }?.flatten()
   }
 

diff --git a/...com/netflix/spinnaker/clouddriver/openstack/client/OpenstackLoadBalancerV2Provider.groovy b/...com/netflix/spinnaker/clouddriver/openstack/client/OpenstackLoadBalancerV2Provider.groovy
@@ -66,14 +66,9 @@ class OpenstackLoadBalancerV2Provider implements OpenstackLoadBalancerProvider,
 
   @Override
   LoadBalancerV2 getLoadBalancer(final String region, final String id) {
-    LoadBalancerV2 result = handleRequest {
+    handleRequest {
       getRegionClient(region).networking().lbaasV2().loadbalancer().get(id)
     }
-
-    if (!result) {
-      throw new OpenstackResourceNotFoundException("Unable to find load balancer ${id} in ${region}")
-    }
-    result
   }
 
   @Override

diff --git a/...om/netflix/spinnaker/clouddriver/openstack/client/OpenstackOrchestrationV1Provider.groovy b/...om/netflix/spinnaker/clouddriver/openstack/client/OpenstackOrchestrationV1Provider.groovy
@@ -90,13 +90,9 @@ class OpenstackOrchestrationV1Provider implements OpenstackOrchestrationProvider
 
   @Override
   Stack getStack(String region, String stackName) {
-    Stack stack = handleRequest {
+    handleRequest {
       getRegionClient(region).heat().stacks().getStackByName(stackName)
     }
-    if (!stack) {
-      throw new OpenstackProviderException("Unable to find stack $stackName in region $region")
-    }
-    stack
   }
 
   @Override

diff --git a/...groovy/com/netflix/spinnaker/clouddriver/openstack/client/OpenstackSwiftV1Provider.groovy b/...groovy/com/netflix/spinnaker/clouddriver/openstack/client/OpenstackSwiftV1Provider.groovy
@@ -46,11 +46,7 @@ class OpenstackSwiftV1Provider implements OpenstackSwiftProvider, OpenstackReque
       }
 
       // TODO consider checking content type before reading the response to ensure it is text
-      String entity = response.getEntity(String)
-      if (!entity) {
-        throw new OpenstackResourceNotFoundException("Failed to read the Swift object ${container}/${name} in region ${region}")
-      }
-      entity
+      return response.getEntity(String)
     }
   }
 }
diff --git a/...om/netflix/spinnaker/clouddriver/openstack/config/OpenstackConfigurationProperties.groovy b/...om/netflix/spinnaker/clouddriver/openstack/config/OpenstackConfigurationProperties.groovy
@@ -19,6 +19,8 @@ package com.netflix.spinnaker.clouddriver.openstack.config
 import com.netflix.spinnaker.clouddriver.consul.config.ConsulConfig
 import groovy.transform.ToString
 
+import java.util.concurrent.TimeUnit
+
 @ToString(includeNames = true)
 class OpenstackConfigurationProperties {
 
@@ -35,14 +37,20 @@ class OpenstackConfigurationProperties {
     List<String> regions
     Boolean insecure
     String heatTemplatePath
-    LbaasConfig lbaas
+    LbaasConfig lbaas = new LbaasConfig()
+    StackConfig stack = new StackConfig()
     ConsulConfig consul
     String userDataFile
   }
 
   static class LbaasConfig {
-    Integer pollTimeout
-    Integer pollInterval
+    int pollTimeout = 60 // seconds
+    int pollInterval = 5 // seconds
+  }
+
+  static class StackConfig {
+    int pollTimeout = TimeUnit.MINUTES.toSeconds(10).toInteger()
+    int pollInterval = 5 // seconds
   }
 
   List<ManagedAccount> accounts = []

diff --git a/...ovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/LoadBalancerStatusAware.groovy b/...ovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/LoadBalancerStatusAware.groovy
diff --git a/...groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/StackPoolMemberAware.groovy b/...groovy/com/netflix/spinnaker/clouddriver/openstack/deploy/ops/StackPoolMemberAware.groovy
@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.clouddriver.openstack.deploy.ops
 
 import com.netflix.spinnaker.clouddriver.openstack.deploy.description.servergroup.MemberData
+import com.netflix.spinnaker.clouddriver.openstack.deploy.exception.OpenstackResourceNotFoundException
 import com.netflix.spinnaker.clouddriver.openstack.security.OpenstackCredentials
 import org.openstack4j.model.network.ext.ListenerV2
 import org.openstack4j.model.network.ext.LoadBalancerV2
@@ -35,6 +36,10 @@ trait StackPoolMemberAware {
   List<MemberData> buildMemberData(OpenstackCredentials credentials, String region, String subnetId, List<String> lbIds, Closure portParser) {
     lbIds.collectMany { loadBalancerId ->
       LoadBalancerV2 loadBalancer = credentials.provider.getLoadBalancer(region, loadBalancerId)
+      if (!loadBalancer) {
+        throw new OpenstackResourceNotFoundException("Could not find load balancer: $loadBalancerId in region: $region")
+      }
+
       loadBalancer.listeners.collect { item ->
         ListenerV2 listener = credentials.provider.getListener(region, item.id)
         String listenerShortId

diff --git a/...penstack/deploy/ops/instance/AbstractRegistrationOpenstackInstancesAtomicOperation.groovy b/...penstack/deploy/ops/instance/AbstractRegistrationOpenstackInstancesAtomicOperation.groovy
@@ -23,15 +23,16 @@ import com.netflix.spinnaker.clouddriver.openstack.client.OpenstackClientProvide
 import com.netflix.spinnaker.clouddriver.openstack.deploy.description.instance.OpenstackInstancesRegistrationDescription
 import com.netflix.spinnaker.clouddriver.openstack.deploy.exception.OpenstackOperationException
 import com.netflix.spinnaker.clouddriver.openstack.deploy.exception.OpenstackProviderException
-import com.netflix.spinnaker.clouddriver.openstack.deploy.ops.LoadBalancerStatusAware
+import com.netflix.spinnaker.clouddriver.openstack.deploy.exception.OpenstackResourceNotFoundException
+import com.netflix.spinnaker.clouddriver.openstack.deploy.ops.loadbalancer.LoadBalancerChecker
 import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation
 import org.openstack4j.model.network.ext.ListenerV2
 import org.openstack4j.model.network.ext.LoadBalancerV2
 
 /**
  * Base class that will handle both load balancer registration and deregistration.
  */
-abstract class AbstractRegistrationOpenstackInstancesAtomicOperation implements AtomicOperation<Void>, LoadBalancerStatusAware {
+abstract class AbstractRegistrationOpenstackInstancesAtomicOperation implements AtomicOperation<Void> {
 
   abstract String getBasePhase() // Either 'REGISTER' or 'DEREGISTER'.
   abstract Boolean getAction() // Either 'true' or 'false', for Register and Deregister respectively.
@@ -56,9 +57,12 @@ abstract class AbstractRegistrationOpenstackInstancesAtomicOperation implements
       task.updateStatus basePhase, "Start $verb all instances $preposition load balancers..."
       OpenstackClientProvider provider = description.credentials.provider
       description.loadBalancerIds.each { lb ->
-        BlockingStatusChecker checker = createBlockingActiveStatusChecker(description.credentials, description.region, lb)
         task.updateStatus basePhase, "Getting details for load balancer $lb..."
         LoadBalancerV2 loadBalancer = provider.getLoadBalancer(description.region, lb)
+        if (!loadBalancer) {
+          throw new OpenstackResourceNotFoundException("Could not find load balancer: $lb in region: $description.region")
+        }
+
         description.instanceIds.each { id ->
           task.updateStatus basePhase, "Getting ip address for service instance $id..."
           String ip = provider.getIpForInstance(description.region, id)
@@ -69,15 +73,19 @@ abstract class AbstractRegistrationOpenstackInstancesAtomicOperation implements
               task.updateStatus basePhase, "Getting internal port from load balancer $loadBalancer.name for listener $listenerItem.id..."
               int internalPort = provider.getInternalLoadBalancerPort(description.region, listenerItem.id)
               task.updateStatus basePhase, "Adding member with ip $ip to load balancer $loadBalancer.name on internal port $internalPort with weight $description.weight..."
-              checker.execute {
-                provider.addMemberToLoadBalancerPool(description.region, ip, listener.defaultPoolId, loadBalancer.vipSubnetId, internalPort, description.weight)
+              provider.addMemberToLoadBalancerPool(description.region, ip, listener.defaultPoolId, loadBalancer.vipSubnetId, internalPort, description.weight)
+              task.updateStatus basePhase, "Waiting on member add status with ip $ip to load balancer $loadBalancer.name on internal port $internalPort with weight $description.weight..."
+              LoadBalancerChecker.from(description.credentials.credentials.lbaasConfig, LoadBalancerChecker.Operation.UPDATE).execute {
+                provider.getLoadBalancer(description.region, lb)
               }
             } else {
               task.updateStatus basePhase, "Getting member id for server instance $id and ip $ip on load balancer $loadBalancer.name..."
               String memberId = provider.getMemberIdForInstance(description.region, ip, listener.defaultPoolId)
               task.updateStatus basePhase, "Removing member with ip $ip from load balancer $loadBalancer.name..."
-              checker.execute {
-                provider.removeMemberFromLoadBalancerPool(description.region, listener.defaultPoolId, memberId)
+              provider.removeMemberFromLoadBalancerPool(description.region, listener.defaultPoolId, memberId)
+              task.updateStatus basePhase, "Waiting on remove status for mmber with ip $ip from load balancer $loadBalancer.name..."
+              LoadBalancerChecker.from(description.credentials.credentials.lbaasConfig, LoadBalancerChecker.Operation.UPDATE).execute {
+                provider.getLoadBalancer(description.region, lb)
               }
             }
           }

diff --git a/...ouddriver/openstack/deploy/ops/instance/TerminateOpenstackInstancesAtomicOperation.groovy b/...ouddriver/openstack/deploy/ops/instance/TerminateOpenstackInstancesAtomicOperation.groovy
@@ -18,6 +18,7 @@ package com.netflix.spinnaker.clouddriver.openstack.deploy.ops.instance
 
 import com.netflix.spinnaker.clouddriver.openstack.client.OpenstackClientProvider
 import com.netflix.spinnaker.clouddriver.openstack.deploy.description.instance.OpenstackInstancesDescription
+import com.netflix.spinnaker.clouddriver.openstack.deploy.exception.OpenstackResourceNotFoundException
 import com.netflix.spinnaker.clouddriver.openstack.deploy.ops.servergroup.AbstractStackUpdateOpenstackAtomicOperation
 import com.netflix.spinnaker.clouddriver.openstack.domain.LoadBalancerResolver
 import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperations
@@ -57,7 +58,13 @@ class TerminateOpenstackInstancesAtomicOperation extends AbstractStackUpdateOpen
     if (instanceId) {
       task.updateStatus phaseName, "Getting server group name from instance $instanceId ..."
       Server server = provider.getServerInstance(description.region, instanceId)
+      if (!server) {
+        throw new OpenstackResourceNotFoundException("Could not find server: $instanceId in region: $description.region")
+      }
       serverGroupName = server.metadata?.get("metering.stack.name") ?: provider.getStack(description.region, server.metadata?.get("metering.stack"))?.name
+      if (!serverGroupName) {
+        throw new OpenstackResourceNotFoundException("Could not find server group name for server: $instanceId")
+      }
       task.updateStatus phaseName, "Found server group name $serverGroupName from instance $instanceId."
     }
     serverGroupName
@@ -71,12 +78,18 @@ class TerminateOpenstackInstancesAtomicOperation extends AbstractStackUpdateOpen
     Resource asg = provider.getAsgResourceForStack(description.region, stack)
     task.updateStatus phaseName, "Finding nested stack for resource $asg.type ..."
     Stack nested = provider.getStack(description.region, asg.physicalResourceId)
+    if (!nested) {
+      throw new OpenstackResourceNotFoundException("Could not find stack $asg.physicalResourceId in region: $description.region")
+    }
 
     description.instanceIds.each { id ->
 
       //get server name
       task.updateStatus phaseName, "Getting server details for $id ..."
       Server server = provider.getServerInstance(description.region, id)
+      if (!server) {
+        throw new OpenstackResourceNotFoundException("Could not find server: $id in region: $description.region")
+      }
 
       //get resource
       task.updateStatus phaseName, "Finding server group resource for $id ..."