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

Support specifying StorageClass while creating volumes #11451

Merged
merged 2 commits into from
Oct 22, 2016
Merged

Support specifying StorageClass while creating volumes #11451

merged 2 commits into from
Oct 22, 2016

Conversation

gnufied
Copy link
Member

@gnufied gnufied commented Oct 19, 2016

Add support for specifying StorageClass when user is creating
volumes via oc set volume command

@gnufied gnufied force-pushed the support-claim-class-volume branch from feb73d0 to 3965aba Compare October 19, 2016 18:09
@gnufied
Support specifying StorageClass while creating volumes
4a63ef2 
Add support for specifying StorageClass when user is creating
volumes via oc set volume command
@gnufied gnufied force-pushed the support-claim-class-volume branch from 3965aba to 4a63ef2 Compare October 19, 2016 18:10
@childsb
Copy link
Contributor

childsb commented Oct 19, 2016

[test]

@gnufied
Update completions and man pages for volume cmd
cd3f8a8 
Also reword the documentation help
childsb
childsb reviewed Oct 20, 2016
View reviewed changes
docs/man/man1/oc-set-volumes.1
@@ -52,6 +52,10 @@ For descriptions on other volume types, see
select all resources in the namespace of the specified resource types

.PP
\fB\-\-claim\-class\fP=""
StorageClass to use for provisioning the persistent volume.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove all references to 'provisioning' in help string since not all storageClass is provisioned. Replace with something like 'StorageClass to use for the persistent volume claim'.

docs/man/man1/openshift-cli-set-volumes.1
@@ -52,6 +52,10 @@ For descriptions on other volume types, see
select all resources in the namespace of the specified resource types

.PP
\fB\-\-claim\-class\fP=""
StorageClass to use for provisioning the persistent volume.
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with something like 'StorageClass to use for the persistent volume claim'.

docs/man/man1/openshift-cli-volumes.1
@@ -26,6 +26,10 @@ DEPRECATED: This command has been moved to "openshift cli set volume"
select all resources in the namespace of the specified resource types

.PP
\fB\-\-claim\-class\fP=""
StorageClass to use for provisioning the persistent volume.
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with something like 'StorageClass to use for the persistent volume claim'.

pkg/cmd/cli/cmd/set/volume.go
@@ -186,6 +190,7 @@ func NewCmdVolume(fullName string, f *clientcmd.Factory, out, errOut io.Writer)
cmd.Flags().StringVar(&addOpts.ConfigMapName, "configmap-name", "", "Name of the persisted config map. Must be provided for configmap volume type")
cmd.Flags().StringVar(&addOpts.SecretName, "secret-name", "", "Name of the persisted secret. Must be provided for secret volume type")
cmd.Flags().StringVar(&addOpts.ClaimName, "claim-name", "", "Persistent volume claim name. Must be provided for persistentVolumeClaim volume type")
cmd.Flags().StringVar(&addOpts.ClaimClass, "claim-class", "", "StorageClass to use for provisioning the persistent volume.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with something like 'StorageClass to use for the persistent volume claim'.

docs/man/man1/oc-volumes.1
@@ -26,6 +26,10 @@ DEPRECATED: This command has been moved to "oc set volume"
select all resources in the namespace of the specified resource types

.PP
\fB\-\-claim\-class\fP=""
StorageClass to use for provisioning the persistent volume.
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with something like 'StorageClass to use for the persistent volume claim'.

@gnufied gnufied force-pushed the support-claim-class-volume branch from 52ebf8d to cd3f8a8 Compare October 20, 2016 15:15
@gnufied
Copy link
Member Author

gnufied commented Oct 20, 2016

@childsb done. PTAL

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to cd3f8a8

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10313/) (Base Commit: 9ef2b7f)

@gnufied
Copy link
Member Author

gnufied commented Oct 20, 2016

It seems to have flaked on #11024

@rootfs
Copy link
Member

rootfs commented Oct 20, 2016

claim-mode -> access-mode?

@eparis
Copy link
Member

eparis commented Oct 20, 2016

claim-mode is already in oc volume, changes would break existing setups. We could create a --claim-access-mode and work to deprecate --claim-mode if you wanted. But not in this PR.

@eparis
Copy link
Member

eparis commented Oct 20, 2016

What does oc set volume do if I have a --claim-name= and --claim-size= but the claim (by name) exists already? Does it just ignore the size? Does it try to create a new PVC (and fail?)

How about the access mode? The SC?

@gnufied
Copy link
Member Author

gnufied commented Oct 20, 2016

In first case - user's input is rejected outright and she gets following error:

─kube_scripts/xaos_openshift_master> oc set volume dc/mysql5 --add --claim-mode=rwo --claim-name='sandbox6-data' --mount-path='/sandbox2' --name='sandbox7-data' --type='persistentVolumeClaim' --claim-size="5G"
error: persistentvolumeclaims "sandbox6-data" already exists

How about Access mode? The values are not overwritten unless user explicitly uses --overwrite

@eparis
Copy link
Member

eparis commented Oct 21, 2016

[merge]

@eparis
Copy link
Member

eparis commented Oct 22, 2016 •
edited
Loading

[merge] #11114

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to cd3f8a8

@openshift-bot
Copy link
Contributor

openshift-bot commented Oct 22, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10470/) (Base Commit: fe6bff4) (Image: devenv-rhel7_5226)

@openshift-bot openshift-bot merged commit d4771f5 into openshift:master Oct 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@childsb childsb childsb left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gnufied @childsb @openshift-bot @rootfs @eparis