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

IBM block storage CSI Integration with IBM Cloud Satellite Storage #83

Merged
merged 24 commits into from
Mar 22, 2021
Merged

IBM block storage CSI Integration with IBM Cloud Satellite Storage #83

merged 24 commits into from
Mar 22, 2021

Conversation

ArbelNathan
Copy link
Contributor

GHE issue: #26

Comment on lines 34 to 37
{
"key": "support-description",
"value": "For support contact IBM: https://www.ibm.com/support/home/"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

make it . This will enable UI to display URL as a hyperlink

{
  "key": "support_URL",
  "value": "https://www.ibm.com/support/home/"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"displayname": "Secret name",
"name": "secret-name",
"required": "true",
"category": "text"
Copy link
Collaborator

@prgavali prgavali Mar 3, 2021

Choose a reason for hiding this comment

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

Replace "category": "text" to "category": "config"

We have 2 categories config and secret

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in every "category": "text" in file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in custom_parameters.json too?

Copy link
Collaborator

@prgavali prgavali left a comment

Choose a reason for hiding this comment

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

added comments

Copy link
Collaborator

@derekpoindexter derekpoindexter left a comment

Choose a reason for hiding this comment

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

left comments & commit suggestions - also had a few questions about the parameters and where to find them/examples


IBM block storage CSI driver is based on an open-source IBM project, included as a part of IBM Storage orchestration for containers. IBM Storage orchestration for containers enables enterprises to implement a modern container-driven hybrid multicloud environment that can reduce IT costs and enhance business agility, while continuing to derive value from existing systems.

For full release notes, compatiblity, installation, and user information, see the IBM block storage CSI driver documentation: https://www.ibm.com/support/knowledgecenter/SSRQ8T_1.4.0/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
For full release notes, compatiblity, installation, and user information, see the IBM block storage CSI driver documentation: https://www.ibm.com/support/knowledgecenter/SSRQ8T_1.4.0/
For full release notes, compatiblity, installation, and user information, see the [IBM block storage CSI driver](documentation: https://www.ibm.com/support/knowledgecenter/SSRQ8T_1.4.0/).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


## Prerequisites

For full prerequisite instructions, see the following documentation, referred to in the introductory paragraph.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
For full prerequisite instructions, see the following documentation, referred to in the introductory paragraph.
- Review the [compatibility and requirements documentation](https://www.ibm.com/support/knowledgecenter/SSRQ8T_1.4.0/csi_ug_requirements.html).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which steps do they need to complete on this page or others? It would be good to have a list of prereqs with links like

- Review the compatibility and requirements
- [Install the required packages](link)
- etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done
I think the documentation is good enough, if we will right it again here we could miss or change things that will cause issues.
they need to go over all of it.

## Prerequisites

For full prerequisite instructions, see the following documentation, referred to in the introductory paragraph.
**Installation** > **Compatibility and requirements**.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
**Installation** > **Compatibility and requirements**.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 15 to 17



Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 27 to 37
| Parameter | Required? | Description | Default value if not provided |
| --- | --- | --- | --- |
| `namespace` | Optional | Deployment namespace. | `default` |
| `sc-name` | Required | Storage class name to be created | N/A |
| `space-efficiency` | Optional | Space efficiency of volume to be created | N/A |
| `pool` | Required | Pool ID of volume to be created in | N/A |
| `secret-name` | Required | Existing secret name | N/A |
| `secret-namespace` | Required | Existing secret name namespace | N/A |
| `fstype` | Optional | File system type | `ext4` |
| `prefix` | Optional | Prefix name of volume to be created | N/A |
| `VolumeExpansion` | Optional | Is volume expansion allowed? | `false` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| Parameter | Required? | Description | Default value if not provided |
| --- | --- | --- | --- |
| `namespace` | Optional | Deployment namespace. | `default` |
| `sc-name` | Required | Storage class name to be created | N/A |
| `space-efficiency` | Optional | Space efficiency of volume to be created | N/A |
| `pool` | Required | Pool ID of volume to be created in | N/A |
| `secret-name` | Required | Existing secret name | N/A |
| `secret-namespace` | Required | Existing secret name namespace | N/A |
| `fstype` | Optional | File system type | `ext4` |
| `prefix` | Optional | Prefix name of volume to be created | N/A |
| `VolumeExpansion` | Optional | Is volume expansion allowed? | `false` |
| Parameter | Required? | Description | Default value if not provided |
| --- | --- | --- | --- |
| `namespace` | Optional | The namespace where you want to create the deployment. | `default` |
| `sc-name` | Required | The name of the storage class name that is created. | N/A |
| `space-efficiency` | Optional | The space efficiency of the volume that is created. Example: | N/A |
| `pool` | Required | The ID of the worker pool where you want to create the volume. Example: | N/A |
| `secret-name` | Required | The name of your existing Kubernetes secret. | N/A |
| `secret-namespace` | Required | The namespace that your Kubernetes secret is in. | N/A |
| `fstype` | Optional | The file system type. Specify... | `ext4` |
| `prefix` | Optional | The prefix name of the volume that is created. Example: | N/A |
| `VolumeExpansion` | Optional | Specify `true` to allow volume expansion or `false` to disallow volume expansion. | `false` |

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Do the secret & deployment namespace need to be the same?
  • What are some examples of the space-effeciency parameter? Or a link to review examples.
  • Is pool the worker pool id?
  • What are possible fstypes
  • What are possible prefixes or examples

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done
instead of adding examples (that eventually mislead due to other options that will not be included in them) I added a link for this step in our documentation that include explanation on all fields.

"category": "config"
},
{
"description": "Existing secret name",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"description": "Existing secret name",
"description": "The name of your existing Kubernetes secret.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"category": "config"
},
{
"description": "Existing secret name namespace",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"description": "Existing secret name namespace",
"description": "The namespace that your Kubernetes secret is in.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"category": "config"
},
{
"description": "File system type",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"description": "File system type",
"description": "The file system type.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"category": "config"
},
{
"description": "Prefix name of volume to be created",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"description": "Prefix name of volume to be created",
"description": "The prefix name of the volume that is created.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"category": "config"
},
{
"description": "Is volume expansion allowed?",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"description": "Is volume expansion allowed?",
"description": "Specify 'true' to allow volume expansion or 'false' to disallow volume expansion.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@derekpoindexter
Copy link
Collaborator

Comment on lines 20 to 24
```

ibmcloud sat storage template get --name ibm-csi-block --version 1.4.0

```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
```
ibmcloud sat storage template get --name ibm-csi-block --version 1.4.0
```

ibmcloud sat storage template get --name ibm-csi-block --version 1.4.0


Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is the fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed, hope it what you meant

Copy link
Collaborator

Choose a reason for hiding this comment

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

removing the extra linebreaks on L19 & 21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

@ArbelNathan
Copy link
Contributor Author

can you also provide example pvc / pv and app pods per the readme - https://github.com/IBM/ibm-satellite-storage/blob/develop/.github/README_TEMPLATE.md#deploying-an-app-that-uses-your-storage-configuration

All pvc / pv/ app pods/ storageclass/ secret/ snapshot ect. examples with commands and outputs can be found in our Documentation. objects examples, using examples

{
"description": "The name of the storage class that is created.",
"displayname": "Storage Class name",
"name": "sc-name",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change "sc-name" to "name" . we have some checks over it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also add new variable for is-default-class

eg

	  "description": "Specify 'true' or 'false' to make the created storage class the default class.",
	  "displayname": "Default",
	  "name": "is-default-class",
	  "default": "false",
	  "required": "false",
	  "category": "config",
	  "type": "text"
	},
	```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
name: "{{ sc-name }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change "sc-name" to "name"
and add add new variable for is-default-class

. Please refer https://github.com/IBM/ibm-satellite-storage/blob/develop/config-templates/netapp/netapp-ontap-san/20.07/storage-class-template.yaml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

@prgavali prgavali left a comment

Choose a reason for hiding this comment

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

added couple of comments

PR
storageclass name parameter and default sc
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
name: "{{ name }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add annotation for is-default-class in storageclass-template ?
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
name: "{{ name }}"
annotations:
storageclass.kubernetes.io/is-default-class: "{{ is-default-class }}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

@prgavali prgavali left a comment

Choose a reason for hiding this comment

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

LGTM

@ArbelNathan
Copy link
Contributor Author

ArbelNathan commented Mar 18, 2021

tested (through API with storageclass) and working.
also works with CLI cmd (no storageclass)

Copy link
Member

@nkkashyap nkkashyap left a comment

Choose a reason for hiding this comment

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

Looks good

@nkkashyap nkkashyap merged commit 8a99b7e into IBM:develop Mar 22, 2021
mssachan added a commit that referenced this pull request Apr 2, 2021
* parameters (#79)

* parameters

* Update custom-parameters.json

* Update custom-parameters.json

* Update custom-parameters.json

* Update custom-parameters.json

* Update AWS EBS SC doc (#80)

* Made changes to create the COS secret from the template

* Added command to fetch endpoints from CLI

* Addressed README review comments

* Addressed review comments

* Updated to get COS location and endpoint from UI

* Update max volume limit for AWS EBS gold Storageclass (#81)

* Update README.md

* Added the ocs-remote template

* Fix AWS template's metadata (#87)

* Updated the readme according to review comments

* Addressed review comments

* Addressed review comments

* Updated ocs-local template with disk path changes

* Update README.md

* fixed parameter mismatch for netapp (#88)

* fixed parameter mismatch for netapp

Signed-off-by: Neeraj K Kashyap <[email protected]>

* fixed parameter mismatch for netapp

Signed-off-by: Neeraj K Kashyap <[email protected]>

* fixed parameter mismatch for netapp

Signed-off-by: Neeraj K Kashyap <[email protected]>

* update logo images (#90)

Signed-off-by: Neeraj K Kashyap <[email protected]>

* Added regex and placeholder (#89)

* IBM block storage CSI Integration with IBM Cloud Satellite Storage (#83)

* adding first testing files

* add diver to template list

* temp metadata.json

* type -> category

* add default ext4 to Fstype parameter in StorageClass

* remove sc files

* new README.md

* 2 README.md

* 3 README.md

* remove line

* add info from template and running deployment

* new line

* fixes

* add sc

* remove "" in boolean

* add storageclass to README.md

* refactoring

* PR - support_URL, text to config

* change order

* PR fixes

* pool parameter description change

* minor fixes

* PR
storageclass name parameter and default sc

* PR added annotations is-default-class

* Added regex and placeholder- updated name (#94)

* update netapp templates

Signed-off-by: Neeraj K Kashyap <[email protected]>

* PR added annotations is-default-class

* Fix devicePath issue in local-volume-file template (#92)

* Fix devicePath issue in local-volume-file template

* Update deployment.yaml

* add regex and update description (#97)

* add regex and update description

Signed-off-by: Neeraj K Kashyap <[email protected]>

* add regex and update description

Signed-off-by: Neeraj K Kashyap <[email protected]>

* add regex and update description

Signed-off-by: Neeraj K Kashyap <[email protected]>

* disable a template for prod promotion (#99)

Signed-off-by: Neeraj K Kashyap <[email protected]>

Co-authored-by: derekpoindexter <[email protected]>
Co-authored-by: Mayank Sachan <[email protected]>
Co-authored-by: Shirisha.S.Rao1 <[email protected]>
Co-authored-by: Shirisha-S-Rao-1 <[email protected]>
Co-authored-by: Pramod Gavali <[email protected]>
Co-authored-by: Arbel Nathan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants