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

cephfs: no need to check for zero volume size #2472

Merged
merged 2 commits into from
Sep 7, 2021
Merged

Conversation

humblec
Copy link
Collaborator

@humblec humblec commented Sep 1, 2021

At present there is a 'todo' to check for zero volume size
in the createVolume request which in unwanted, ie the pvc
creation with size 0 fail from the kubernetes api validation itself:

For ex:

```
..spec.resources[storage]: Invalid value: "0": must be greater than zero```
```
so we dont need any extra check in the controller server

Signed-off-by: Humble Chirammal hchiramm@redhat.com

@humblec humblec requested a review from Madhu-1 September 1, 2021 09:26
@mergify mergify bot added the component/cephfs Issues related to CephFS label Sep 1, 2021
@Madhu-1 Madhu-1 added the ci/skip/e2e skip running e2e CI jobs label Sep 1, 2021
nixpanic
nixpanic previously approved these changes Sep 1, 2021
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

would be good to give a reference to the CSI spec and/or the implementation where the creation of a 0 size volume is prevented.

@humblec
Copy link
Collaborator Author

humblec commented Sep 1, 2021

would be good to give a reference to the CSI spec and/or the implementation where the creation of a 0 size volume is prevented.

@nixpanic Its actually at CO side, the pvc creation fail like this:

# kubectl create -f check-zero.yaml 
The PersistentVolumeClaim "rbd-pvc-zero" is invalid: spec.resources[storage]: Invalid value: "0": must be greater than zero```

@nixpanic
Copy link
Member

nixpanic commented Sep 1, 2021

Not sure a kubectl check is sufficient, it really should be some CSI standard...

@mergify mergify bot dismissed nixpanic’s stale review September 2, 2021 06:04

Pull request has been modified.

@humblec
Copy link
Collaborator Author

humblec commented Sep 2, 2021

@Madhu-1 @nixpanic ptal.. thanks !

Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

The commit still mentions

creation with size 0 fail from the api validation itself

which might only be correct for Kubernetes, it is better to mention that

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Madhu-1
Madhu-1 previously approved these changes Sep 2, 2021
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

The commit still mentions

creation with size 0 fail from the api validation itself

which might only be correct for Kubernetes, it is better to mention that

This still needs addressing?

@mergify mergify bot dismissed Madhu-1’s stale review September 2, 2021 11:04

Pull request has been modified.

@humblec humblec dismissed nixpanic’s stale review September 2, 2021 11:04

addressed this too.. ptal.

@humblec
Copy link
Collaborator Author

humblec commented Sep 2, 2021

@nixpanic may be final look ? :)

@humblec
Copy link
Collaborator Author

humblec commented Sep 3, 2021

@nixpanic ptal.. thanks !

At present there is a 'todo' to check for zero volume size
in the createVolume request which in unwanted, ie the pvc
creation with size 0 fail from the kubernetes api validation itself:

For ex:

```
..spec.resources[storage]: Invalid value: "0": must be greater than zero```
```
so we dont need any extra check in the controller server

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/skip/e2e skip running e2e CI jobs cleanup component/cephfs Issues related to CephFS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants