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: deploy vsi only to the vsi-zone-* subnets for null value #661

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Aashiq-J
Copy link
Member

@Aashiq-J Aashiq-J commented Dec 19, 2023

Description

Issue: https://github.ibm.com/GoldenEye/issues/issues/6951

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@Aashiq-J
Copy link
Member Author

/run pipeline

@ocofaigh
Copy link
Member

@Aashiq-J I expect if we had an upgrade test for the "Existing VPC" VSI extension DA this change would actually fail the upgrade test because it will attempt to destroy VSIs from subnets that did not start with <prefix>-vsi-zone-*. Can we add an upgrade test and ensure that is the case? We also need to double check any documentation we may have published stating the DA deploy VSIs to all subnets, and maybe rename it to say all VSI subnets.

@Aashiq-J
Copy link
Member Author

/run pipeline

@Aashiq-J Aashiq-J marked this pull request as ready for review December 20, 2023 03:42
@Aashiq-J
Copy link
Member Author

@ocofaigh ,
Added upgrade test and it is failing as expected.

It is trying to recreate the vsi in the vsi-subnets and delete the ones in the other subnets.
Screenshot 2023-12-20 at 11 39 34 AM
Screenshot 2023-12-20 at 11 39 58 AM

@@ -598,3 +598,116 @@ func TestRunVsiExtention(t *testing.T) {
logger.Log(t, "END: Destroy (existing resources)")
}
}

func TestRunUpgradeVsiExtention(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Is alot of the code here duplicated from the TestRunVsiExtention test? Can we maybe remove duplication by adding the common code to a function and having each test call that function?

@@ -1,6 +1,6 @@
# Add a VSI to a landing zone VPC

This architecture creates virtual server instances (VSI) in some or all of the subnets of one VPC of an existing landing zone deployable architecture. To create VSIs in multiple VPCs, deploy the extension once for each VPC.
This architecture creates virtual server instances (VSI) in some or all of the VSI subnets of one VPC of an existing landing zone deployable architecture. To create VSIs in multiple VPCs, deploy the extension once for each VPC.
Copy link
Member

@ocofaigh ocofaigh Dec 20, 2023

Choose a reason for hiding this comment

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

Same change needs to be made in ibm_catalog.json here

@ocofaigh
Copy link
Member

@Aashiq-J I wasn't expecting any new creates to be seen in the upgrade test? I thought current code was creating VSI in every subnet, and then the new code would delete the VSIs in the non VSI subnets. Why is code trying to create new VSIs?

@Aashiq-J
Copy link
Member Author

@ocofaigh ,
When updating the logic to deploy only on VSI subnet, the map structure changes which terraform sees it as a new different map entries and recreates the cluster.

@Aashiq-J
Copy link
Member Author

/run pipeline

@ocofaigh
Copy link
Member

Need to update to v4 of VSI module, but can't do that until Projects provider official state migration step

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.

None yet

2 participants