Skip to content

added google_service_networking_connection deletion to 5.0 guide#9009

Merged
zli82016 merged 2 commits into
mainfrom
google_service_networking_connection-delete-guide
Sep 21, 2023
Merged

added google_service_networking_connection deletion to 5.0 guide#9009
zli82016 merged 2 commits into
mainfrom
google_service_networking_connection-delete-guide

Conversation

@zli82016

@zli82016 zli82016 commented Sep 19, 2023

Copy link
Copy Markdown
Member

The change is in PR #8904

Release Note Template for Downstream PRs (will be copied)

docs: added `google_service_networking_connection` deletion to 5.0 guide

@modular-magician

Copy link
Copy Markdown
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 4 insertions(+))
Terraform Beta: Diff ( 1 file changed, 4 insertions(+))


### `deleteConnection` method is used to delete the resource

`google_service_networking_connection` now uses `deleteConnection` method instead of `removePeering` method during the deletion step. Previously, `removePeering` method was used because `deleteConnection` method was unavailable. Sometimes a private connection cannot be deleted immediately after the resource using that connectioin is deleted, and you have to delete the private connection manually after a waiting period.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`google_service_networking_connection` now uses `deleteConnection` method instead of `removePeering` method during the deletion step. Previously, `removePeering` method was used because `deleteConnection` method was unavailable. Sometimes a private connection cannot be deleted immediately after the resource using that connectioin is deleted, and you have to delete the private connection manually after a waiting period.
`google_service_networking_connection` now uses `deleteConnection` API method instead of `removePeering` method during the deletion step. Previously, `removePeering` method was used because `deleteConnection` method was unavailable. In some cases a private connection cannot be deleted immediately after the resource using that connection is deleted, and users may have to delete the private connection manually after a waiting period.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes a private connection cannot be deleted immediately after the resource using that connection is deleted, and you have to delete the private connection manually after a waiting period.

Are we expecting deletion to return an error often? If so, that seems very annoying for users. Should there be a retry?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retry is a good idea, but there is a corner case for sql database instances. It returns an error that the connection is still be used by the service. After the action to delete a sql instance is made, it needs to wait for four days to delete the resource in deed, and then the connection can be deleted. It seems that the waiting period is too long to have a retry.

@c2thorn c2thorn Sep 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Looking at the code, I see in this case we silently fail instead. That is not as bad.
Definitely 4 days is too long for a retry


`google_service_networking_connection` now uses the Create endpoint instead of the Patch endpoint during the creation step. Previously, Patch was used as a workaround for an issue that has since been resolved.

### `deleteConnection` method is used to delete the resource

This comment was marked as outdated.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code, I see we actually do abandon if the delete fails, so this suggestion is not valid.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### `deleteConnection` method is used to delete the resource
### `deleteConnection` API method is attempted to delete the resource

maybe?

@zli82016 zli82016 Sep 20, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code, I see we actually do abandon if the delete fails, so this suggestion is not valid.

I am not sure if you checked the latest version of PR 8904. It does not abandon the resource after deletion failed and just logged the error.

I think about it again. It looks like I should return the error instead of catch it and log it? But I am not sure if it will affect the users or not to return the error.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misread and thought it was abandon, thanks for the correction. In the PR now, what happens after terraform destroy in that scenario? Just logs and keeps the state the same?

The previous functionality was to remove peering and then abandon the resource without deleting right?

@zli82016 zli82016 Sep 20, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point.

Right, the previous implementation calls removePeering method. It removed peering, then abandoned the resource without deleting. The connection is not deleted actually, which caused the issue that the tenant projects/resources reach to the limit. https://buganizer.corp.google.com/issues/248298764

In the PR 8904, I change it to call the method deleteConnection, catch and log the error. After terraform destroy, the connection disappears in the state. That is not expected.

I tested another option to not catch and return error if deleteConnection fails. terraform destroy fails with the error and the connection stays in the state. It looks like I should go with this option.

@zli82016 zli82016 changed the title Add 5.0 guide to delete google_service_networking_connection added google_service_networking_connection deletion to 5.0 guide Sep 20, 2023
@modular-magician

Copy link
Copy Markdown
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 1 file changed, 4 insertions(+))
Terraform Beta: Diff ( 1 file changed, 4 insertions(+))

@zli82016 zli82016 requested a review from c2thorn September 20, 2023 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants