Skip to content
This repository has been archived by the owner on Jul 6, 2023. It is now read-only.

Adding odf module and example #3

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

Conversation

kosilva93
Copy link

No description provided.

@kosilva93 kosilva93 closed this Feb 19, 2022
@kosilva93 kosilva93 reopened this Feb 19, 2022
@kosilva93 kosilva93 closed this Feb 19, 2022
@kosilva93 kosilva93 deleted the odf branch February 19, 2022 10:19
@kosilva93 kosilva93 restored the odf branch February 19, 2022 10:19
@kosilva93 kosilva93 reopened this Feb 19, 2022
@kosilva93
Copy link
Author

@umarali-nagoor

Copy link

@akgunjal akgunjal left a comment

Choose a reason for hiding this comment

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

check comments

examples/odf/README.md Outdated Show resolved Hide resolved

After the service shows as active in the IBM Cloud resource view, verify the deployment:

ibmcloud oc cluster addon ls -c <cluster_name>

Choose a reason for hiding this comment

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

Remove oc from here as its not needed in ibmcloud command

Copy link
Author

Choose a reason for hiding this comment

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

We need oc to run the addon enable command in our shell script

Copy link

@annumberhocker annumberhocker Apr 1, 2022

Choose a reason for hiding this comment

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

@kosilva93 This command can be converted to ibmcloud ks cluster addon ls -c <cluster_name>. You need to add to the README that the ibmcloud container-service/kubernetes-service plugin is required. This can be added with the command ibmcloud plugin install kubernetes-service.

@@ -7,23 +7,23 @@ variable "install_storage" {
description = "If set to false does not install storage and attach the volumes to the worker nodes. Enabled by default"
}

variable unique_id {
variable "unique_id" {

Choose a reason for hiding this comment

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

Do this file need to be removed as its for portworx?

Choose a reason for hiding this comment

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

This one is correct as it is in the examples/portworx/variables.tf file. It should be removed from the examples/odf/variables.tf file

@@ -11,7 +11,7 @@ owner: "ibmtfprovider"
published_at: "2021-12-02T10:34:28.911362Z"
registry: "https://registry.terraform.io/modules/terraform-ibm-modules/cluster-storage/ibm/latest"
alias: "portworx"
tags: ["portworx","storage","cluster","cluster-storage"]
tags: ["portworx","storage","cluster","cluster-storage","odf"]

Choose a reason for hiding this comment

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

portworx needs to removed here?

Choose a reason for hiding this comment

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

Since portworx and odf are combined in the same module directory, shouldn't they be combined in the same metadata.yaml file?

is_nullable: false
is_force_new: false
is_provision_controller: false
is_count_controller: false
outputs:
- name: "portworx_is_ready"

Choose a reason for hiding this comment

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

portworx needs to removed here?

Choose a reason for hiding this comment

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

Portworx and odf are combined in this same directory.

scripts/install_odf.sh Outdated Show resolved Hide resolved
@umarali-nagoor umarali-nagoor self-requested a review March 2, 2022 07:54
Create the file `test.auto.tfvars` with the following input variables, these values are fake examples:

```hcl
enable = true
Copy link

@annumberhocker annumberhocker Mar 4, 2022

Choose a reason for hiding this comment

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

@kosilva93 These options are not accurate. Need to update


module "odf" {
source = "./../.."
// TODO: With Terraform 0.13 replace the parameter 'enable' or the conditional expression using 'with_iaf' with 'count'

Choose a reason for hiding this comment

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

Remove this comment

description = "The cluster where we are going to enable ODF"
}

variable "region" {
Copy link

@annumberhocker annumberhocker Mar 4, 2022

Choose a reason for hiding this comment

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

This variable description needs to be updated to say

"The region of the cluster ODF will be installed on: us-south, us-east, eu-gb, eu-de, jp-tok, au-syd, etc."


## 4. Cleanup

WIP

Choose a reason for hiding this comment

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

add Running terraform destroy will disable the ODF extension.

Copy link
Author

Choose a reason for hiding this comment

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

done

@kosilva93
Copy link
Author

new push made for review

@@ -0,0 +1,74 @@
# Test ODF Terraform Module

Copy link
Contributor

Choose a reason for hiding this comment

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

ODF examples directory should have following files

  1. Versions.tf
  2. output.tf

Choose a reason for hiding this comment

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

@kosilva93 output.tf still missing from examples/odf

description = "If set to true installs ODF on the given cluster"
}

variable "ibmcloud_api_key" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are we passing/using this variable ?

Copy link

@annumberhocker annumberhocker Apr 1, 2022

Choose a reason for hiding this comment

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

The scripts/install_odf.sh requires it to run some ibmcloud cli commands.

description = "Name of the namespace"
type = string
default = "kube-system"
}

variable pwx_plan {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't make any changes to files not related to ODF. Revert back the changes.

Copy link

@annumberhocker annumberhocker Apr 1, 2022

Choose a reason for hiding this comment

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

The only changes were to wrap the variable names with "". I think the vscode .tf extension probably did this as it is best practice



# Retrieve the openshift cluster version
ROKS_VERSION=`oc get clusterversion -o jsonpath='{.items[].status.history[].version}{"\n"}'`

Choose a reason for hiding this comment

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

Use kubectl instead of oc. We can not require a user to download the oc command since they might not have an OpenShift account to download it from.


if [ $ROKS_VERSION_COMP -ge 470 ]; then
echo "Supported version";
ibmcloud oc cluster addon enable openshift-data-foundation -c ${CLUSTER} --version ${ROKS_VERSION} \

Choose a reason for hiding this comment

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

I think this can be changed to ibmcloud ks cluster addon enable openshift-data-foundation

ibmcloud login -apikey ${IC_API_KEY}
ibmcloud ks cluster config -c ${CLUSTER} --admin

ibmcloud oc cluster addon disable openshift-data-foundation -c ${CLUSTER} -f

Choose a reason for hiding this comment

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

change to ibmcloud ks cluster addon...

Copy link

@annumberhocker annumberhocker left a comment

Choose a reason for hiding this comment

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

@kosilva93 these things still need to be fixed:

  • examples/odf directory should have an output.tf file that basically passes thru the odf_is_ready variable.

  • Fix kube_config_path variable in examples/odf/variables.tf and variables.tf to be:

variable "kube_config_path" {
   description = "Directory to store the kubeconfig file. If running on Schematics, use `/tmp/.schematics/.kube/config`"
   type        = string
   default     = "./.kube/config"
}
  • Why is the unique_id Portworx variable needed in examples/odf/variables.tf and examples/odf/main.tf? ODF shouldn't really need to know about this. If you remove it from variables.tf and main.tf it shouldn't create an error since the odf.tf file doesn't require it as input.

  • Fix description of region in examples/odf/variables.tf to:

 The region of the cluster ODF will be installed on: us-south, us-east, eu-gb, eu-de, jp-tok, au-syd, etc.
  • There are still a couple of uses of oc in the scripts/install_odf.sh file. Replace with kubectl

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

Successfully merging this pull request may close these issues.

4 participants