destroy: add destroyer code to destroy Azure clusters #1771
Conversation
openshift-ci-robot
added
size/XXL
abhinavdahiya
force-pushed
the
azure_destroy
branch
from
e0e3f86 to
80fb6ca
Compare
|
/retest |
|
Why does nobody support tagging DNS records?? Sigh... |
pkg/destroy/azure/azure.go
Outdated
There was a problem hiding this comment.
Why the local variable vs. o.resourceGroupsClient = resources.NewGroupsGroupClient(o.SubscriptionID)? Are we casting to an Interface or some such and loosing access to the Authorizer property.
There was a problem hiding this comment.
Azure sdk doesn't have a way to initialize a client with auth.
So the
client := NewClient()
client.Authorizer = auth
o.client = client
seems more appropriate of create a new client, and use that to initialize/assign our client.
Do you have strongly about not using the local var? If not personally current make more sense to me and i would like to keep it as is...?
There was a problem hiding this comment.
I was thinking:
o.client = NewClient()
o.client.Authorizer = authbut don't feel strongly either way.
There was a problem hiding this comment.
If it functions properly, I think Trevor's recommendation is more concise.
|
/test e2e-openstack |
abhinavdahiya
force-pushed
the
azure_destroy
branch
from
80fb6ca to
5771e49
Compare
This bump is required to include `github.com/Azure/azure-sdk-for-go/services/preview/dns/mgmt/2018-03-01-preview/dns/`
pkg/destroy/azure/azure.go
Outdated
There was a problem hiding this comment.
getSharedDNSZone -> getSharedDNSZones?
pkg/destroy/azure/azure.go
Outdated
There was a problem hiding this comment.
Do we want our own type, or can we use dns.Zone directly?
There was a problem hiding this comment.
[dns.Zone][https://godoc.org/github.com/Azure/azure-sdk-for-go/services/preview/dns/mgmt/2018-03-01-preview/dns#Zone] has pointers for fields, doesn't have the resource group (that needs to be inferred from ID).
This small spare struct stores the details in easy to use way.
pkg/destroy/azure/azure.go
Outdated
There was a problem hiding this comment.
This will fail instead of endlessly retrying (which is what the AWS destroyer does now). Sometimes (e.g. stuck resource), failing is good. Sometimes (e.g. network hiccup), retrying is good. Do we have a design preference? Or is this behavior going to be a per-platform choice?
There was a problem hiding this comment.
Azure only makes 2 calls, delete public records and another is delete resource group. Also the underlying impl has retries for retryable errors..
So I think this is good start. and We can make it retryable on our end when we think that will become necessary.
|
/test e2e-openstack |
abhinavdahiya
force-pushed
the
azure_destroy
branch
from
5771e49 to
3ef6475
Compare
also fixed the commit message. |
abhinavdahiya
force-pushed
the
azure_destroy
branch
from
3ef6475 to
1bd15ad
Compare
It uses the `infra-id` from clustermetadata to extract the ResourceGroup
The destroy code performs 2 operations:
1) Remove dns records from the shared public DNS zone
- Find the private dns zones in Resource Group
- Find all public zones that can be parent of the private zone, and remove all records that match the `complete` dns record in the private zone
- Use the records in the private zone to remove the corresponding records from the shared dns zone.
2) Delete the Resource Group
Azure automatically deletes all the resources in the Resource Group
abhinavdahiya
force-pushed
the
azure_destroy
branch
from
1bd15ad to
9a57507
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, jstuever The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@abhinavdahiya: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
7 participants
It uses the
infra-idfrom clustermetadata to extract the ResourceGroupThe destroy code performs 2 operations:
Remove dns records from the shared public DNS zone
completedns record in the private zoneDelete the Resource Group
Azure automatically deletes all the resources in the Resource Group
/cc @wking @jstuever