Added warning after PVC creation #519

surajnarwade · 2017-03-28T09:55:30Z

Partially resolve #376
Added warning, such that PV should be created before PVC or
if dynamic provision is there, no need to create PV.

cdrage · 2017-03-28T12:19:15Z

pkg/transformer/kubernetes/kubernetes.go

@@ -410,6 +410,7 @@ func (k *Kubernetes) ConfigVolumes(name string, service kobject.ServiceConfig) (
 			if err != nil {
 				return nil, nil, nil, errors.Wrap(err, "k.CreatePVC failed")
 			}
+			log.Printf("volume %q of size \"100 Mi\" created, PersistentVolume should be created before in order to PersistentVolumeClaim start working, unless it will be in pending state \n If Cluster has dynamic Provisioning and Storage Classes, So you don't have to do anything", volumeName)


This should be INFO log.Info and this output is wayyyyy too long.

We already output when a PersistentVolumeClaim is created. This should be a small INFO.

@cdrage @kadel , as discussed in the issue, we have to tell user that 100 mb PVC is created and create PV, so that PVC will start working, correct me if I am wrong.

cdrage · 2017-03-28T12:19:32Z

script/test/cmd/tests.sh

 # openshift test
-convert::expect_success "kompose --provider=openshift -f $KOMPOSE_ROOT/script/test/fixtures/volume-mounts/simple-vol-mounts/docker-compose.yml convert --stdout -j" "$KOMPOSE_ROOT/script/test/fixtures/volume-mounts/simple-vol-mounts/output-os.json"
+convert::expect_success_and_warning "kompose --provider=openshift -f $KOMPOSE_ROOT/script/test/fixtures/volume-mounts/simple-vol-mounts/docker-compose.yml convert --stdout -j" "$KOMPOSE_ROOT/script/test/fixtures/volume-mounts/simple-vol-mounts/output-os.json" "volume ""httpd-claim0"" of size ""100 Mi"" created, PersistentVolume should be created before in order to PersistentVolumeClaim start working, unless it will be in pending state
+ If Cluster has dynamic Provisioning and Storage Classes, So you don't have to do anything"


No need to write any tests for this, this is just a log output, no change to logic in code.

As we are giving information after PVC creation now, we have to modify tests for fixture based on volumes.

cdrage · 2017-03-30T12:48:19Z

@surajnarwade I commented earlier on the INFO but I still believe that your output is way too long:

			log.Infof("volume %q of size \"100 Mi\" created, PersistentVolume should be created before in order to PersistentVolumeClaim start working, unless it will be in pending state \n If Cluster has dynamic Provisioning and Storage Classes, So you don't have to do anything", volumeName)

This can be shortened to something along the lines of:

PersistentVolumeClaim of size 100Mi created. PVC's will not work unless a PersistentVolume has been created.

It should be straight-to-the-point!

cdrage · 2017-03-30T12:50:22Z

I think one of the tests may be giving a false positive... I believe this INFO needs to be added to openshift.go.

cdrage · 2017-03-30T12:52:15Z

Also, the additional information would be best added here: https://github.com/kubernetes-incubator/kompose/blob/master/pkg/transformer/kubernetes/kubernetes.go#L667

Or else you'll just get two outputs.

surajnarwade · 2017-03-31T13:08:09Z

cc @cdrage @kadel

cdrage · 2017-03-31T13:19:22Z

This LGTM. Sorry to nitpick again, but would it be better to get the 100 value from a constant? (so we don't have to change the log info if we decide to change the default value)

surajnarwade · 2017-04-03T06:55:59Z

@cdrage ,done with changes

cdrage · 2017-04-03T15:20:05Z

@surajnarwade Could you change these lines too to take from the constant?

https://github.com/surajnarwade/kompose/blob/c37831c29c0c9d78f8138858d236f86ef502c88d/pkg/transformer/kubernetes/kubernetes.go#L247

kadel · 2017-04-03T17:26:23Z

pkg/transformer/openshift/openshift.go

@@ -65,6 +65,9 @@ type OpenShift struct {
 // used when undeploying resources from OpenShift
 const TIMEOUT = 300

+//default value of Persistent Volume Claim
+const PVCValue = "100Mi"


do we need separate constant for openshift?
It might be better to reuse kubernetes.PVCValue

kadel · 2017-04-03T17:28:05Z

pkg/transformer/kubernetes/kubernetes.go

@@ -61,6 +61,9 @@ type Kubernetes struct {
 // used when undeploying resources from kubernetes
 const TIMEOUT = 300

+//default value of Persistent Volume Claim


this is not value of Persistent Volume Claim, it should say that this is default size of Persistent Volume Claim

kadel · 2017-04-03T17:29:19Z

pkg/transformer/kubernetes/kubernetes.go

@@ -61,6 +61,9 @@ type Kubernetes struct {
 // used when undeploying resources from kubernetes
 const TIMEOUT = 300

+//default value of Persistent Volume Claim
+const PVCValue = "100Mi"


can we have 'size' in name of variable? like PVCsize or something like that. Or even better 'PVCRequestSize' as it says everything about what it does :-)

cdrage · 2017-04-10T16:31:40Z

LGTM, does this look good to you too @kadel ?

kadel · 2017-04-11T09:04:37Z

pkg/transformer/kubernetes/kubernetes.go

@@ -664,7 +667,7 @@ func (k *Kubernetes) Deploy(komposeObject kobject.KomposeObject, opt kobject.Con
 			if err != nil {
 				return err
 			}
-			log.Infof("Successfully created PersistentVolumeClaim: %s", t.Name)
+			log.Infof("Successfully created PersistentVolumeClaim: %s of size %s. PVC's will not work unless a PersistentVolume has been created.", t.Name, PVCRequestSize)


Cluster can be also setup with dynamic provisioning. Even new minikube is setup like this by default.
How about this:
Successfully created PersistentVolumeClaim: %s of size %s. For PVC to work your cluster has to be setup with Dynamic provisioning of storage or PersistentVolume has to be created.

Or something in those lines that mentions dynamic provisioning. I bet there is better sentence to explain it than what I've put together ;-)

@kadel, I had written something like this, but removed as @cdrage said, #519 (comment)

I thought it was there before, but missed @cdrage's comment.
We can make it a bit shorter, but we have to mention that you don't have to do anything if you have dynamic provisioning, otherwise it will be confusing.

Another version:
PersistentVolumeClaim of size 100Mi created. If your cluster has dynamic storage provisioning, you don't have to do anything. Otherwise you have to create PersistentVolume to make PVC work.

This a bit shorter than original and says everything.
thoughts @cdrage ?

@kadel That looks excellent! I approve 👍

Partially resolve kubernetes#376 and kubernetes#345 Added warning, such that PV should be created before PVC or if dynamic provision is there, no need to create PV.

surajnarwade · 2017-04-13T09:47:57Z

cc @kadel

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 28, 2017

surajnarwade force-pushed the fix-pvc-info branch from 6d1dd80 to dcbcfee Compare March 28, 2017 10:19

cdrage requested changes Mar 28, 2017

View reviewed changes

surajnarwade force-pushed the fix-pvc-info branch from dcbcfee to a76a976 Compare March 30, 2017 05:15

surajnarwade force-pushed the fix-pvc-info branch from a76a976 to ef9fbb8 Compare March 31, 2017 09:37

cdrage approved these changes Mar 31, 2017

View reviewed changes

surajnarwade force-pushed the fix-pvc-info branch 2 times, most recently from b3ec791 to 309a1e3 Compare March 31, 2017 14:03

surajnarwade force-pushed the fix-pvc-info branch from 309a1e3 to c37831c Compare April 3, 2017 10:29

surajnarwade force-pushed the fix-pvc-info branch from c37831c to 2882376 Compare April 3, 2017 16:55

kadel reviewed Apr 3, 2017

View reviewed changes

surajnarwade force-pushed the fix-pvc-info branch 2 times, most recently from 0760390 to aa30acc Compare April 4, 2017 05:40

kadel requested changes Apr 11, 2017

View reviewed changes

Added warning after PVC creation

9a6c1d9

Partially resolve kubernetes#376 and kubernetes#345 Added warning, such that PV should be created before PVC or if dynamic provision is there, no need to create PV.

surajnarwade force-pushed the fix-pvc-info branch from aa30acc to 9a6c1d9 Compare April 13, 2017 08:55

kadel approved these changes Apr 13, 2017

View reviewed changes

cdrage merged commit 87fc1b5 into kubernetes:master Apr 18, 2017

surajssd mentioned this pull request May 3, 2017

Should we not generate PVC's by default? #376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added warning after PVC creation #519

Added warning after PVC creation #519

surajnarwade commented Mar 28, 2017

cdrage Mar 28, 2017 •

edited

Loading

surajnarwade Mar 30, 2017

cdrage Mar 28, 2017

surajnarwade Mar 30, 2017

cdrage commented Mar 30, 2017

cdrage commented Mar 30, 2017

cdrage commented Mar 30, 2017

surajnarwade commented Mar 31, 2017

cdrage commented Mar 31, 2017

surajnarwade commented Apr 3, 2017

cdrage commented Apr 3, 2017

kadel Apr 3, 2017 •

edited

Loading

kadel Apr 3, 2017

kadel Apr 3, 2017 •

edited

Loading

cdrage commented Apr 10, 2017

kadel Apr 11, 2017

surajnarwade Apr 11, 2017

kadel Apr 11, 2017

cdrage Apr 12, 2017

surajnarwade commented Apr 13, 2017

Added warning after PVC creation #519

Added warning after PVC creation #519

Conversation

surajnarwade commented Mar 28, 2017

cdrage Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage commented Mar 30, 2017

cdrage commented Mar 30, 2017

cdrage commented Mar 30, 2017

surajnarwade commented Mar 31, 2017

cdrage commented Mar 31, 2017

surajnarwade commented Apr 3, 2017

cdrage commented Apr 3, 2017

kadel Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

cdrage commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented Apr 13, 2017

cdrage Mar 28, 2017 •

edited

Loading

kadel Apr 3, 2017 •

edited

Loading

kadel Apr 3, 2017 •

edited

Loading