Implement OpenStackService to use OpenStack native API (1) (Instances) (List)

@jigishaddweb

Thank you for adding the feature to support OpenStack REST API.

1. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackServiceFactory.php
   @@ -0,0 +1,232 @@
   +    return !empty($cloud_config->field_use_openstack_ec2_api->value)
   +    ? new OpenStackEc2Service(
   +        $this->entityTypeManager,
   +        $this->configFactory,
   +        $this->currentUser,
   +        $this->cloudConfigPluginManager,
   +        $this->fieldTypePluginManager,
   +        $this->entityFieldManager,
   +        $this->lock,
   +        $this->queueFactory,
   +        $this->httpClient,
   +        $this->moduleHandler
   +      )
   +    : new OpenStackRestService(
   +        $this->entityTypeManager,
   +        $this->cloudConfigPluginManager,
   +        $this->clientFactory,
   +        $this->state,
   +        $this->lock
   +      );
   

   Can we write this one as follows w/ $openstack_service->setCloudContext($cloud_context);? In this way, we don't have to put $this->openStackService->setCloudContext($cloud_context); in each occurrence.

       $openstack_service = !empty($cloud_config->field_use_openstack_ec2_api->value)
       ? new OpenStackEc2Service(
           $this->entityTypeManager,
           $this->configFactory,
           $this->currentUser,
           $this->cloudConfigPluginManager,
           $this->fieldTypePluginManager,
           $this->entityFieldManager,
           $this->lock,
           $this->queueFactory,
           $this->httpClient,
           $this->moduleHandler
         )
       : new OpenStackRestService(
           $this->entityTypeManager,
           $this->cloudConfigPluginManager,
           $this->clientFactory,
           $this->state,
           $this->lock
         );
   
        $openstack_service->setCloudContext($cloud_context);
        return $openstack_service;
   

2. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackServiceFactory.php
   @@ -0,0 +1,232 @@
   +    $openstack_ec2_api = $cloud_config->field_use_openstack_ec2_api->value;
   

   This code can be removed since it is not used.

@jigishaddweb

Thank you for the update.

+++ b/modules/cloud_service_providers/openstack/openstack.module
@@ -72,19 +75,21 @@ function openstack_cron() {
+  /** @var \Drupal\openstack\Service\OpenStackFactory $openstack_service */
+  $openstack_service = $openstack_factory->get($cloud_config->getCloudContext());

+++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
@@ -125,16 +156,21 @@ class OpenStackBatchOperations {
+      /** @var \Drupal\openstack\Service\Ec2\OpenStackService $openstack_service */
+    $openstack_service = $openstack_factory->get($cloud_context);

+++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
@@ -229,9 +265,10 @@ class OpenStackBatchOperations {
+    /** @var \Drupal\openstack\Service\Ec2\OpenStackService $openstack_service */
+    $openstack_service = $openstack_factory->get($cloud_context);

+++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
@@ -312,12 +349,13 @@ class OpenStackBatchOperations {
+    /** @var \Drupal\openstack\Service\Ec2\OpenStackService $openstack_service */
+    $openstack_service = $openstack_factory->get($cloud_context);

+++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
@@ -348,14 +386,15 @@ class OpenStackBatchOperations {
+    /** @var \Drupal\openstack\Service\Ec2\OpenStackService $openstack_service */
+    $openstack_service = $openstack_factory->get($cloud_context);

+++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
@@ -421,9 +460,10 @@ class OpenStackBatchOperations {
+    /** @var \Drupal\openstack\Service\Ec2\OpenStackService $openstack_service */
+    $openstack_service = $openstack_factory->get($cloud_context);

+++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
@@ -495,14 +535,15 @@ class OpenStackBatchOperations {
+    /** @var \Drupal\openstack\Service\Ec2\OpenStackService $openstack_service */
+    $openstack_service = $openstack_factory->get($cloud_context);

The comments should be:

    /** @var \Drupal\openstack\Service\OpenStackServiceInterface $openstack_service */

@jigishaddweb

Thank you for the update. I tested the patch and it looks good to me now.

@baldwinlouie

Could you please review the patch?

@jigishaddweb

I have the following error during the cron job. Could you please take a look at it?

TypeError: Argument 1 passed to Drupal\openstack\Service\Ec2\OpenStackService::addMockHandler() must be an instance of Aws\Ec2\Ec2Client, null given, called in /var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/openstack/src/Service/Ec2/OpenStackService.php on line 133 in Drupal\openstack\Service\Ec2\OpenStackService->addMockHandler() (line 148 of /var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/openstack/src/Service/Ec2/OpenStackService.php)
#0 /var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/openstack/src/Service/Ec2/OpenStackService.php(133): Drupal\openstack\Service\Ec2\OpenStackService->addMockHandler(NULL)
#1 /var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php(374): Drupal\openstack\Service\Ec2\OpenStackService->getEc2Client(Array)
#2 /var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php(742): Drupal\aws_cloud\Service\Ec2\Ec2Service->execute('DescribeSnapsho...', Array, Array)
#3 /var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/openstack/src/Service/Ec2/OpenStackService.php(746): Drupal\aws_cloud\Service\Ec2\Ec2Service->describeSnapshots()
#4 /var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/openstack/src/Service/Ec2/OpenStackService.php(798): Drupal\openstack\Service\Ec2\OpenStackService->updateSnapshotEntities('openstack_snaps...', 'test_regionone')
#5 

@jigishaddweb,

This is a good looking patch. Thank you. I have the following comments:

1. +++ b/modules/cloud_service_providers/openstack/src/Controller/ApiController.php
   @@ -362,12 +379,20 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +        $opnestack_service = $this->openStackServiceFactory->get($cloud_context);
   +        $updated = $opnestack_service->$update_method_name();
   

   There's a typo here. $opnestack_service should be $openstack_service

2. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
   @@ -35,6 +35,30 @@ class OpenStackBatchOperations {
   +  private static function openStackServiceType($cloud_context) {
   

   If this method is returning boolean, I think the method should be changed to something like isEc2ServiceType() or isRestServiceType(). Something that follows makes it easier to understand that the method returns a boolean

3. +++ b/modules/cloud_service_providers/openstack/openstack.module
   @@ -370,6 +381,30 @@ function openstack_form_cloud_config_openstack_credentials_validate(array &$form
   +    if ($response === 401) {
   +      $form_state->setErrorByName(
   +        'cloud_context',
   +        t('Please check the credentials you have entered.')
   +      );
   +    }
   +  }
   +
   

   What happens if the $response code is another error code, such as 500?

   I think its better if the method generateAuthToken($credentials)
   does a try/catch. If the response code is anything other than a 200, it should throw an exception. openstack_form_cloud_config_openstack_credentials_validate() can catch the exception to perform error handling.

4. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
   @@ -229,9 +265,10 @@ class OpenStackBatchOperations {
   +    /** @var \Drupal\openstack\Service\OpenStackFactory $openstack_factory */
   +    $openstack_factory = \Drupal::service('openstack.factory');
   +    /** @var \Drupal\openstack\Service\OpenStackServiceInterface $openstack_service */
   +    $openstack_service = $openstack_factory->get($cloud_context);
   

   This can be combined like such:

   $openstack_service = \Drupal::service('openstack.factory')->get($cloud_context);
   

5. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
   @@ -312,12 +349,13 @@ class OpenStackBatchOperations {
   +    /** @var \Drupal\openstack\Service\OpenStackFactory $openstack_factory */
   +    $openstack_factory = \Drupal::service('openstack.factory');
   +    /** @var \Drupal\openstack\Service\OpenStackServiceInterface $openstack_service */
   +    $openstack_service = $openstack_factory->get($cloud_context);
   

   Same here

6. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
   @@ -348,14 +386,15 @@ class OpenStackBatchOperations {
   +    /** @var \Drupal\openstack\Service\OpenStackFactory $openstack_factory */
   +    $openstack_factory = \Drupal::service('openstack.factory');
   ...
   +    $openstack_service = $openstack_factory->get($cloud_context);
   

   Same here

7. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
   @@ -421,9 +460,10 @@ class OpenStackBatchOperations {
   +    /** @var \Drupal\openstack\Service\OpenStackFactory $openstack_factory */
   +    $openstack_factory = \Drupal::service('openstack.factory');
   +    /** @var \Drupal\openstack\Service\OpenStackServiceInterface $openstack_service */
   +    $openstack_service = $openstack_factory->get($cloud_context);
   

   Same here

8. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
   @@ -495,14 +535,15 @@ class OpenStackBatchOperations {
   +    /** @var \Drupal\openstack\Service\OpenStackFactory $openstack_factory */
   +    $openstack_factory = \Drupal::service('openstack.factory');
   +    /** @var \Drupal\openstack\Service\OpenStackServiceInterface $openstack_service */
   +    $openstack_service = $openstack_factory->get($cloud_context);
   

   Same here

9. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
   @@ -553,9 +594,10 @@ class OpenStackBatchOperations {
   +    /** @var \Drupal\openstack\Service\OpenStackFactory $openstack_factory */
   +    $openstack_factory = \Drupal::service('openstack.factory');
   +    /** @var \Drupal\openstack\Service\OpenStackServiceInterface $openstack_service */
   +    $openstack_service = $openstack_factory->get($cloud_context);
    
   

   Same here

10. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
    @@ -697,14 +739,15 @@ class OpenStackBatchOperations {
    +    /** @var \Drupal\openstack\Service\OpenStackFactory $openstack_factory */
    +    $openstack_factory = \Drupal::service('openstack.factory');
    +    /** @var \Drupal\openstack\Service\OpenStackServiceInterface $openstack_service */
    +    $openstack_service = $openstack_factory->get($cloud_context);
    

    Same here

11. +++ b/modules/cloud_service_providers/openstack/src/Service/Rest/OpenStackService.php
    @@ -55,32 +129,434 @@ class OpenStackService extends CloudServiceBase {
    +    batch_process();
    

    Please make sure to support Drush running the batch function.

    Please look at the runBatch() for Ec2Service.php

    Here is the example code from that method. Make sure to do a try/catch/finally. Also, note how drush_backend_batch_process vs batch_process() is called.

        try {
          // Log the start time.
          $start = $this->getTimestamp();
          $batch_array = $batch_builder->toArray();
          batch_set($batch_array);
          // Reset the progressive so batch works with out a web head.
          $batch = &batch_get();
          $batch['progressive'] = FALSE;
          // Check if using Drush to process the queue.
          // using drush_backend_batch_process() will fix the "Route not found"
          // error.
          if (PHP_SAPI === 'cli' && function_exists('drush_backend_batch_process')) {
            drush_backend_batch_process();
          }
          else {
            batch_process();
          }
          // Log the end time.
          $end = $this->getTimestamp();
          $this->logger('ec2_service')->info($this->t('@updater - @cloud_context: Batch operation took @time seconds.', [
            '@cloud_context' => $this->cloudContext,
            '@updater' => $batch_array['title'],
            '@time' => $end - $start,
          ]));
        }
        catch (Exception $e) {
          $this->handleException($e);
        }
        finally {
          // Reset the batch otherwise this operation hangs when using Drush.
          // https://www.drupal.org/project/drupal/issues/3166042
          $batch = [];
        }
    

@jigishaddweb

Thank you for the update. I commented in your MR.

@jigishaddweb

Thank you for the update.

@baldwinlouie

What do you think?

@jigishaddweb,

Thank you for updating the patch. I only see a couple of items below.

1. +++ b/modules/cloud_service_providers/openstack/openstack.services.yml
   @@ -1,8 +1,15 @@
   +parameters:
   +  cloud_context: ''
   +
   

   do we need to declare this parameter in openstack.services.yml

2. +++ b/modules/cloud_service_providers/openstack/src/Controller/ApiController.php
   @@ -22,11 +23,18 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +  protected $entityTypeManager;
   

   This does not need to be declared. It already declared in ControllerBase

3. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackBatchOperations.php
   @@ -125,16 +129,21 @@ class OpenStackBatchOperations {
   +      /** @var \Drupal\openstack\Service\OpenStackServiceInterface $openstack_service */
   

   Please double check this variable declaration. I don't think it is needed.

@jigishaddweb, thank you for the updated patch. It looks good now.

@baldwinlouie

Thank you for your review. I'll merge the patch to 3.x and close this issue as Fixed.