Main review mods 2023 12 06 #55

tlhumphrey2 · 2023-12-06T13:24:21Z

No description provided.

… folder structure by replacing the subfolders of modules, i.e. aks, logging, storage, and vnet, with the folders in the root by the same names, i.e. aks, logging, storage, and vnet. Also, placed the contents of the folder, 'hpcc' in the root and deleted the folder.

…oy hpcc without external storage.

…d branch 'tlh-bug-fixes-and-personalize-easy-deploy-v0'

…nch: tlh-bug-fixes-and-personalize-easy-deploy-v0. Also, in hpcc/hpcc.tf module 'hpcc' now sources Godji's opinionated hpcc instead my my local clone of it. As of this date, 2023/09/22, Godji has not merged a change I made to my local opinionated. So, hpcc will not deploy until he merges or you switch to sourcing a local clone of my opnionated hpcc.

…if one wants a thor cluster

Add ecl code security

…hing branch:add-terraform-to-deploy-everything

…ariables branch:aks-is-now-using-easy-deploy-variables

… so lite.auto.tfvars is correctly copied to hpcc and/or aks directories.

…ariables branch:aks-is-now-using-easy-deploy-variables. Changed scripts/deploy…

…ariables branch:aks-is-now-using-easy-deploy-variables

branch:few-changes-20231020

dcamper

A few new comments, but be aware that there are still older outstanding conversations as well.

dcamper · 2023-12-19T14:11:20Z

aks/locals.tf

+    node_type_version = "v2"      # v1, v2
+    node_size         = var.aks_serv_node_size
+    single_group      = false
+    min_capacity      = 4


Please verify this min_capacity value. Is it really needed? I am somewhat suspicious of it, if we have trimmed the HPCC cluster down at all then we may not need 4 nodes at minimum.

This is in the nodepool called 'hpccpool'. This is the only nodepool used by the hpcc cluster when 'aks_4nodepools=false'. I set 'min_capacity=4' because the terraform was always using 4 nodepools before I added this new option. I assume you want to know if the hpcc cluster can use less than 4 nodes. I have verified that it can.

Please verify the previous comment.

dcamper · 2023-12-19T14:24:59Z

lite-variables.tf

+variable "aks_azure_region" {
+  type        = string
+  description = "REQUIRED.  The Azure region abbreviation in which to create these resources.\nMust be one of [\"eastus\", \"eastus2\", \"centralus\"].\nExample entry: eastus2"
+  validation {


I think this is noted elsewhere, but: We should remove the region constraints. What is there now, which was brought over from old code, pertains to our group and not to OSS. Users should be free to place their clusters anywhere in the world.

dcamper · 2023-12-19T14:25:41Z

lite-variables.tf

+}
+
+variable "aks_enable_roxie" {
+  description = "REQUIRED.  Enable ROXIE?\nThis will also expose port 8002 on the cluster.\nExample entry: false"


Is this port number correct?

Fixed. Should have been 18002.

dcamper · 2023-12-19T14:27:09Z

lite.auto.tfvars.example

+#------------------------------------------------------------------------------
+
+# Enable ROXIE?
+# This will also expose port 8002 on the cluster.


Is 8002 correct?

Fixed. Changed to 18002.

…d replaced with floor values and '1' if result is 0.

…scription' from outputs.tf

README.md

dcamper · 2024-01-03T19:42:39Z

hpcc/hpcc.tf

@@ -1,6 +1,7 @@
 module "hpcc" {
  #source = "[email protected]:hpccsystems-solutions-lab/tlh-opinionated-terraform-azurerm-hpcc.git"
  source = "github.com/hpccsystems-solutions-lab/tlh-opinionated-terraform-azurerm-hpcc.git"
+  #source = "/home/azureuser/tlhumphrey2/RBA-terraform-azurerm-hpcc"


Please remove commented-out code.

Some commented-out code I don't want to remove until we are fairly sure most everything works properly.

In aks/locals.tf, I have changed to min_capacity of hpccpool and servpool.

Leaving this conversation re: commented-out code as unresolved until it is completed.

I have spoke with Godji and Milind. Godji said that he has the role of Owner and has never tried a role with fewer permissions than Owner. Milind says he has always used TFE to deploy the RBA hpcc terraform, but, believes if you use the terraform outside of TFE you would need elevated permissions.

dcamper · 2024-01-03T19:43:31Z

hpcc/main.tf

@@ -30,11 +30,11 @@ module "metadata" {
  project             = local.metadata.project
 }

-resource "null_resource" "delete_ephemeral_storage_accounts" {
+/*resource "null_resource" "delete_ephemeral_storage_accounts" {


Please remove commented-out code.

Some commented-out code I don't want to delete until we are sure everything works.

Leaving unresolved until the commented-out code is removed.

dcamper · 2024-01-03T19:45:46Z

lite-variables.tf

 }

 variable "aks_enable_roxie" {
-  description = "REQUIRED.  Enable ROXIE?\nThis will also expose port 8002 on the cluster.\nExample entry: false"
+  description = "REQUIRED if you want roxie.  Enable ROXIE?\nThis will also expose port 18002 on the cluster.\nExample entry: false"


"if you want roxie" is incorrect, strictly speaking. If someone omitted the value entirely, because they don't want a Roxie, would the plan fail?

Also, note the capitalization of "Roxie"

Fixed. lite-variables.tf no longer says roxie is required.

dcamper · 2024-01-03T19:47:12Z

lite-variables.tf

 }

 variable "aks_enable_roxie" {
-  description = "REQUIRED.  Enable ROXIE?\nThis will also expose port 8002 on the cluster.\nExample entry: false"
+  description = "REQUIRED if you want roxie.  Enable ROXIE?\nThis will also expose port 18002 on the cluster.\nExample entry: false"


TODO: ECL Watch now uses port 443 instead of 18010, correct? This same thing needs to happen with Roxie: It should use port 443 instead of 18002. 443 is the default https port.

I haven't changed to port 443, because when I did, the terraform didn't work properly. So, I'm waiting until we have most issues solved before trying again.

dcamper · 2024-01-03T19:49:06Z

aks/locals.tf

+    node_size         = var.aks_node_sizes.roxie
+    single_group      = false
+    min_capacity      = 1
+    max_capacity      = 3


Tim, please address the previous questions.

dcamper · 2024-01-03T19:49:30Z

aks/locals.tf

+    node_type_version = "v2"      # v1, v2
+    node_size         = var.aks_serv_node_size
+    single_group      = false
+    min_capacity      = 4


Please verify the previous comment.

dcamper · 2024-01-03T19:52:42Z

hpcc/scripts/delete_ephemeral_storage_accounts

@@ -0,0 +1,16 @@
+#!/bin/bash


Has the previous issue been resolved? Are any storage accounts created and then destroyed if the user only wants ephemeral storage?

lite-locals.tf

dcamper · 2024-01-03T20:00:29Z

lite-locals.tf

+    }
+    workerMemory = {
+      query      = "3G"
+      thirdParty = "500M"


The query value is really workerResources.memory - thirdParty, rounded down. Please change the hardcoded 3G to that computation.

The goal of this block is to divide workResources.memory into "memory for Thor" and "memory for 3rd party tools".

I changed workerMemory.query to the same value as workerResources.memory. But, I don't know what you want thirdParty to be?????

In your commit 81eca56 you changed the query value to exactly the same value as workerResources.memory, which is not what was requested. Your change runs the risk of over-allocation of RAM, leading to an Out Of Memory exception and a killed pod.

Fixed in commit "ed598d8".

…rom lite.auto.tfvars

README.md

…ols are set to this. Also, fixed workerMemory.query and workerMemory.thirdParty.

dcamper · 2024-01-05T13:28:55Z

lite-locals.tf

-      query      = format("%dG", local.thor_worker_ram)
-      thirdParty = "500M"
+      query      = format(%dG", local.workerMemory_query)
+      thirdParty = format(%dG", local.workerMemory_thirdParty)


This is not the same as the original. The hardcoded value was "500M" but your computed value is "500G" -- definitely an error or OOM condition if a Thor job starts.

…e-variables.tf

…capacity changed for hpccpool & servpool. In lite-locals.tf, Corrected workerMemory values.

tlhumphrey2 · 2024-01-05T22:04:01Z

In lite-locals.tf, I corrected the calculation of workMemory.query. From: Dan S. Camper ***@***.***> Sent: Friday, January 5, 2024 6:29 AM To: hpccsystems-solutions-lab/terraform-azurerm-hpcc-lite ***@***.***> Cc: Humphrey, Timothy L (RIS-HBE) ***@***.***>; Author ***@***.***> Subject: Re: [hpccsystems-solutions-lab/terraform-azurerm-hpcc-lite] Main review mods 2023 12 06 (PR #55) *** External email: use caution *** @dcamper commented on this pull request.

________________________________ In lite-locals.tf<#55 (comment)>:

@@ -432,8 +433,8 @@ locals {

memory = format("%dG", local.thor_worker_ram) } workerMemory = { - query = format("%dG", local.thor_worker_ram) - thirdParty = "500M" + query = format(%dG", local.workerMemory_query) + thirdParty = format(%dG", local.workerMemory_thirdParty) This is not the same as the original. The hardcoded value was "500M" but your computed value is "500G" -- definitely an error or OOM condition if a Thor job starts. - Reply to this email directly, view it on GitHub<#55 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAIUZYF7GHALK4XKQDLMAR3YM752FAVCNFSM6AAAAABAJMATMSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMBVHE3DQMJVGY>. You are receiving this because you authored the thread.Message ID: ***@***.******@***.***>>

…

________________________________ The information contained in this e-mail message is intended only for the personal and confidential use of the recipient(s) named above. This message may be an attorney-client communication and/or work product and as such is privileged and confidential. If the reader of this message is not the intended recipient or an agent responsible for delivering it to the intended recipient, you are hereby notified that you have received this document in error and that any review, dissemination, distribution, or copying of this message is strictly prohibited. If you have received this communication in error, please notify us immediately by e-mail, and delete the original message.

README.md

dcamper · 2024-01-08T14:37:01Z

hpcc/hpcc.tf

@@ -1,6 +1,7 @@
 module "hpcc" {
  #source = "[email protected]:hpccsystems-solutions-lab/tlh-opinionated-terraform-azurerm-hpcc.git"
  source = "github.com/hpccsystems-solutions-lab/tlh-opinionated-terraform-azurerm-hpcc.git"
+  #source = "/home/azureuser/tlhumphrey2/RBA-terraform-azurerm-hpcc"


Leaving this conversation re: commented-out code as unresolved until it is completed.

dcamper · 2024-01-08T14:37:40Z

hpcc/main.tf

@@ -30,11 +30,11 @@ module "metadata" {
  project             = local.metadata.project
 }

-resource "null_resource" "delete_ephemeral_storage_accounts" {
+/*resource "null_resource" "delete_ephemeral_storage_accounts" {


Leaving unresolved until the commented-out code is removed.

dcamper · 2024-01-08T14:39:16Z

hpcc/scripts/delete_ephemeral_storage_accounts

@@ -0,0 +1,16 @@
+#!/bin/bash


Please continue to work on this issue; leaving unresolved.

…nd 2) the need for 28 vCPUs before deploying HPCC.

dcamper

No new comments, but there are still unresolved conversations. In addition, the issue of not being able to deploy into the eastus2 region is still unresolved.

tlhumphrey2 · 2024-01-08T16:27:52Z

OK. Now, I'm working on the issue of not being able to deploy into eastus2. As far as I know there shouldn't be unresolved conversations. How to I find any unresolved conversations? From: Dan S. Camper ***@***.***> Sent: Monday, January 8, 2024 9:22 AM To: hpccsystems-solutions-lab/terraform-azurerm-hpcc-lite ***@***.***> Cc: Humphrey, Timothy L (RIS-HBE) ***@***.***>; Author ***@***.***> Subject: Re: [hpccsystems-solutions-lab/terraform-azurerm-hpcc-lite] Main review mods 2023 12 06 (PR #55) *** External email: use caution *** @dcamper requested changes on this pull request. No new comments, but there are still unresolved conversations. In addition, the issue of not being able to deploy into the eastus2 region is still unresolved. - Reply to this email directly, view it on GitHub<#55 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAIUZYEKKDGXD7UIAPQ7RVTYNQMLDAVCNFSM6AAAAABAJMATMSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMBZGUZTANRVGM>. You are receiving this because you authored the thread.Message ID: ***@***.******@***.***>>

…

________________________________ The information contained in this e-mail message is intended only for the personal and confidential use of the recipient(s) named above. This message may be an attorney-client communication and/or work product and as such is privileged and confidential. If the reader of this message is not the intended recipient or an agent responsible for delivering it to the intended recipient, you are hereby notified that you have received this document in error and that any review, dissemination, distribution, or copying of this message is strictly prohibited. If you have received this communication in error, please notify us immediately by e-mail, and delete the original message.

…ged availability_zone from 1 to 2. Why? In eastus2, getting error message saying hpccpool could not be created because constrints too restrictive.

…y? 300 was not enough time for HPCC to fully deploy on my azure vm Standard_B2s

…must be installed).

Ubuntu and others added 30 commits September 21, 2023 13:15

branch:HPCC-27615-fixes-deploy-without-external-storage. Now can depl…

5a51f03

…oy hpcc without external storage.

branch:HPCC-27615-my-local-has-flat-structure

35b1a43

branch:HPCC-27615-easy-deploy. Get is a merge of HPCC-27615 latest an…

f31f1c7

…d branch 'tlh-bug-fixes-and-personalize-easy-deploy-v0'

branch:new-variable-enable_thor. Now one must set 'enable_thor=true' …

977e107

…if one wants a thor cluster

branch:fix-roxie-so-port-18002-is-used

1b54ae7

branch:add-htpasswd-support

eba6f63

branch:add-ecl-code-security. Plus, , setup 'storage_data_lz'

58268fa

branch:add-ecl-code-security

fbafd5d

Merge pull request #1 from tlhumphrey2/add-ecl-code-security

32b918b

Add ecl code security

branch:add-terraform-to-deploy-everything

226139f

Merge pull request #2 from tlhumphrey2/add-terraform-to-deploy-everyt…

982e692

…hing branch:add-terraform-to-deploy-everything

branch:add-terraform-to-deploy-everything

347ceef

Merge pull request #3 from tlhumphrey2/add-terraform-to-deploy-everyt…

1e0e18f

…hing branch:add-terraform-to-deploy-everything

branch:aks-is-now-using-easy-deploy-variables

e5f5853

Merge pull request #4 from tlhumphrey2/aks-is-now-using-easy-deploy-v…

9f93f16

…ariables branch:aks-is-now-using-easy-deploy-variables

branch:aks-is-now-using-easy-deploy-variables

7fb2c89

Merge pull request #5 from tlhumphrey2/aks-is-now-using-easy-deploy-v…

cde9f4f

…ariables branch:aks-is-now-using-easy-deploy-variables

branch:aks-is-now-using-easy-deploy-variables. Changed scripts/deploy…

0f56039

… so lite.auto.tfvars is correctly copied to hpcc and/or aks directories.

Merge pull request #6 from tlhumphrey2/aks-is-now-using-easy-deploy-v…

1bf7dc9

…ariables branch:aks-is-now-using-easy-deploy-variables. Changed scripts/deploy…

branch:aks-is-now-using-easy-deploy-variables

e14e9eb

Merge pull request #7 from tlhumphrey2/aks-is-now-using-easy-deploy-v…

3de0cc1

…ariables branch:aks-is-now-using-easy-deploy-variables

branch:aks-is-now-using-easy-deploy-variables

dbe5cef

Merge pull request #8 from tlhumphrey2/aks-is-now-using-easy-deploy-v…

ff58b6d

…ariables branch:aks-is-now-using-easy-deploy-variables

branch:few-changes-20231020

2203f42

Merge pull request #9 from tlhumphrey2/few-changes-20231020

71434db

branch:few-changes-20231020

branch:HPCC-27615-easy-deploy-bryan1

c54dad8

branch:HPCC-27615-easy-deploy-bryan1-w-oss

f077f15

branch:HPCC-27615-easy-deploy-bryan1-w-oss-zones

1868452

Make 1 or 4 nodepools optional. Added aks_4nodepools

97f7eef

dcamper requested changes Dec 19, 2023

View reviewed changes

Ubuntu added 4 commits December 19, 2023 15:09

Removed all error messages in 'thorpool_max_capacity' calculations an…

4ba65e2

…d replaced with floor values and '1' if result is 0.

Removed all occurrences of region restriction.

a8cef3e

In lite-variables.tf, changed 8002 to 18002

d899526

In lite.auto.tfvars.example, changed 8002 to 18002

2255093

tlhumphrey2 requested a review from dcamper December 19, 2023 15:42

Ubuntu added 3 commits December 19, 2023 16:36

Removed all error messages in 'thorpool_max_capacity' calculations an…

a5e3dc5

…d replaced with floor values and '1' if result is 0.

To metadata.tf of storage, added 'additional_tags'. Plus, removed 'de…

abbb3bf

…scription' from outputs.tf

'workerResources' memory. Added 'G'. Caused thor container error.

9cf1765

dcamper requested changes Jan 3, 2024

View reviewed changes

Ubuntu added 3 commits January 3, 2024 20:44

Added scripts/extract-aks-tfvars to properly extra 'aks_' variables f…

f58b8b7

…rom lite.auto.tfvars

In lite-variables.tf, no longer says REQUIRED for aks_enable_roxie.

9cdfc88

Changed workerMemory.query to same value as workerResources.memory.

81eca56

dcamper reviewed Jan 4, 2024

View reviewed changes

README.md Show resolved Hide resolved

Created 'aks_nodepools_max_capacity'. max_capacity of all hpcc nodepo…

ed598d8

…ols are set to this. Also, fixed workerMemory.query and workerMemory.thirdParty.

dcamper reviewed Jan 5, 2024

View reviewed changes

Ubuntu added 2 commits January 5, 2024 15:38

Corrected 'aks_nodepools_max_capacity' code of lite-locals.tf and lit…

05f46ff

…e-variables.tf

In README.md, minimum vCPU requirements given. In aks/locals.tf, min_…

09cad2b

…capacity changed for hpccpool & servpool. In lite-locals.tf, Corrected workerMemory values.

tlhumphrey2 requested a review from dcamper January 8, 2024 14:09

dcamper requested changes Jan 8, 2024

View reviewed changes

tlhumphrey2 requested a review from dcamper January 8, 2024 14:49

Added in README.md: 1) info about the directory, and what is in it, a…

4d1b78f

…nd 2) the need for 28 vCPUs before deploying HPCC.

dcamper requested changes Jan 8, 2024

View reviewed changes

Ubuntu added 3 commits January 10, 2024 18:40

In scripts/needed-auto-tfvars-files/aks/aks.auto.tfvars.example, chan…

e4a8cc4

…ged availability_zone from 1 to 2. Why? In eastus2, getting error message saying hpccpool could not be created because constrints too restrictive.

In lite-locals.tf, increased 'helm_chart_timeout' from 300 to 600. Wh…

431fad0

…y? 300 was not enough time for HPCC to fully deploy on my azure vm Standard_B2s

In README.md, said that 'jq' and 'kubelogin' are required (i.e. they …

3c7c641

…must be installed).

tlhumphrey2 requested a review from dcamper January 22, 2024 18:34

Main review mods 2023 12 06 #55

Are you sure you want to change the base?

Main review mods 2023 12 06 #55

Conversation

tlhumphrey2 commented Dec 6, 2023

dcamper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlhumphrey2 commented Jan 5, 2024 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcamper left a comment

Choose a reason for hiding this comment

tlhumphrey2 commented Jan 8, 2024 via email