Manage OpenStack ServerTemplate CRUD

@jigish.addweb @yas

Thanks for the patch. It looks good to me.

@jigish and @yas, thank you for the patch. I have comments for both @jigish and @yas. Please review them.

1. +++ b/modules/cloud_service_providers/aws_cloud/aws_cloud.module
   @@ -320,6 +328,9 @@ function aws_cloud_subnet_allowed_values_function(FieldStorageConfig $definition
   @@ -1843,8 +1854,14 @@ function _aws_cloud_delete_server_template($cloud_config) {
   

   This function needs a bit of work. The following code does not apply to Openstack. There are no LaunchTemplates in Openstack.

       // Delete Launchtemplate.
       foreach ($cloud_config_entities ?: [] as $cloud_config_entity) {
         $entity_name = $cloud_config_entity->getName();
         $result = $ec2_service->deleteLaunchTemplate([
           'LaunchTemplateName' => $entity_name,
         ]);
         if (isset($result['LaunchTemplate'])) {
           \Drupal::messenger()->addMessage(
             t('The launch template @name has been deleted.', [
               '@name' => $entity_name,
             ])
           );
         }
         else {
           \Drupal::messenger()->addError(
             t('The launch template "@name" couldn\'t delete.', [
               '@name' => $entity_name,
             ])
           );
         }
       }
   

2. +++ b/modules/cloud_service_providers/openstack/openstack.module
   @@ -810,3 +880,563 @@ function openstack_query_openstack_instance_views_access_alter(AlterableInterfac
   +  $form['bastion'] = [
   +    '#type' => 'details',
   +    '#title' => t('Bastion'),
   +    '#open' => TRUE,
   +  ];
   +
   +  $form['bastion']['as_bastion'] = [
   +    '#type' => 'checkbox',
   +    '#title' => t('As a bastion instance'),
   +    '#description' => t('Launch as a bastion instance.'),
   +    '#default_value' => FALSE,
   +    '#states' => [
   +      'visible' => [
   +        ':input[name="bastion_instance"]' => ['value' => ''],
   +      ],
   +    ],
   +  ];
   

   @yas, Is Bastion required or needed for Openstack?

3. +++ b/modules/cloud_service_providers/openstack/openstack.module
   @@ -810,3 +880,563 @@ function openstack_query_openstack_instance_views_access_alter(AlterableInterfac
   + * Return orders of AWS Cloud cloud server template fields.
   

   The comment should apply to Openstack.

4. +++ b/modules/cloud_service_providers/openstack/src/Form/Config/OpenStackAdminSettings.php
   @@ -157,6 +157,50 @@ class OpenStackAdminSettings extends ConfigFormBase {
   +    $form['schedule']['schedule_settings'] = [
   +      '#type' => 'details',
   +      '#title' => $this->t('OpenStack Instance Scheduler'),
   +      '#open' => TRUE,
   +    ];
   +
   +    $form['schedule']['schedule_settings']['openstack_scheduler_tag'] = [
   +      '#type' => 'textfield',
   +      '#title' => $this->t('Schedule Tag'),
   +      '#description' => $this->t('Name of scheduling tag. This tag value is defined when setting up the <a href=":stack">OpenStack Instance Scheduler</a>.', [
   +        ':stack' => 'https://docs.aws.amazon.com/solutions/latest/instance-scheduler/deployment.html',
   +      ]),
   +      '#default_value' => $config->get('openstack_scheduler_tag'),
   +    ];
   +
   +    $form['schedule']['schedule_settings']['openstack_scheduler_periods'] = [
   +      '#type' => 'textarea',
   +      '#title' => $this->t('Schedule periods'),
   +      '#description' => $this->t('<p>Schedules defined in OpenStack Instance Scheduler. The values entered are shown in the Schedule field on instance edit form and launch template launch form. Enter one value per line, in the format <strong>key|label</strong>.</p><p>The key corresponds to the schedule name defined in OpenStack Instance Scheduler. The label is a free form descriptive value shown to users. An example configuration might be:<br/>office-hours|Office Hours - Monday to Friday 9:00am - 5:00pm.<br/><p>See <a href=:stack>Scheduler Configuration</a> for more information.</p>', [
   +        ':stack' => 'https://docs.aws.amazon.com/solutions/latest/instance-scheduler/components.html',
   +      ]),
   +      '#default_value' => $config->get('openstack_scheduler_periods'),
   +    ];
   +
   +    $form['cost_management'] = [
   +      '#type' => 'details',
   +      '#title' => $this->t('Cost Management'),
   +      '#open' => TRUE,
   +    ];
   +
   +    $form['cost_management']['openstack_instance_type_cost'] = [
   +      '#type' => 'checkbox',
   +      '#title' => $this->t('Instance Type Cost'),
   +      '#description' => $this->t('Enable Instance Type cost in launch template create or edit form.'),
   +      '#default_value' => $config->get('openstack_instance_type_cost'),
   +    ];
   +
   +    $form['cost_management']['openstack_instance_type_cost_list'] = [
   +      '#type' => 'checkbox',
   +      '#title' => $this->t('Instance Type Cost List'),
   +      '#description' => $this->t('Enable Instance Type Cost List in launch template launch form.'),
   +      '#default_value' => $config->get('openstack_instance_type_cost_list'),
   +    ];
   +
   

   @yas, please verify if these are all required for Openstack. For example, I don't think scheduler_tag will work for Openstack, since it pertains to AWS Instance Scheduler. What about Cost Management and Instance Type Cost List

5. +++ b/modules/cloud_service_providers/openstack/src/Plugin/cloud/server_template/OpenStackServerTemplatePlugin.php
   @@ -0,0 +1,646 @@
   +    // Create a new VPC peering connection.
   +    $result = $this->ec2Service->createVpcPeeringConnection([
   +      'VpcId' => $from_vpc_id,
   +      'PeerVpcId' => $to_vpc_id,
   +    ]);
   +    $connection_id = $result['VpcPeeringConnection']['VpcPeeringConnectionId'];
   +
   +    // Accept a VPC peering connection request.
   +    $result = $this->ec2Service->acceptVpcPeeringConnection([
   +      'VpcPeeringConnectionId' => $connection_id,
   +    ]);
   +    if (empty($result['VpcPeeringConnection'])) {
   +      $this->messenger->addError($this->t(
   +        'Failed to accept the VPC peering connection request. It is possible due to that two VPCs have overlapping CIDR blocks. Please confirm the CIDR blocks of the VPC @from_vpc_id and @to_vpc_id.',
   +        ['@from_vpc_id' => $from_vpc_id, '@to_vpc_id' => $to_vpc_id]
   +      ));
   +      return FALSE;
   +    }
   +
   +    $this->messenger->addMessage($this->t(
   +      'The VPC peering connection was created from the VPC @from_vpc_id to the VPC @to_vpc_id.',
   +      ['@from_vpc_id' => $from_vpc_id, '@to_vpc_id' => $to_vpc_id]
   +    ));
   

   Please verify if VpcPeering API works for openstack. When I check the documentation, it seems like a majority of the Peering Connection api is not supported.

   https://docs.openstack.org/api-ref/ec2-api/

6. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackEc2Service.php
   @@ -636,6 +704,120 @@ class OpenStackEc2Service extends Ec2Service {
   +    $result = $this->describeLaunchTemplates($params);
   +
   +    if (isset($result)) {
   +
   +      $stale = [];
   +      foreach ($entities ?: [] as $entity) {
   +        $stale[$entity->getName()] = $entity;
   +      }
   +
   +      /* @var \Drupal\Core\Batch\BatchBuilder $batch_builder */
   +      $batch_builder = $this->initBatch('Launch Template Update');
   

   I don't think LaunchTemplates are supported in Openstack? Please verify.

7. +++ b/modules/cloud_service_providers/openstack/src/Service/OpenStackEc2Service.php
   @@ -636,6 +704,120 @@ class OpenStackEc2Service extends Ec2Service {
   +    $result = $this->describeLaunchTemplates($params);
   +
   +    if (isset($result)) {
   +
   +      $stale = [];
   +      foreach ($entities ?: [] as $entity) {
   +        $stale[$entity->getName()] = $entity;
   +      }
   +
   +      foreach ($result['LaunchTemplates'] ?: [] as $template) {
   +        if (isset($stale[$template['LaunchTemplateName']])) {
   +          unset($stale[$template['LaunchTemplateName']]);
   +        }
   +
   +        OpenStackBatchOperations::updateCloudServerTemplate($this->cloudContext, $template);
   +      }
   +
   

   Please verify Launch Templates. I don't think they are supported in Openstack.

@baldwinlouie

Thank you for your high quality review for the patch. Basically we should strip out unsupported AWS features.

1. OpenStack doesn't support Launch Template, so we should remove that code.
2. We don't have to include bastion-related logic into the code. Bastion concept is still necessary even in OpenStack, however it should be implemented in the future.
3. Thanksる
4. We can remove scheduler- and cost_management-related form items.
5. We can remove VPC peering-related code.
6. We can remove that code related to Launch Template.
7. Ditto.

@jigishaddweb

Could you please update the patch?

@jigish, and @yas,

Thank you @jigish for the updated patch. It is looking good. I have the following comments. @yas, please review them as well, and let me know your thoughts.

1. +++ b/modules/cloud_service_providers/aws_cloud/aws_cloud.module
   @@ -162,10 +162,12 @@ function aws_cloud_image_allowed_values_function(FieldStorageConfig $definition,
   +    if ($entity->isNew() && ($entity->bundle() === 'aws_cloud' || $entity->bundle() === 'openstack')) {
   

   I don't think we should have Openstack logic in the aws_cloud module.

   I think we should have an Openstack version of this function.

2. +++ b/modules/cloud_service_providers/aws_cloud/aws_cloud.module
   @@ -212,11 +214,15 @@ function aws_cloud_image_allowed_values_function(FieldStorageConfig $definition,
    function aws_cloud_availability_zone_allowed_values_function(FieldStorageConfig $definition, ContentEntityInterface $entity = NULL, $cacheable) {
    
   +  $module_name = (!empty($entity) && $entity->bundle() === 'aws_cloud') ? 'aws_cloud' : 'openstack';
   +
   

   I don't think we should have Openstack logic in the aws_cloud module.

   I think we should have an Openstack version of this function.

3. +++ b/modules/cloud_service_providers/aws_cloud/aws_cloud.module
   @@ -253,11 +259,13 @@ function aws_cloud_availability_zone_allowed_values_function(FieldStorageConfig
    function aws_cloud_vpc_allowed_values_function(FieldStorageConfig $definition, ContentEntityInterface $entity = NULL, $cacheable) {
      $allowed_values = [];
   +  $module_name = (!empty($entity) && $entity->bundle() === 'aws_cloud') ? 'aws_cloud' : 'openstack';
   +
   

   I don't think we should have Openstack logic in the aws_cloud module.

   I think we should have an Openstack version of this function.

4. +++ b/modules/cloud_service_providers/aws_cloud/aws_cloud.module
   @@ -293,7 +301,7 @@ function aws_cloud_vpc_allowed_values_function(FieldStorageConfig $definition, C
   @@ -320,6 +328,9 @@ function aws_cloud_subnet_allowed_values_function(FieldStorageConfig $definition
   
   @@ -320,6 +328,9 @@ function aws_cloud_subnet_allowed_values_function(FieldStorageConfig $definition
      if (!empty($entity) && $entity->bundle() === 'aws_cloud') {
        $allowed_values = aws_cloud_get_subnet_options_by_vpc_id(NULL, $entity);
      }
   +  else {
   +    $allowed_values = openstack_get_subnet_options_by_vpc_id(NULL, $entity);
   +  }
   

   I don't think we should have Openstack logic in the aws_cloud module.

   I think we should have an Openstack version of this function.

5. +++ b/modules/cloud_service_providers/aws_cloud/src/Plugin/Field/Util/AwsCloudReservedKeyChecker.php
   +++ b/modules/cloud_service_providers/aws_cloud/src/Plugin/Field/Util/AwsCloudReservedKeyChecker.php
   @@ -26,12 +26,12 @@ class AwsCloudReservedKeyChecker implements ReservedKeyCheckerInterface {
   

   I don't think we should have Openstack logic in AWS.

   Should we subclass this for Openstack and add the Openstack specific logic in there.

@baldwinlouie

Thank you for your review.

For 1-5. I agree to your comments. We should not include hard-coded string like 'openstack' in aws_cloud.module.

@jigishaddweb

Can you modify the logic?

@jigish, @yas,

Thank you for the updated patch. It looks good now. I only have one comment below.

+++ b/modules/cloud_service_providers/openstack/src/Plugin/cloud/server_template/OpenStackServerTemplatePlugin.php
@@ -0,0 +1,540 @@
+      $images = \Drupal::entityTypeManager()

Please switch to $this->entityTypeManager

@jigish, Thank you. The patch looks good to me now.

@jigishaddweb

Thank you for the update. Could you please re-create the patch by following up newly introduced our own method processOperationStatus() for addMessage() and the log at #3133275-25?

@jigishaddweb

Thank you for the update. Could you please add \Drupal::logger() at the following occurrences?

+++ b/modules/cloud_service_providers/openstack/openstack.module

+  \Drupal::messenger()->addStatus(t('The @type @label has been deleted.', [
+    '@type'  => $server_template->getEntityType()->getSingularLabel(),
+    '@label' => $server_template->label(),
+  ]));

  \Drupal::logger('openstack')->notice('@type: deleted @label.', [
    '@type' => $server_template->getEntityType()->getSingularLabel(),
    '@label' => $server_template->label(),
  ]);

+++ b/modules/cloud_service_providers/openstack/openstack.module

+  \Drupal::messenger()->addStatus(
+    t('The @type %label has been created.', [
+      '@type' => $server_template->getEntityType()->getSingularLabel(),
+      '%label' => $server_template->toLink($server_template->label())->toString(),
+    ])
+  );

  \Drupal::logger('openstack')->notice('@type: created %label.', [
    '@type' => $server_template->bundle(),
    '%label' => $server_template->label(),
    'link' => $server_template->toLink(t('View'))->toString(),
  ]);

@jigishaddweb

Thank you for the refactoring. It looks good now. I'll merge the patch to 8.x-1.x and 8.x-2.x and close this issue as Fixed.