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.

Sign up for GitHub

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

Jump to bottom

To attach additional data volumes to nodes #212

Merged
merged 1 commit into from
Jul 6, 2021
Merged

To attach additional data volumes to nodes #212

merged 1 commit into from
Jul 6, 2021

Conversation

aishwaryabk
Copy link
Collaborator

Signed-off-by: Aishwarya Kamat aishwarya.kamat@ibm.com

@yussufsh
Copy link
Contributor

Is this part of #210?

Couple of suggestions:

  1. Use worker, master variables instead of creating new ones eg: master["data_volume_size"]. We should also change it in powervs.
  2. I know OCS team need to have N number of volumes per worker node. Suggest we also include that.

@bpradipt your thoughts?

@bpradipt
Copy link
Contributor

Is this part of #210?

Couple of suggestions:

1. Use `worker`, `master` variables instead of creating new ones eg: `master["data_volume_size"]`. We should also change it in powervs.

2. I know OCS team need to have N number of volumes per worker node. Suggest we also include that.

@bpradipt your thoughts?

Makes sense. The requirement to add multiple volumes comes due to OCS and needs to be handled for both on-prem and off-prem

@lukebrowning
Copy link

Leet me know when you think this is functional. I will test it for you with ocs.

@lukebrowning
Copy link

I tested this change and it works.

@aishwaryabk aishwaryabk force-pushed the data-disk branch 2 times, most recently from 1059ccd to 916c3d0 Compare July 2, 2021 06:36
@aishwaryabk aishwaryabk force-pushed the data-disk branch 2 times, most recently from 044f64c to 25ad805 Compare July 2, 2021 07:00
@aishwaryabk
Copy link
Collaborator Author

aishwaryabk commented Jul 2, 2021 •
edited
Loading

Updated the files to attach multiple volumes to the nodes.

yussufsh
yussufsh requested changes Jul 2, 2021
View reviewed changes
docs/var.tfvars-doc.md Outdated Show resolved Hide resolved
modules/4_nodes/nodes.tf Outdated Show resolved Hide resolved
modules/4_nodes/nodes.tf Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
yussufsh
yussufsh requested changes Jul 2, 2021
View reviewed changes
modules/4_nodes/nodes.tf Outdated Show resolved Hide resolved
@aishwaryabk aishwaryabk force-pushed the data-disk branch 3 times, most recently from 3a971eb to a8245be Compare July 2, 2021 14:41
@aishwaryabk
Copy link
Collaborator Author

@yussufsh Thank you for the suggestions. I have updated the changes.

yussufsh
yussufsh requested changes Jul 2, 2021
View reviewed changes
docs/var.tfvars-doc.md Outdated Show resolved Hide resolved
modules/4_nodes/nodes.tf Outdated Show resolved Hide resolved
@lukebrowning
Copy link

Expecting four mount points per worker node.

[root@rdr-lbr48-c244-bastion-0 ~]# ssh core@worker-0 lsblk -a | grep 768
sdb 8:16 0 768G 0 disk
sdd 8:48 0 768G 0 disk
sdf 8:80 0 768G 0 disk
sdh 8:112 0 768G 0 disk
[root@rdr-lbr48-c244-bastion-0 ~]# ssh core@worker-1 lsblk -a | grep 768
sdb 8:16 0 768G 0 disk
sdd 8:48 0 768G 0 disk
sdf 8:80 0 768G 0 disk
sdh 8:112 0 768G 0 disk
[root@rdr-lbr48-c244-bastion-0 ~]# ssh core@worker-2 lsblk -a | grep 768
sdb 8:16 0 768G 0 disk
sdd 8:48 0 768G 0 disk

@yussufsh
Copy link
Contributor

yussufsh commented Jul 5, 2021

/approve

Tip: we also mention in docs that the data_volume_size is in unit GB.

Prajyot-Parab
Prajyot-Parab reviewed Jul 5, 2021
View reviewed changes
Copy link
Collaborator

@Prajyot-Parab Prajyot-Parab left a comment

Choose a reason for hiding this comment

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

/lgtm

cc - @bpradipt

bpradipt
bpradipt reviewed Jul 5, 2021
View reviewed changes
variables.tf Outdated Show resolved Hide resolved
bpradipt
bpradipt reviewed Jul 5, 2021
View reviewed changes
docs/var.tfvars-doc.md Outdated Show resolved Hide resolved
@bpradipt
Copy link
Contributor

bpradipt commented Jul 5, 2021

@aishwaryabk the patch looks good. Just minor formatting comments and we should be good to go.
Thanks

@lukebrowning
Copy link

Looks good. Tested latest changes.

bpradipt
bpradipt reviewed Jul 6, 2021
View reviewed changes
variables.tf Outdated
@@ -96,6 +96,9 @@ variable "master" {
# availability_zone = ""
# optional fixed IPs
# fixed_ips = []
# optional data volumes to master nodes
# data_volume_size = 100 #Default volume size to be attached to the master nodes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Default volume size => Default volume size (in GB)

bpradipt
bpradipt reviewed Jul 6, 2021
View reviewed changes
variables.tf Outdated
@@ -109,6 +112,9 @@ variable "worker" {
# availability_zone = ""
# optional fixed IPs
# fixed_ips = []
# optional data volumes to worker nodes
# data_volume_size = 100 #Default volume size to be attached to the worker nodes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Default volume size => Default volume size (in GB)

bpradipt
bpradipt reviewed Jul 6, 2021
View reviewed changes
docs/var.tfvars-doc.md Outdated
@@ -76,6 +76,11 @@ bootstrap = {instance_type = "<bootstrap-compute-template>"
master = {instance_type = "<master-compute-template>", image_id = "<image-uuid-rhcos>", "count" = 3, fixed_ips = ["<IPv4 address>", "<IPv4 address>", "<IPv4 address>"]}
worker = {instance_type = "<worker-compute-template>", image_id = "<image-uuid-rhcos>", "count" = 2, fixed_ips = ["<IPv4 address>", "<IPv4 address>"]}
```
To attach additional volumes to the master and worker nodes, use the optional `data_volume_count` and `data_volume_size` in master and worker variables. You can set the `data_volume_count` to any number depending on the number of volumes to be attached to each node. You can also change the `data_volume_size` as per your requirement (the `data_volume_size` is in unit GB).
Copy link
Contributor

Choose a reason for hiding this comment

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

How about this?
To attach additional volumes to master or worker nodes, set the optional data_volume_count key to the number of volumes that is to be attached and the data_volume_size to the size (in GB) for each volume.

@aishwaryabk
To attach additional data volumes to nodes
ab3dbff 
Signed-off-by: Aishwarya Kamat <aishwarya.kamat@ibm.com>
bpradipt
bpradipt approved these changes Jul 6, 2021
View reviewed changes
Copy link
Contributor

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

/lgtm

@ppc64le-cloud-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aishwaryabk, bpradipt, yussufsh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ppc64le-cloud-bot ppc64le-cloud-bot merged commit fd9751e into ocp-power-automation:master Jul 6, 2021
@aishwaryabk aishwaryabk deleted the data-disk branch July 6, 2021 11:21
@lukebrowning lukebrowning mentioned this pull request Jul 7, 2021
Closed
@lukebrowning
Copy link

Please backport to release branches 4.6 and 4.7.

@bpradipt
Copy link
Contributor

bpradipt commented Jul 8, 2021

It'll be backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Prajyot-Parab Prajyot-Parab Prajyot-Parab left review comments

@bpradipt bpradipt bpradipt approved these changes

@yussufsh yussufsh yussufsh approved these changes

Assignees

@bpradipt bpradipt

@Prajyot-Parab Prajyot-Parab

Labels
approved lgtm size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aishwaryabk @yussufsh @bpradipt @lukebrowning @ppc64le-cloud-bot @Prajyot-Parab