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

feat: additional load balancer config #536

Merged
merged 19 commits into from
Sep 27, 2023
Merged

feat: additional load balancer config #536

merged 19 commits into from
Sep 27, 2023

Conversation

rajatagarwal-ibm
Copy link
Member

Description

The landing-zone-vsi module by default created an Application load balancer with the primary network interface subnet. The actual resource ibm_is_lb allows to configure these variables using profile and subnets. This PR adds them as optional config.

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.

Merge actions for mergers

  • When merging, use a relevant conventional commit message that is based on the PR contents and any release notes provided by the PR author. The commit message determines whether a new version of the module is needed, and if so, which semver increment to use (major, minor, or patch).
  • Merge by using "Squash and merge".

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

variables.tf Outdated Show resolved Hide resolved
@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

@ocofaigh
Copy link
Member

@rajatagarwal-ibm It seems we have no examples or tests that actually test adding load balancers to VSIs. Can we add an example (or maybe just update the fscloud example) to do this? That way consumers can see how to use the variable, and we test it as part of our tests

@rajatagarwal-ibm
Copy link
Member Author

@ocofaigh sure, let me update fscloud example to consume LB

@ocofaigh
Copy link
Member

@rajatagarwal-ibm maybe actually it makes sense to create a new example. Perhaps a custom example where we provision the module with extra customisations such as load balancer and additional data volumes. Check if we have other gaps too.
I added additional data volumes to the fscloud example to cover that gap a while ago, but perhaps there are more

@rajatagarwal-ibm
Copy link
Member Author

@ocofaigh sure, other than Loadbalancer, there are "Secondary Interface Variables" which are not used in the fscloud example. Perhaps I can add Load Balancer and "Secondary Interface Variables" to the new example. Should I also remove "block_storage_volumes" from the FSCloud example to the new custom example?

@ocofaigh
Copy link
Member

I think you can leave it in the fscloud example, since it would test being encrypted using HPCS.

@rajatagarwal-ibm
Copy link
Member Author

As discussed with @ocofaigh, I will be creating a new example "complete" which creates LB and Secondary Interface. I will also use Key Protect for encryption in this example which will enable parallel execution of FSCloud tests(uses HPCS) with the complete tests.

Along with that, I will also update tests:

  • Add complete test to the PR test
  • Replace FSCloud Upgrade test with complete Upgrade test in the PR Test
  • Run FSCloud standard test in the PR test (no change needed).

@ocofaigh
Copy link
Member

@rajatagarwal-ibm In #539 Steve has added placement group support and updated the basic example to use it. I think it should be moved the complete example you are going to create

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

Left some comments. Can we also add Key Protect encryption to the complete example, as there isnt any example doing that since fscloud example does HPCS

@@ -0,0 +1,10 @@
# Complete Example
Copy link
Member

Choose a reason for hiding this comment

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

Complete Example using a placement group, attaching a load balancer, and adding additional data volumes

Copy link
Member

Choose a reason for hiding this comment

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

@rajatagarwal-ibm The title was not updated here?

Copy link
Member

Choose a reason for hiding this comment

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

Ah its below - it needs to be in title so its exposed in main readme

examples/complete/main.tf Show resolved Hide resolved
- A new public SSH key if one is not passed in.
- A new VPC with 3 subnets
- A new placement group
- A VSI in each subnet
Copy link
Member

Choose a reason for hiding this comment

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

placed in the placement group

- A new VPC with 3 subnets
- A new placement group
- A VSI in each subnet
- A new Application Load Balancer
Copy link
Member

Choose a reason for hiding this comment

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

A new Application Load Balancer to balance traffic between all virtual servers that are created by this example

examples/complete/main.tf Show resolved Hide resolved
)
})
default = null
}
Copy link
Member

Choose a reason for hiding this comment

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

I would just remove this from the example if we are not passing any value for it

description = "User data to initialize VSI deployment"
type = string
default = null
}
Copy link
Member

Choose a reason for hiding this comment

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

I would just remove this from the example if we are not passing any value for it

description = "CRN of boot volume encryption key"
type = string
default = null
}
Copy link
Member

Choose a reason for hiding this comment

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

remove this

description = "Number of VSI instances for each subnet"
type = number
default = 1
}
Copy link
Member

Choose a reason for hiding this comment

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

can we just pass this in directly to in the example main.tf - no need to expose as a variable in the example

type = string
description = "Name for VPC"
default = "vpc"
}
Copy link
Member

Choose a reason for hiding this comment

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

remove this from here - use the prefix variable for vpc name

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

@rajatagarwal-ibm
Copy link
Member Author

/run pipeline

@ocofaigh ocofaigh self-requested a review September 26, 2023 15:05
@ocofaigh
Copy link
Member

/run pipeline

@ocofaigh ocofaigh merged commit 2ee0f7c into main Sep 27, 2023
2 checks passed
@ocofaigh ocofaigh deleted the configurable-lb branch September 27, 2023 19:04
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 2.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants