Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NotFound error in DNS isn't handled properly #3739

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

bitoku
Copy link
Collaborator

@bitoku bitoku commented Jul 30, 2024

Which issue this PR addresses:

Fixes N/A

What this PR does / why we need it:

In a part of other task, I found it failed to delete a cluster when dns records are already deleted.
This was caused by the update of the sdk to track2.
Since track2, autorest has been deprecated, we needed to change it to ResponseError.

cf. https://github.com/Azure/go-autorest
https://github.com/Azure/azure-sdk-for-go/blob/e302bb60ef995455b0eaabf590a75c8396e32834/sdk/azcore/internal/exported/response_error.go#L112

I found https://pkg.go.dev/github.com/Azure/azure-sdk-for-go-extensions/pkg/errors#IsNotFoundErr this repository but we can't use it, because the error we get is NotFound, but it checks if the error is ResourceNotFound.
Probably this discrepancy might come from the Azure API server, but anyway we can't use it.

Test plan for issue:

Delete a dev cluster with dns records already deleted.
Unit test (it couldn't find the error when we updated the sdk though)

Is there any documentation that needs to be updated for this PR?

N/A

How do you know this will function as expected in production?

Delete a cluster with dns records already deleted.

Copy link
Collaborator

@mociarain mociarain left a comment

Choose a reason for hiding this comment

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

Small change in syntax but LGTM

pkg/util/dns/dns.go Outdated Show resolved Hide resolved
@bitoku bitoku force-pushed the atokubi/dns-deletion-bugfix branch from 983cb2e to 0aed6cf Compare July 30, 2024 12:15
@bitoku
Copy link
Collaborator Author

bitoku commented Jul 30, 2024

I force-pushed this time again, but you can compare it with the previous one from Compare button right above.

@bitoku bitoku added the hold Hold label Jul 30, 2024
@bitoku
Copy link
Collaborator Author

bitoku commented Jul 30, 2024

I found another problem in DNS manager. I'll make it hold to fix all of them.

@bitoku bitoku changed the title Fix dns deletion error when dns is already deleted. Fix NotFound error in DNS isn't handled properly Jul 30, 2024
@bitoku bitoku force-pushed the atokubi/dns-deletion-bugfix branch from 0aed6cf to 2143cb8 Compare July 30, 2024 13:55
@bitoku
Copy link
Collaborator Author

bitoku commented Jul 30, 2024

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@mociarain
Copy link
Collaborator

/azp run ci, e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@bitoku bitoku removed the hold Hold label Jul 30, 2024
Copy link

@AldoFusterTurpin AldoFusterTurpin left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@mociarain mociarain merged commit db4508d into master Jul 30, 2024
21 checks passed
@mociarain mociarain deleted the atokubi/dns-deletion-bugfix branch July 30, 2024 16:12
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