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

Proposal for volume/storage support for opencompose #79

Merged
merged 1 commit into from
May 5, 2017

Conversation

surajssd
Copy link
Contributor

This is a proposal of how the volume definition in opencompose should look like.

@surajssd surajssd changed the title Proposal for volume/stotage support for opencompose Proposal for volume/storage support for opencompose Mar 21, 2017
volumes:
- name: db
path: /app/store
access: ro
Copy link
Member

Choose a reason for hiding this comment

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

This is binary options, it can be only ro or rw. How about changing it to something that reflects this more?
For example readOnly that can boolean, and if not set that false.

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

volumes:
- name: db
path: /app/store
access: ro
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about mode?
access is a bit confusing, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it to accessMode: ReadWriteMany


volumes:
- name: db
size: 5Gib
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's make sure we support GiB, GB, G, MiB, MB, M, 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.

ack


`ro` -> `ReadOnlyMany`
`rw` -> `ReadWriteOnce`
`rwm` -> `ReadWriteMany`
Copy link
Collaborator

Choose a reason for hiding this comment

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

rom, rwo, rwm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it as k8s has it

@surajssd
Copy link
Contributor Author

@kadel @tnozicka updated the proposal according to our meeting, but please check the TODO things that have not been answered yet!

- `volumeName`: (required field, type: *string*) name of volume from root level `volumes` directive or `service` level `emptyDirVolumes` directive.
- `mountPath`: (required field, type: *string*) This will be mapped to `pod.spec.containers.volumeMounts.mountPath` in `deployment`.
- `volumeSubPath`: (optional field, type: *string*) This will be mapped to `pod.spec.containers.volumeMounts.subPath` in `deployment`.
- `readOnly`: (optional field, type: *bool*, default: false) This will be mapped to `pod.spec.containers.volumeMounts.readOnly` in `deployment`. **TODO**: How does this map to the `accessMode` in root level `volumes`?
Copy link
Contributor

@tnozicka tnozicka Mar 22, 2017

Choose a reason for hiding this comment

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

it is not related to accessMode; those are two completely separate things

}
```
src: http://blog.kubernetes.io/2016/10/dynamic-provisioning-and-storage-in-kubernetes.html
- `reclaimPolicy`: (optional field, type: *string*) **TODO**: No field in `pvc`.
Copy link
Member

Choose a reason for hiding this comment

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

oh my bad 😊
we shouldn't have reclaimPolicy here. This is PersistentVolume property, so we don't have to worry about that.


- `name`: (required field, type: *string*) name of volume, which is referenced from containers for mounting. `pvc` will be created with this name.
- `size`: (required field, type: *string*) In `pvc` this will be put in `pvc.spec.resources.requests`
- `accessMode`: (optional field, type: *string*) In `pvc` this will turn into `pvc.spec.accessModes`. The possible values of this field could be: `ReadOnlyMany`, `ReadWriteOnce` or `ReadWriteMany`. **TODO**: Decide default value because `pvc.spec.accessModes` is a required field.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @kadel

Copy link
Member

@kadel kadel Mar 30, 2017

Choose a reason for hiding this comment

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

maybe we should do it as required field for now. We can decide on default later.

@kadel
Copy link
Member

kadel commented Apr 5, 2017

I think we can keep all proposal docs like this in docs/proposal what do you think?
If you squash this we can merge it.

@surajssd
Copy link
Contributor Author

@kadel sure

@surajssd
Copy link
Contributor Author

@kadel ping

@kadel
Copy link
Member

kadel commented May 3, 2017

should this be squashed before merge? or should we keep it as it is to retain history?

This is a proposal of how the volume definition in opencompose
should look like.
@surajssd surajssd force-pushed the storage-proposal branch from 22207ea to 4e99f5b Compare May 5, 2017 09:50
@surajssd
Copy link
Contributor Author

surajssd commented May 5, 2017

should this be squashed before merge? or should we keep it as it is to retain history?

@kadel squashed

@kadel kadel merged commit 67d5df4 into redhat-developer:master May 5, 2017
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