Support from Acquia helps fund testing for Drupal Acquia logo

Comments

liuchanggang created an issue. See original summary.

liuchanggang’s picture

Issue summary: View changes
liuchanggang’s picture

Status: Active » Needs review
FileSize
7.06 KB

This patch needs rebase when 3171199 is merged.
Thanks

yas’s picture

Status: Needs review » Needs work

@liuchanggang

Thank you for the refactoring. Could you please re-create the patch due to the recent merge?

liuchanggang’s picture

Status: Needs work » Needs review
FileSize
6.99 KB

@yas,

I have rebased and retested it. It is ready to merge.

Thanks

Chang

liuchanggang’s picture

Changed status updating function.

yas’s picture

Status: Needs review » Needs work

@liuchanggang

Thank you for the update. I tested the patch but it didn't work as we expected.

  1. Create a VPC Peering Connection from Cloud Orchestrator
  2. Delete the VPC Peering Connection from AWS management console
  3. Try to edit the VPC Peering Connection from Cloud Orchestrator → It should go to the listing page, but it'll show the edit page. I could edit a tag. FYI, The edit page shows Status Code: deleted
  4. Try to delete the VPC Peering Connection from Cloud Orchestrator → The "deleted" status message is shown but it is not actually deleted.

Could you please double-check the behaviors accordingly?

liuchanggang’s picture

@yas,

Question: Do we want to show the items of DELETED status?

By now, if we click the Refresh button, Deleted VPC Peering Connections shows up on list page and same for Failed ones.
By AWS rule, failed VPC Peering Connections stay for 48 hours before disappearing. Deleted items have other rules

Do we want to change this behavior?

Thanks

yas’s picture

@liuchanggang

Could you please give us the test scenario(s) and the expected result(s)?

Thanks

liuchanggang’s picture

Status: Needs work » Needs review
FileSize
9.14 KB

@yas,

1, Created a few of AWS VPC peering connections, they could have Failed or Pending status.
2, Delete those with pending status. They would change to Deleted status.
3, Click Refresh button, we can see those failed/deleted VPC connections on list page. They should disappear in 24 hours from API output.
4, During the first 24 hours, when users click the Edit/Delete button/link, it will redirect to list page and show warning message

"The VPC Peering Connection Name has deleted status. It can't be edited or deleted."
"The VPC Peering Connection Name has failed status. It can't be edited or deleted."

5, after 24 hours, they are not in API output any longer, when we click Edit/Delete, entity would be removed and redirect to list page showing warning message

"The VPC Peering Connection Name has already been deleted."

liuchanggang’s picture

@yas

I updated the warning messages to be

''The VPC Peering Connection test1 has failed status. During this state, it cannot be accepted, rejected, or deleted. The failed VPC peering connection remains visible to the requester for 2 hours."

"The VPC Peering Connection test2 has deleted status. The VPC peering connection remains visible to the party that deleted it for 2 hours, and visible to the other party for 2 days. If the VPC peering connection was created within the same AWS account, the deleted request remains visible for 2 hours."

yas’s picture

@liuchanggang

Thank you for the update. I tested the patch and it gives an error. That's fine as we expected.

Only local images are allowed.

And what about the behavior when we click the Accept button? It shows Access denied. We expect the same error message as the Edit or Delete button.

accept_1.png

liuchanggang’s picture

Status: Needs work » Needs review
FileSize
10.81 KB

@yas,

I updated VpcPeeringConnectionAcceptForm to have same update logic.

Thanks

yas’s picture

Status: Needs review » Needs work

@liuchanggang

Thank you for the update. I tested the patch but the result was the same as my previous comment at #12. Could you please double-check it?

liuchanggang’s picture

Status: Needs work » Needs review
FileSize
12.77 KB

@yas,

I see the issue is that even after status is updated to Deleted, we still see the Accept button and when clicking it, we see denied errors.
Accept button shouldn't show on page when status is deleted. I resolved the problem by adding
self::clearCacheValue();

I have updated the patch.

Thanks

baldwinlouie’s picture

Status: Needs review » Needs work
+++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
@@ -1547,7 +1551,7 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
+    self::clearCacheValue();

@@ -1905,6 +1909,7 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
+    self::clearCacheValue();

@@ -2019,6 +2024,7 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
+    self::clearCacheValue();

@@ -2125,6 +2131,7 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
+    self::clearCacheValue();

@@ -2232,6 +2239,7 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
+    self::clearCacheValue();

@@ -2508,6 +2516,7 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
+    self::clearCacheValue();

I'm worried about self::clearCacheValue(). For example, updateAllResourceList also calls clearCacheValue.

We can get into a scenario, where the cache is cleared multiple times during one operation. When that happens, the site will slow down.

Would it make more sense to put it in updateSingleEntity()?

That way, we know it won't be called multiple times when updateAllResourceList is called

liuchanggang’s picture

Status: Needs work » Needs review
FileSize
10.87 KB

@baldwinlouie

Thanks for insight. I reviewed all those update entity functions and removed unnecessary clearCacheValue().

yas’s picture

@liuchanggang

Thank you for the update. I tested the patch and it looks working as we expected.

@baldwinlouie

What do you think?

baldwinlouie’s picture

Status: Needs review » Reviewed & tested by the community

@liuchanggang, Thank you for the updated patch. It looks good to me now.

yas’s picture

@baldwinlouie

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

yas’s picture

Status: Reviewed & tested by the community » Fixed

  • yas committed b54b55d on 8.x-1.x authored by liuchanggang
    Issue #3171393 by liuchanggang, yas, baldwinlouie: Refresh the resource...

  • yas committed 88b0919 on 8.x-2.x authored by liuchanggang
    Issue #3171393 by liuchanggang, yas, baldwinlouie: Refresh the resource...

  • yas committed 86cb9d2 on 3.x authored by liuchanggang
    Issue #3171393 by liuchanggang, yas, baldwinlouie: Refresh the resource...

Status: Fixed » Closed (fixed)

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