Deployer Node Changes Specific to LSF Cluster Creation #199

jayeshh123 · 2025-02-13T16:06:23Z

Description

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.

Signed-off-by: Jayesh-Kumar3 <[email protected]>

bksagar · 2025-02-17T16:20:20Z

locals.tf

  # dependency: landing_zone -> deployer
-  vpc_id                     = var.vpc == null ? one(module.landing_zone.vpc_id) : var.vpc
+  vpc_id                     = var.vpc == null ? one(module.landing_zone.vpc_id) : var.vpc_id
+  vpc                        = var.vpc == null ? one(module.landing_zone.vpc_name) : var.vpc


@jayeshh123 As discussed please look at the requirement to use this vpc block for the local, as we are not using this at the code level

@manirtm Please change the name as vpc_name

bksagar · 2025-02-17T16:31:07Z

locals.tf

+  bastion_public_key_content  = module.deployer.bastion_public_key_content
+  bastion_private_key_content = module.deployer.bastion_private_key_content
+
+  deployer_hostname = var.enable_bastion ? flatten(module.deployer.deployer_vsi_data[*].list)[0].name : ""


@jayeshh123 As discussed, we need to check with PC about the support for existing bastion feature. Once that is done, this logic needs to be enhanced

bksagar · 2025-02-17T16:39:37Z

locals.tf

+  storage_subnets  = var.vpc != null && var.storage_subnets != null ? local.existing_storage_subnets : module.landing_zone.storage_subnets
+  protocol_subnets = var.vpc != null && var.protocol_subnets != null ? local.existing_protocol_subnets : module.landing_zone.protocol_subnets
+
+  storage_subnet  = [for subnet in local.storage_subnets : subnet.name]


@jayeshh123 As discussed over the review, please try to use the name that is already fetched from the data source blocks instead of looping to the for loop again.

bksagar · 2025-02-17T16:43:52Z

locals.tf

  # dependency: landing_zone_vsi -> file-share
-  compute_subnet_id         = local.compute_subnets[0].id
-  compute_security_group_id = module.landing_zone_vsi.compute_sg_id
+  compute_subnet_id         = var.vpc == null && var.compute_subnets == null ? local.compute_subnets[0].id : [for subnet in data.ibm_is_subnet.existing_compute_subnets : subnet.id][0]


@jayeshh123 Please re-look at this logic and probably use the locals that are already been called once

bksagar · 2025-02-17T16:47:08Z

locals.tf

  vpc_crn           = var.vpc == null ? one(module.landing_zone.vpc_crn) : one(data.ibm_is_vpc.itself[*].crn)
  # TODO: Fix existing subnet logic
  #subnets_crn       = var.vpc == null ? module.landing_zone.subnets_crn : ###
+  existing_compute_subnet_crns  = [for subnet in data.ibm_is_subnet.existing_compute_subnets : subnet.crn]


@jayeshh123 Please look at this logic and try to use the same local conditions

bksagar · 2025-02-17T16:48:12Z

locals.tf

  # dependency: landing_zone_vsi -> file-share
 }

+data "external" "get_hostname" {


@jayeshh123 As discussed please remove the code and this is not required

bksagar · 2025-02-17T16:54:33Z

locals.tf

 locals {
  # dependency: dns -> dns-records
-  dns_instance_id = module.dns.dns_instance_id
+  dns_instance_id = var.enable_deployer ? "" : module.dns[0].dns_instance_id


@jayeshh123 As discussed please work on the existing dns id support, as this solution supports the dns id. This logic needs to be enhanced in the upcoming PR. For this PR this changes should be ok

bksagar · 2025-02-17T16:54:55Z

locals.tf

  # dependency: dns -> dns-records
-  dns_instance_id = module.dns.dns_instance_id
+  dns_instance_id = var.enable_deployer ? "" : module.dns[0].dns_instance_id
+  dns_custom_resolver_id = var.enable_deployer ? "" : module.dns[0].dns_custom_resolver_id


@jayeshh123 As discussed please work on the existing support id support, as this solution supports the dns id. This logic needs to be enhanced in the upcoming PR. For this PR this changes should be ok

bksagar · 2025-02-17T17:03:36Z

main.tf

  dns_domain_names           = var.dns_domain_names
  kms_encryption_enabled     = local.kms_encryption_enabled
  boot_volume_encryption_key = local.boot_volume_encryption_key
+  enable_bastion             = var.enable_bastion


@jayeshh123 When we support the existing bastion, we may need to tweak this logic.Just Fyi

bksagar · 2025-02-17T17:04:04Z

main.tf

+  "enable_vpc_flow_logs": ${var.enable_vpc_flow_logs},
+  "allowed_cidr": ${local.allowed_cidr},
+  "vpc_id": "${local.vpc_id}",
+  "vpc": "${local.vpc}",


@jayeshh123 This is something this needs to re-checked and remove this value

bksagar · 2025-02-17T17:06:08Z

main.tf

+  filename = local.schematics_inputs_path
+}
+
+resource "null_resource" "tf_resource_provisioner" {


@jayeshh123 You cannot have any individual resources being created from the main.tf file. Instead this should be called from a module directly

bksagar · 2025-02-17T17:06:17Z

main.tf

+  ]
+}
+
+resource "null_resource" "cluster_destroyer" {


@jayeshh123 You cannot have any individual resources being created from the main.tf file. Instead this should be called from a module directly

bksagar · 2025-02-17T17:10:09Z

main.tf

 # Code for SCC Instance
 module "scc_instance_and_profile" {
-  count                   = var.scc_enable ? 1 : 0
+  count                   = var.enable_deployer == true && var.scc_enable ? 1 : 0


@jayeshh123 Lets talk to Nupur and PC about this and update the logic accordingly

bksagar · 2025-02-17T17:10:53Z

modules/ansible-roles/roles/lsf-ansible-command/tasks/lsf-ansible-command.yml

@@ -0,0 +1,39 @@
+---


@jayeshh123 As discussed if this is not used, please remove this file

bksagar · 2025-02-17T17:24:30Z

modules/landing_zone/locals.tf

  # Calculate network acl rules (can be done inplace in vpcs)
  # TODO: VPN expectation
-  cidrs_network_acl_rules = compact(flatten([local.schematics_reserved_cidrs, var.allowed_cidr, var.network_cidr, "161.26.0.0/16", "166.8.0.0/14"]))
+  cidrs_network_acl_rules = compact(flatten([local.schematics_reserved_cidrs, var.allowed_cidr, var.network_cidr, "161.26.0.0/16", "166.8.0.0/14", "0.0.0.0/0"]))


@jayeshh123 This is something that we need to discuss and configure the ACL as we are not allowed to add 0.0.0.0/0

bksagar · 2025-02-17T17:26:13Z

modules/landing_zone/locals.tf

        acl_name       = "hpc-acl"
        cidr           = var.bastion_subnets_cidr[0]
-        public_gateway = false
+        public_gateway = true


@jayeshh123 When we review this PR for the next changes, please validate if we need the PGW for the bastion node. As we dont support the PGW on bastion nodes. just an FYI

bksagar · 2025-02-18T06:15:59Z

modules/landing_zone_vsi/main.tf

  count            = local.enable_storage ? 1 : 0
  source           = "./../key"
-  private_key_path = "./../../modules/ansible-roles/storage_id_rsa" #checkov:skip=CKV_SECRET_6
+  private_key_path = var.enable_bastion ? "${path.root}/../../modules/ansible-roles/storage_id_rsa" : "${path.root}/modules/ansible-roles/storage_id_rsa" #checkov:skip=CKV_SECRET_6


This also needs an enhancements when we implement the existing bastion feature

bksagar · 2025-02-18T06:17:01Z

modules/landing_zone_vsi/main.tf

  vpc_id                       = var.vpc_id
 }

+resource "ibm_is_security_group_rule" "add_comp_sg_bastion" {


@jayeshh123 As we discussed this resource blocks are not something we must be calling directly.
Although I understand that this was a hack that you brought in. But in the next upcoming PR, lets get to use the fix that has been provided by the DA team . Lets work on the same

bksagar · 2025-02-18T06:21:17Z

solutions/lsf/outputs.tf

-output "lsf" {
-  value = module.lsf
-}
+# output "ssh_to_compute" {


All the unwanted output please remove form the code to avoid confusions @jayeshh123

bksagar · 2025-02-18T06:22:19Z

variables.tf

  )
-  default     = null
+  default = [{
+    mount_path = "/mnt/binaries"


@jayeshh123 This is something that we may need to think through. Since at the solutions level we are using the /mnt/vpcstorage as the mount paths. We need to use a common point for all the solutions

bksagar · 2025-02-18T06:22:58Z

variables.tf

+  description = "Run landing zone module."
+}
+
+variable "vpc_id" {


@jayeshh123 As discussed please take a look if this variable is still required as we take the var.vpc and through data block we can fetch the ID

bksagar

Approved.

There are certain changes those are commented and from the discussion we are merging this PR to unblock and in the upcoming PR all of these should be tracked down, since this is on the development stage have taken a leverage to merge this code.

On the real time prod enc this cannot be done as the above.

Approved

jayeshh123 · 2025-02-18T13:05:27Z

Similar changes are merged from another branch. Now develop branch is having all changes along with other enhancements. Hence this PR can be closed.

bksagar

@manirtm Please change the name "itself" to an appropriate name on the below code block.Please check if there is a general name used and all that needs to be changed

vpc_crn = var.vpc == null ? one(module.landing_zone.vpc_crn) : one(data.ibm_is_vpc.itself[*].crn)

Also please remove any one of the data source blocks as both of them are performing the same operations. like below from the datasource.tf file.

data "ibm_is_vpc" "itself" {
count = var.vpc == null ? 0 : 1
name = var.vpc
}

bksagar

@manirtm Please ensure that all the existing resource features together and do not miss

bksagar

@manirtm Please make sure to run the pre-commit before merging any changes to develop branch. Without this, tracking things at the end of the PR will take a lot of efforts.

Only after the pre-commit is passing i can approve the PR

bksagar

@manirtm Please make the changes on the root main.tf file and on the solutions main.tf to point the resource group name to existing_resource_group.

Please make the necessary changes.

And where ever we have the cluster_id for LSF, please make that changes as cluster_name and make the necessary changes

bksagar

@manirtm All the bastion details that needs to be introduced should have existing_bastion_name .. This is for the next iteration where you implement the existing resources

jayeshh123 · 2025-04-21T09:17:57Z

All the changes are done and merged. All comments are taken care of. Hence this PR can be closed.

Jayesh-Kumar3 added 9 commits February 10, 2025 15:01

LSF Deployer Node Changes

ca0c984

Signed-off-by: Jayesh-Kumar3 <[email protected]>

removed Duplicate Variable

eb794a1

Signed-off-by: Jayesh-Kumar3 <[email protected]>

Made observability conditional

e1c599c

Signed-off-by: Jayesh-Kumar3 <[email protected]>

Added restart playbook and sg changes

ef5d250

Signed-off-by: Jayesh-Kumar3 <[email protected]>

added playbook to modify grp/all file

70d06d8

Signed-off-by: Jayesh-Kumar3 <[email protected]>

added deployer node hostname and ip var

300fbc1

Signed-off-by: Jayesh-Kumar3 <[email protected]>

modified locals

35f677b

Signed-off-by: Jayesh-Kumar3 <[email protected]>

added condition for dns

5495e6c

Signed-off-by: Jayesh-Kumar3 <[email protected]>

Added depends on

bc0de4f

Signed-off-by: Jayesh-Kumar3 <[email protected]>

jayeshh123 requested a review from bksagar February 13, 2025 16:06

jayeshh123 requested review from go-davidng and nupurg-ibm as code owners February 13, 2025 16:06

jayeshh123 self-assigned this Feb 13, 2025

bksagar reviewed Feb 17, 2025

View reviewed changes

bksagar reviewed Feb 18, 2025

View reviewed changes

bksagar approved these changes Feb 18, 2025

View reviewed changes

bksagar mentioned this pull request Feb 18, 2025

Deployer Node Changes Specific to LSF Cluster Creation along with Ansible Playbook #200

Merged

6 tasks

jayeshh123 added 2 commits February 26, 2025 12:33

Removed 0.0.0.0/0 As it was added for testing

b6f2140

Removed 0.0.0.0/0 As it was added for testing

38a31f7

bksagar requested changes Mar 7, 2025

View reviewed changes

jayeshh123 closed this Apr 21, 2025

jayeshh123 deleted the jay_lsf_dep_pr branch April 21, 2025 09:18

Uh oh!

Deployer Node Changes Specific to LSF Cluster Creation #199

Deployer Node Changes Specific to LSF Cluster Creation #199

Uh oh!

Conversation

jayeshh123 commented Feb 13, 2025

Description

Release required?

Release notes content

Run the pipeline

Checklist for reviewers

For mergers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bksagar left a comment

Choose a reason for hiding this comment

Uh oh!

jayeshh123 commented Feb 18, 2025

Uh oh!

bksagar left a comment

Choose a reason for hiding this comment

Uh oh!

bksagar left a comment

Choose a reason for hiding this comment

Uh oh!

bksagar left a comment

Choose a reason for hiding this comment

Uh oh!

bksagar left a comment

Choose a reason for hiding this comment

Uh oh!

bksagar left a comment

Choose a reason for hiding this comment

Uh oh!

jayeshh123 commented Apr 21, 2025

Uh oh!

Reviewers