Adds storage create and delete commands for devfiles. #3626

mik-dass · 2020-07-23T11:16:38Z

What type of PR is this?

/kind feature

What does does this PR do / why we need it:

Adds storage create and delete commands for devfile v2.

Which issue(s) this PR fixes:

part of #3398
fixes #3641

PR acceptance criteria:

Unit test
Integration test
Documentation

How to test changes / Special notes to the reviewer:

Create a devfile component
Create a storage for the component odo storage create <name> --path <path>
Check if the new volume was added to the devfile and mounted to all the container components.
Execute odo push and oc get pvc and check if the PVC was created or not.
Delete the storage odo storage delete <name> -f
Check the volume is removed from the devfile and unmounted from all the container components.
Execute odo push and oc get pvc and check if the PVC was deleted or not.
Try out the json output for the commands.

codecov · 2020-07-24T15:42:39Z

Codecov Report

Merging #3626 into master will decrease coverage by 0.28%.
The diff coverage is 47.79%.

@@            Coverage Diff             @@
##           master    #3626      +/-   ##
==========================================
- Coverage   44.39%   44.10%   -0.29%     
==========================================
  Files         137      139       +2     
  Lines       12930    13281     +351     
==========================================
+ Hits         5740     5858     +118     
- Misses       6629     6853     +224     
- Partials      561      570       +9

Impacted Files	Coverage Δ
pkg/component/component.go	`25.76% <0.00%> (-0.04%)`	⬇️
pkg/devfile/adapters/kubernetes/utils/utils.go	`49.47% <0.00%> (-2.75%)`	⬇️
pkg/devfile/parser/context/context.go	`31.03% <0.00%> (-2.30%)`	⬇️
pkg/devfile/parser/context/fake.go	`0.00% <0.00%> (ø)`
pkg/devfile/parser/data/common/errors.go	`0.00% <0.00%> (ø)`
pkg/kclient/volumes.go	`90.16% <0.00%> (-3.06%)`	⬇️
pkg/odo/cli/storage/create.go	`0.00% <0.00%> (ø)`
pkg/odo/cli/storage/delete.go	`0.00% <0.00%> (ø)`
pkg/storage/labels/labels.go	`100.00% <ø> (ø)`
pkg/storage/storage.go	`47.65% <0.00%> (-1.83%)`	⬇️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06922bb...15dd50c. Read the comment docs.

mik-dass · 2020-07-30T18:19:20Z

odo]  ✗  Get https://raw.githubusercontent.com/odo-devfiles/registry/master/devfiles/nodejs/devfile.yaml: net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers)

/retest

dharmit · 2020-07-31T11:35:04Z

pkg/devfile/adapters/common/utils_test.go

@@ -128,7 +128,7 @@ func TestGetVolumes(t *testing.T) {
 	}{
 		{
 			name:      "Case 1: Valid devfile with container referencing a volume component",
-			component: []versionsCommon.DevfileComponent{testingutil.GetFakeContainerComponent("comp1"), testingutil.GetFakeVolumeComponent("myvolume1")},
+			component: []versionsCommon.DevfileComponent{testingutil.GetFakeContainerComponent("comp1"), testingutil.GetFakeVolumeComponent("myvolume1", "4Gi")},


Can we make a const for 4Gi value so that it can be changed at only one place if need be and avoid any error if more tests are added?

dharmit · 2020-07-31T11:38:05Z

pkg/devfile/parser/data/2.0.0/components.go

+		if component.Container != nil {
+			for _, volumeMount := range component.Container.VolumeMounts {
+				if volumeMount.Path == path {
+					return fmt.Errorf("another volume is mounted on the same path")


Should there be more info about which container and path this error showed up for? Would help debug things if someone hits the error, no?

dharmit · 2020-07-31T11:45:11Z

pkg/devfile/parser/data/2.0.0/components.go

+			volumeFound = true
+		}
+	}
+	if volumeFound && mountFound {


Hope I don't embarass myself with this one, but IIUC, mountFound and volumeFound cannot be true at the same time since they're being set to true in opposite situations (one in if and another in else). Unless we make it:

if component.Volume != nil{

instead of:

else if component.Volume != nil

but IIUC, mountFound and volumeFound cannot be true at the same time since they're being set to true in opposite situations (one in if and another in else)

They can be true at the same time too. A component can be a volume or a container. Since we are looping over the components, one can be volume (volumeFound=true) and the other can be a container, which has the volume mounted to it (mountFound=true).

dharmit · 2020-07-31T11:49:25Z

pkg/odo/cli/storage/create.go

-// Run contains the logic for the odo storage create command
-func (o *StorageCreateOptions) Run() (err error) {
-	storageResult, err := o.LocalConfigInfo.StorageCreate(o.storageName, o.storageSize, o.storagePath)
+func (o *StorageCreateOptions) devFileRun() error {


s/devFile/devfile

It's not a valid camel case word so the GoLand editor's linter keeps complaining. Anyways I have changed it to devfile .

dharmit · 2020-07-31T11:50:23Z

pkg/odo/cli/storage/create.go

-func (o *StorageCreateOptions) Run() (err error) {
-	storageResult, err := o.LocalConfigInfo.StorageCreate(o.storageName, o.storageSize, o.storagePath)
+func (o *StorageCreateOptions) devFileRun() error {
+	devFile, err := devfile.ParseAndValidate(o.devfilePath)


Same s/devFile/devfile. I think it's called devfile. We use Devfile only when referring to it in beginning of the sentence. That's what I've observed in the docs PR.

devfile collides with a package name here, so I kept it to devFile to avoid confusion.

dharmit · 2020-07-31T11:54:21Z

pkg/odo/cli/storage/create.go

+		storageCreateCmd.Flags().StringVar(&o.devfilePath, "devfile", "./devfile.yaml", "Path to a devfile.yaml")
+	}
+
+	o.isDevfile = experimental.IsExperimentalModeEnabled() && util.CheckPathExists(o.devfilePath)


Shouldn't this be the other way round as we move out of experimental mode? I mean, first evaluate o.isDevfile and then if o.isDevfile then storageCreateCmd.Flags().StringVar(&o.devfilePath, "devfile", "./devfile.yaml", "Path to a devfile.yaml")

dharmit · 2020-07-31T11:54:37Z

pkg/odo/cli/storage/delete.go

+		storageDeleteCmd.Flags().StringVar(&o.devfilePath, "devfile", "./devfile.yaml", "Path to a devfile.yaml")
+	}
+
+	o.isDevfile = experimental.IsExperimentalModeEnabled() && util.CheckPathExists(o.devfilePath)


Same as earlier comment. Shouldn't this be other way round?

mik-dass · 2020-07-31T14:41:37Z

@dharmit Fixed.

adisky

while doing odo storage delete, are we removing volumes from devfile?

adisky · 2020-08-03T08:38:38Z

pkg/devfile/parser/data/2.0.0/components.go

+				Path: path,
+			})
+		} else if component.Volume != nil && component.Volume.Name == volume.Name {
+			return fmt.Errorf("volume %s already exists", volume.Name)


update to https://github.com/openshift/odo/blob/master/pkg/devfile/parser/data/common/errors.go#L13

adisky · 2020-08-03T08:39:09Z

pkg/devfile/parser/data/2.0.0/components.go

+		if component.Container != nil {
+			for _, volumeMount := range component.Container.VolumeMounts {
+				if volumeMount.Path == path {
+					return fmt.Errorf("another volume, %s, is mounted to the same path: %s, on the container: %s", volumeMount.Name, path, component.Container.Name)


VolumeMountError type

I feel this error is very specific to this function

cdrage

/hold

See review

cdrage · 2020-08-06T19:45:55Z

pkg/devfile/adapters/kubernetes/storage/utils.go

+	label := "component=" + componentName
+	PVCs, err := Client.GetPVCsFromSelector(label)
+	if err != nil {
+		return errors.Wrapf(err, "Unable to get PVC with selectors "+label)


remove capitalization

cdrage · 2020-08-06T19:46:31Z

pkg/devfile/adapters/kubernetes/storage/utils.go

+
+// DeleteOldPVCs deletes all the old PVCs which are not in the processedVolumes map
+func DeleteOldPVCs(Client *kclient.Client, componentName string, processedVolumes map[string]bool) error {
+	label := "component=" + componentName


this is kinda weird. Better if we use fmt.Sprinft? Ex. label := fmt.Sprintf("component=%s", componentName)

cdrage · 2020-08-06T19:46:45Z

pkg/devfile/adapters/kubernetes/storage/utils.go

+		return errors.Wrapf(err, "Unable to get PVC with selectors "+label)
+	}
+	for _, pvc := range PVCs {
+		storageName, ok := pvc.GetLabels()["storage-name"]


need to make storage-name a const

cdrage · 2020-08-06T19:47:10Z

pkg/devfile/parser/data/1.0.0/components.go

+
+func (d *Devfile100) DeleteVolume(name string) error { return nil }
+
+func (d *Devfile100) GetVolumeMountPath(name string) (string, error) {


why not 1 line?

cdrage · 2020-08-06T19:49:00Z

pkg/odo/cli/storage/create.go

 	*genericclioptions.Context
 }

 // NewStorageCreateOptions creates a new StorageCreateOptions instance
 func NewStorageCreateOptions() *StorageCreateOptions {
-	return &StorageCreateOptions{}
+	return &StorageCreateOptions{devfilePath: "./devfile.yaml"}


devfile.yaml should be using the const from component file. In case we were to ever change the name in the future.

cdrage · 2020-08-06T19:49:32Z

pkg/odo/cli/storage/create.go

 	if len(args) != 0 {
 		o.storageName = args[0]
 	} else {
-		o.storageName = o.Component() + "-" + util.GenerateRandomString(4)
+		o.storageName = o.componentName + "-" + util.GenerateRandomString(4)


may be better to do fmt.Springf(%s-%d), o.componentName, util.GenerateRandomString(4) similar to my other comment.

cdrage · 2020-08-06T19:49:51Z

pkg/odo/cli/storage/delete.go

 	*genericclioptions.Context
 }

 // NewStorageDeleteOptions creates a new StorageDeleteOptions instance
 func NewStorageDeleteOptions() *StorageDeleteOptions {
-	return &StorageDeleteOptions{}
+	return &StorageDeleteOptions{devfilePath: "./devfile.yaml"}


cdrage · 2020-08-06T19:49:57Z

pkg/odo/cli/storage/delete.go

+
+		o.componentName = o.EnvSpecificInfo.GetName()
+	} else {
+		// this initializes the LocalConfigInfo as well


mik-dass · 2020-08-07T06:25:04Z

@cdrage Fixed

mik-dass · 2020-08-11T07:47:26Z

• Failure [342.086 seconds]
odo devfile watch command tests
/go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_watch_test.go:15
  when executing odo watch after odo push with debug flag
  /go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_watch_test.go:124
    should be able to start a debug session after push with debug flag using odo watch and revert back after normal push [It]
    /go/src/github.com/openshift/odo/tests/integration/devfile/cmd_devfile_watch_test.go:125
    Timeout after 5.00 minutes
    /go/src/github.com/openshift/odo/tests/helper/helper_generic.go:164

/retest

…s in the code

kadel · 2020-08-17T08:03:54Z

After creating storage, endpoints in devfile.yaml are populated with invalid data :-(

▶ odo create java-maven
Experimental mode is enabled, use at your own risk

Validation
 ✓  Checking devfile existence [37637ns]
 ✓  Checking devfile compatibility [136747ns]
 ✓  Creating a devfile component from registry: my-registry [130070ns]
 ✓  Validating devfile component [139888ns]

Please use `odo push` command to create the component with source deployed

▶ odo storage create --path /data --size 10Gi
 ✓  Added storage java-maven-schd to java-maven

Please use `odo push` command to make the storage accessible to the component

▶ odo push
 ✗  invalid devfile schema. errors :
- components.0.container.endpoints.0: targetPort is required
- components.0.container.endpoints.0: Additional property targetport is not allowed
- components.0.container.endpoints.0.exposure: components.0.container.endpoints.0.exposure must be one of the following: "public", "internal", "none"

▶ cat devfile.yaml
schemaVersion: 2.0.0
metadata:
  name: java-maven
  version: 1.0.0
components:
- container:
    endpoints:
    - attributes: {}
      exposure: ""
      path: ""
      secure: false
      name: http-8080
      targetport: 8080
      protocol: ""
    image: quay.io/eclipse/che-java11-maven:nightly
    memoryLimit: 512Mi
    mountSources: true
    name: tools
    volumeMounts:
    - name: m2
      path: /home/user/.m2
    - name: java-maven-schd
      path: /data
- volume:
    name: m2
- volume:
    name: java-maven-schd
    size: 10Gi
commands:
- exec:
    commandLine: mvn package
    component: tools
    group:
      isDefault: true
      kind: build
    id: mvn-package
- exec:
    commandLine: java -jar target/*.jar
    component: tools
    group:
      isDefault: true
      kind: run
    id: run

…endpoint fields

mik-dass · 2020-08-17T08:23:36Z

After creating storage, endpoints in devfile.yaml are populated with invalid data :-(

The yaml tags are missing from the devfile endpoint fields in the master branch. I have added them in a new commit. Please have a look.

kadel · 2020-08-17T13:28:00Z

/approve

openshift-ci-robot · 2020-08-17T13:28:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kadel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cdrage

Thanks for making the changes. The code LGTM! Works and tested well.

/lgtm

However... we have no documentation for these new functions.. We should be adding this to some of the Devfile documentation on https://odo.dev

mik-dass · 2020-08-17T16:02:52Z

However... we have no documentation for these new functions.. We should be adding this to some of the Devfile documentation on https://odo.dev

👍
I have created #3761 to track it.

openshift-ci-robot added kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. labels Jul 23, 2020

openshift-ci-robot requested review from adisky and prietyc123 July 23, 2020 11:17

mik-dass force-pushed the storage_v2 branch from 5f7d02b to be83476 Compare July 23, 2020 12:16

mik-dass marked this pull request as draft July 23, 2020 12:34

mik-dass force-pushed the storage_v2 branch 2 times, most recently from e68b397 to 6888df0 Compare July 24, 2020 15:12

mik-dass force-pushed the storage_v2 branch 6 times, most recently from 4b10eba to e8d408f Compare July 28, 2020 14:32

mik-dass changed the title ~~[WIP] Adds storage create and delete commands for devfiles.~~ Adds storage create and delete commands for devfiles. Jul 29, 2020

mik-dass removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jul 29, 2020

mik-dass marked this pull request as ready for review July 29, 2020 08:36

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 29, 2020

mik-dass force-pushed the storage_v2 branch from e8d408f to 69f9ae7 Compare July 30, 2020 08:59

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jul 30, 2020

mik-dass force-pushed the storage_v2 branch 2 times, most recently from 3bfc0c7 to 3bdcc67 Compare July 30, 2020 14:22

mik-dass force-pushed the storage_v2 branch from 3bdcc67 to c783ac3 Compare July 31, 2020 04:59

dharmit suggested changes Jul 31, 2020

View reviewed changes

openshift-ci-robot assigned dharmit Jul 31, 2020

mik-dass force-pushed the storage_v2 branch from c783ac3 to 6cdac0e Compare July 31, 2020 14:37

adisky reviewed Aug 3, 2020

View reviewed changes

cdrage suggested changes Aug 6, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Aug 6, 2020

openshift-ci-robot assigned cdrage Aug 6, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 6, 2020

mik-dass force-pushed the storage_v2 branch from 24991da to f8a20f7 Compare August 7, 2020 06:23

mik-dass force-pushed the storage_v2 branch from f8a20f7 to 1643097 Compare August 11, 2020 05:02

mik-dass added 6 commits August 17, 2020 12:31

Adds storage create and delete commands for devfiles.

db9a3cd

Uses a existing error type and creates a new NotFoundError type

258ccb2

fixes order of errors while creating volumes

bc1c6b7

adds json output for storage delete and adds tests

71bf517

fixes capitalization of error messages and comments and uses constant…

8419642

…s in the code

Adds support for context flag

5db2465

mik-dass force-pushed the storage_v2 branch from d5f7321 to 5db2465 Compare August 17, 2020 07:02

mik-dass removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Aug 17, 2020

Fixes unit test for devfile writer and updates yaml tags for devfile …

8288f27

…endpoint fields

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Aug 17, 2020

Adds context flag in s2i test

15dd50c

mik-dass force-pushed the storage_v2 branch from 1a203ce to 15dd50c Compare August 17, 2020 13:45

cdrage approved these changes Aug 17, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 17, 2020

openshift-merge-robot merged commit 7c6dc41 into redhat-developer:master Aug 17, 2020

mik-dass deleted the storage_v2 branch August 18, 2020 04:49

mik-dass mentioned this pull request Aug 20, 2020

odo storage with devfile components #3398

Closed

4 tasks


		func (d *Devfile100) DeleteVolume(name string) error { return nil }

		func (d *Devfile100) GetVolumeMountPath(name string) (string, error) {

Adds storage create and delete commands for devfiles. #3626

Adds storage create and delete commands for devfiles. #3626

Conversation

mik-dass commented Jul 23, 2020 • edited Loading

codecov bot commented Jul 24, 2020 • edited Loading

Codecov Report

mik-dass commented Jul 30, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass commented Jul 31, 2020

adisky left a comment

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

mik-dass Aug 3, 2020 • edited Loading

Choose a reason for hiding this comment

cdrage left a comment

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

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

Choose a reason for hiding this comment

mik-dass commented Aug 7, 2020

mik-dass commented Aug 11, 2020

kadel commented Aug 17, 2020

mik-dass commented Aug 17, 2020 • edited Loading

kadel commented Aug 17, 2020

openshift-ci-robot commented Aug 17, 2020

cdrage left a comment

Choose a reason for hiding this comment

mik-dass commented Aug 17, 2020

mik-dass commented Jul 23, 2020 •

edited

Loading

codecov bot commented Jul 24, 2020 •

edited

Loading

mik-dass Jul 31, 2020 •

edited

Loading

mik-dass Aug 3, 2020 •

edited

Loading

mik-dass commented Aug 17, 2020 •

edited

Loading