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.

Sign up for GitHub

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

Jump to bottom

Azure Stack Bootstrap Destroy Bug #5443

Merged

openshift-merge-robot merged 1 commit into openshift:master from patrickdillon:azurestack-destroy-bug

Dec 11, 2021

Contributor

patrickdillon commented Dec 2, 2021

There was a bug where the wrong platform name was being set for Azure Stack Hub bootstrap destroy.

openshift-ci bot requested review from rna-afk and squidboylan

December 2, 2021 17:09

Contributor

rna-afk commented Dec 2, 2021 •

edited

/lgtm
I'm now wondering how my bootstrap destroy code fix was working :(

openshift-ci bot assigned rna-afk

openshift-ci bot added the lgtm label

squidboylan reviewed

View reviewed changes

pkg/destroy/bootstrap/bootstrap.go Show resolved Hide resolved

patrickdillon force-pushed the azurestack-destroy-bug branch from d75a2db to f450524 Compare

December 2, 2021 18:53

openshift-ci bot removed the lgtm label

Contributor

squidboylan commented Dec 2, 2021

/lgtm

patrickdillon mentioned this pull request

azurestack: Fix gather bootstrap for azurestack #5382

Merged

openshift-ci bot assigned squidboylan

openshift-ci bot added the lgtm label

Contributor Author

patrickdillon commented Dec 2, 2021

Doh. I'm deleting my previous comment about this not working. My test was wrong. Hopefully you missed my incorrect comment.

staebler reviewed

View reviewed changes

pkg/destroy/bootstrap/bootstrap.go Outdated

               	// tfvars file is still terraform.azure.auto.tfvars.json in the case of Azure Stack.
               	if platform == typesazure.Name && metadata.Azure.CloudName == typesazure.StackCloud {
-              		platform = "azurestack"
+              		platform = typesazure.StackCloud.Name()

Contributor

staebler Dec 3, 2021 •

edited

I don't think that this is the best solution. We put the tf files under data/data/azurestack, so we call the platform "azurestack" in the stages.

installer/pkg/terraform/stages/azure/stages.go

Lines 20 to 22 in ec1c830

    
           stages.NewStage(azurestack, "vnet"), 
        
           stages.NewStage(azurestack, "bootstrap", stages.WithNormalDestroy()), 
        
           stages.NewStage(azurestack, "cluster"),

So I think we should call the platform "azurestack" here as well, for consistency. We should instead change the following line to "azurestack".

installer/pkg/terraform/stages/platform/stages.go

Line 31 in ec1c830

case azuretypes.StackCloud.Name():

Contributor Author

patrickdillon Dec 8, 2021

@staebler, your point makes sense. I ended up going a bit further and eliminated the use of the "azurestack" magic string. IMHO, it is better to nip this in the bud and clean it up now (mess I introduced); but if you think this is overkill let me know.

patrickdillon force-pushed the azurestack-destroy-bug branch from f450524 to 194b9c2 Compare

December 8, 2021 21:32

openshift-ci bot removed the lgtm label

patrickdillon force-pushed the azurestack-destroy-bug branch 2 times, most recently from ea768d5 to 2e09f53 Compare

December 9, 2021 15:43

Contributor Author

patrickdillon commented Dec 9, 2021

/retest

staebler reviewed

View reviewed changes

pkg/asset/cluster/cluster.go Outdated Show resolved Hide resolved

pkg/terraform/stages/azure/stages.go Outdated Show resolved Hide resolved

pkg/types/azure/doc.go Outdated Show resolved Hide resolved

pkg/destroy/bootstrap/bootstrap.go

		@@ -41,7 +41,7 @@ func Destroy(dir string) (err error) {
		// Set platform to azurestack after setting tfPlatformVarsFileName, because the
		// tfvars file is still terraform.azure.auto.tfvars.json in the case of Azure Stack.

Contributor

staebler Dec 10, 2021

Not for this PR, but we should consider whether there is any benefit in using a different name for the tfvars file for each platform. We never have tfvars for multiple platforms simultaneously, so we could just always use the name terraform.platform.auto.tfvars.json for the file.

Contributor

staebler Dec 13, 2021


          Azure (Stack): Make azurestack platform constant

a237ef3

Removes the use of the "azurestack" magic string and replaces it
with a constant in the Azure Stack type package. Revise the generation
of both Azure and Azure Stack stages accordingly.

patrickdillon force-pushed the azurestack-destroy-bug branch from 2e09f53 to a237ef3 Compare

December 10, 2021 14:48

patrickdillon requested review from staebler and squidboylan

December 10, 2021 14:49

Contributor Author

patrickdillon commented Dec 10, 2021

Updated. Ready for review.

staebler approved these changes

View reviewed changes

Contributor

staebler left a comment

/lgtm
/approve

openshift-ci bot assigned staebler

openshift-ci bot added the lgtm label

Contributor

openshift-ci bot commented Dec 10, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: staebler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [staebler]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the approved label

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

18 similar comments

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 10, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 11, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 11, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 11, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 11, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 11, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor

openshift-bot commented Dec 11, 2021

/retest-required

Please review the full test history for this PR and help us cut down flakes.

Contributor Author

patrickdillon commented Dec 11, 2021

/skip

openshift-merge-robot merged commit 3c98b1f into openshift:master

Contributor

openshift-ci bot commented Dec 11, 2021

@patrickdillon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-single-node	`a237ef3`	link	false	`/test e2e-aws-single-node`
ci/prow/e2e-aws-fips	`a237ef3`	link	false	`/test e2e-aws-fips`
ci/prow/e2e-crc	`a237ef3`	link	false	`/test e2e-crc`
ci/prow/e2e-aws-workers-rhel8	`a237ef3`	link	false	`/test e2e-aws-workers-rhel8`
ci/prow/e2e-libvirt	`a237ef3`	link	false	`/test e2e-libvirt`

Full PR test history. Your PR dashboard.

Instructions 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.

staebler mentioned this pull request

asset/tfvars: use generic name for platform tfvars file #5477

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment