Problem/Motivation

  • Currently, there is a limit to the number of items a queue can have. However, if there are lots of cloud service providers, this number is easily hit. Subsequently, cloud service providers won't be updated.

Issue fork cloud-3267036

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

baldwinlouie created an issue. See original summary.

baldwinlouie’s picture

Status: Active » Needs review

@yas, I've added support for creating multiple queues using a deriver. I've only added the deriver to the AWS, K8s, Openstack, Terraform, and VMware. I don't think this is needed for cloud_cluster or cloud_cluster_worker.

baldwinlouie’s picture

Status: Needs review » Needs work

@yas, setting back to needs work. I need to make queue changes to K8sCloudStorePlugin.php as well. Those were caught during Unit Testing.

baldwinlouie’s picture

Status: Needs work » Needs review
yas’s picture

Title: Refactor queue to handle more queue entries. » Refactor queue to handle more queue entries
Issue summary: View changes
Status: Needs review » Needs work

@baldwinlouie

Thank you for the update and refactoring. I posted my comment above. Thanks!

baldwinlouie’s picture

Status: Needs work » Needs review
yas’s picture

Status: Needs review » Needs work

@baldwinlouie

Thank you for the update. I posted my comment. All-in-all, it looks good to me.

@xiaohua-guan

What do you think?

xiaohua guan’s picture

@yas

It looks good to me. It's a great commit!

yas’s picture

@xiaohua-guan

Thank you for your review.

@baldwinlouie

I found that we need to refactor the following files for queue ID change:

  • .tugboat/config.yml
  • </code>cfn/nested/ec2/ami/template.yaml

Can you include the refactor for cron job for those files? Thanks!

yas’s picture

@baldwinlouie @xiaohua-guan

I posted my suggested refactoring. Could you please review the logic?

Also, the default value of e.g. aws_cloud_queue_limit can be 1 based on my local testing result.

Thanks!

baldwinlouie’s picture

Status: Needs work » Needs review

@yas, please review the refactoring. I've extracted the logic that adds items to the queue into two helper methods.

getResourceQueueName and updateResourceQueue.

I've also updated the following

  • .tugboat/config.yml
  • cfn/nested/ec2/ami/template.yaml
yas’s picture

Status: Needs review » Needs work

@baldwinlouie

Thank you for the update. I reviewed the patch and posted my comments above. Also, could you please change the default value 50 to 1 for <module_name>_queue_limit? Please check: https://git.drupalcode.org/search?search=_queue_limit%3A+50&group_id=2&p... ?

Thanks!

baldwinlouie’s picture

Status: Needs work » Needs review

@yas, all the requested changes have been made.

yas’s picture

Status: Needs review » Needs work

@baldwinlouie

Can you refactor the rest of code at AwsCloudUpdateResourcesQueueWorker? Thanks!

baldwinlouie’s picture

Status: Needs work » Needs review

@yas, please review the updated.

yas’s picture

@baldwinlouie

Thank you for the update. It looks all good to me.

@xiaohua-guan

Could you please review the patch?

NOTE:

I tested the queue limit by running watch drush queue-list and found it guarantees that each queue item will be surely processed when we specify queue limit 1. It will realize one channel to process all queue item since the queue items will increase only when it becomes zero with our new logic, which means all entities are surely updated. It will create something like a single channel literally like FIFO, that we desire. I thought we didn't have to put multiple queue items for each resource at the same time. Each resource needs to be updated once enough during the update cycle.

baldwinlouie’s picture

@yas I've added additional functionality to this patch for queue refactoring.

1. Added a new drush command drush cloud:queue:cleanup. Example usage of the command
drush cloud:queue:cleanup --hours=5 --queue_names=queue1,queue2 . Delete items from a comma delimited list of queues that are older than 5 hours. This lets you delete from any queue of your choosing, even queues created by other modules.
drush cloud:queue:cleanup. This is the default usage and will delete all queue items older than 1 hour. It only deletes from queues created by cloud service providers. Queues created by other modules are ignored.

2. Added try/catch around all queue processing. This way, if there is an exception, that particular queue item will be booted out of the queue. By doing so, we do not have lingering queue items that can potentially error out.

3. When deleting a cloud service provider, delete all their queue items. This will help clean up the table a bit. I've done this in hook_cloud_config_predelete(). Doing so won't cause any CloudConfigPluginException errors to be thrown mistaken.

4. In the processItem of the Queue Worker plugins, I've abstracted the common function that checks if a service method exists. I'm also checking to see if setting the CloudContext will cause any CloudConfigPluginException errors.

Finally, I did not hide the configuration for *_queue_limit variable yet. We haven't really testing if count=1 is truly desired yet. If we want to do this, we can create another issue.

xiaohua guan’s picture

Status: Needs review » Reviewed & tested by the community

@yas

It looks good to me. Thanks.

yas’s picture

@xiaohaua-guan

Thank you for your review.

@baldwinlouie

Thank you for the update. I tested this great patch and looks working well. I'll merge the patch to 4.x and 5.x, and close this issue as Fixed.

  • yas committed 805d285 on 5.x authored by baldwinlouie
    Issue #3267036 by baldwinlouie, yas, Xiaohua Guan: Refactor queue to...

yas’s picture

Status: Reviewed & tested by the community » Fixed

  • yas committed 62675d9 on 4.x authored by baldwinlouie
    Issue #3267036 by baldwinlouie, yas, Xiaohua Guan: Refactor queue to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.