Use volume components from the devfile #3584

maysunfaisal · 2020-07-15T16:34:44Z

Signed-off-by: Maysun J Faisal maysun.j.faisal@ibm.com

What type of PR is this?
/kind feature

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

utilizes the Volume components from the devfile - refer the devfile v2 schema
can now customize volume size using the devfile volume component
updates func and variable names to differentiate between container components and volume components from the devfile for better readability
unit & integration tests

Which issue(s) this PR fixes:

Fixes #3407

PR acceptance criteria:

Unit test 9932537
Integration test TODO a3eeee9
Documentation

How to test changes / Special notes to the reviewer:

confirm that there can not be duplicate volume components in a devfile:

components:
  - container:
      name: tools
      image: maysunfaisal/springbootbuild
      memoryLimit: 768Mi
      command: ['tail']
      args: [ '-f', '/dev/null']
      mountSources: true
      volumeMounts:
        - name: first
          path: /data
  - container:
      name: runtime
      image: maysunfaisal/springbootruntime
      memoryLimit: 768Mi
      command: ['tail']
      args: [ '-f', '/dev/null']
      endpoints:
        - name: '8080/tcp'
          configuration:
            public: true
          targetPort: 8080
      mountSources: false
      volumeMounts:
        - name: first
          path: /data
  - volume:
      name: secondvol
  - volume:
      name: secondvol
      size: 10Gi

use this spring devfile components and change the container volume mounts to either firstvol, secondvol, thirdvol and odo push. When using

firstvol though not defined in volume components, will be implicitly added with a default size of 5Gi
secondvol will use a default size of 5Gi since its size is not mentioend
thirdvol will use the mentioned size of 10Gi

components:
  - container:
      name: tools
      image: maysunfaisal/springbootbuild
      memoryLimit: 768Mi
      command: ['tail']
      args: [ '-f', '/dev/null']
      mountSources: true
      volumeMounts:
        - name: firstvol
          path: /data
  - container:
      name: runtime
      image: maysunfaisal/springbootruntime
      memoryLimit: 768Mi
      command: ['tail']
      args: [ '-f', '/dev/null']
      endpoints:
        - name: '8080/tcp'
          configuration:
            public: true
          targetPort: 8080
      mountSources: false
      volumeMounts:
        - name: firstvol
          path: /data
  - volume:
      name: secondvol
  - volume:
      name: thirdvol
      size: 10Gi

maysunfaisal · 2020-07-15T19:33:47Z

@adisky @johnmcollier Can you pls help review this PR since you both worked with volumes code and converting v1 to v2 schema..

maysunfaisal · 2020-07-16T01:11:20Z

/retest

codecov · 2020-07-16T03:48:43Z

Codecov Report

Merging #3584 into master will increase coverage by 0.21%.
The diff coverage is 80.20%.

@@            Coverage Diff             @@
##           master    #3584      +/-   ##
==========================================
+ Coverage   45.51%   45.73%   +0.21%     
==========================================
  Files         121      122       +1     
  Lines       12187    12237      +50     
==========================================
+ Hits         5547     5596      +49     
- Misses       6089     6090       +1     
  Partials      551      551

Impacted Files	Coverage Δ
pkg/devfile/adapters/kubernetes/utils/utils.go	`52.80% <0.00%> (ø)`
pkg/testingutil/devfile.go	`0.00% <0.00%> (ø)`
pkg/devfile/validate/errors.go	`40.00% <40.00%> (ø)`
pkg/devfile/adapters/docker/component/utils.go	`66.76% <75.00%> (ø)`
pkg/devfile/validate/components.go	`93.10% <95.23%> (+11.28%)`	⬆️
pkg/devfile/adapters/common/command.go	`96.36% <100.00%> (ø)`
pkg/devfile/adapters/common/utils.go	`87.67% <100.00%> (+23.87%)`	⬆️
pkg/devfile/adapters/docker/component/adapter.go	`57.43% <100.00%> (ø)`
pkg/devfile/adapters/docker/storage/utils.go	`77.02% <100.00%> (ø)`
pkg/devfile/adapters/docker/utils/utils.go	`82.82% <100.00%> (ø)`
... and 3 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 f990ac8...137119d. Read the comment docs.

girishramnani · 2020-07-16T11:58:26Z

pkg/devfile/adapters/common/utils.go

@@ -92,7 +92,8 @@ type CommandNames struct {
 	AdapterName string
 }

-func isComponentSupported(component common.DevfileComponent) bool {
+// isComponentAContainer returns a bool if the component is a container
+func isComponentAContainer(component common.DevfileComponent) bool {


Wouldn’t this be cleaner if it was a method on the component called IsContainer?

girishramnani · 2020-07-16T11:58:54Z

pkg/devfile/adapters/common/utils.go

@@ -101,6 +102,15 @@ func isComponentSupported(component common.DevfileComponent) bool {
 	return false
 }

+// isComponentAVolume returns a bool if the component is a volume
+func isComponentAVolume(component common.DevfileComponent) bool {


Same as above?

maysunfaisal · 2020-07-16T19:15:24Z

/retest

johnmcollier · 2020-07-16T19:23:59Z

pkg/devfile/adapters/common/utils.go

+
+	// containerNameToVolumes is a map of the Devfile container name to the Devfile container Volumes
+	containerNameToVolumes := make(map[string][]DevfileVolume)
+	for _, containerComp := range containerComponents {


Any way we can avoid 3 nested for loops? What about by creating / using a map of vol name -> volume components?

pkg/devfile/adapters/common/command.go

pkg/devfile/adapters/common/utils_test.go

pkg/devfile/validate/components_test.go

prietyc123 · 2020-07-17T04:02:13Z

tests/integration/devfile/cmd_devfile_push_test.go

+
+			// Only testing this in Kubernetes because odo OpenShift CI
+			// sets the min pvc storage size to 100Gi
+			if os.Getenv("KUBERNETES") == "true" {


Can we add this condition in JustAfterEach()

Not sure what the point is in JustAfterEach? in AfterEach we remove the namespace/project. So the pvc will be gone by then.

pkg/devfile/adapters/common/command.go

tests/integration/devfile/cmd_devfile_push_test.go

adisky · 2020-07-17T10:42:59Z

@maysun we are verifying valid volume size at very later stage

when i inserted an invalid volume size, i got this error and odo push failed.

$ odo push --show-log

Validation
 ✓  Validating the devfile [48381ns]

Creating Kubernetes resources for component my-vol
 ✗  Failed to start component with name my-vol. Error: Failed to create the component: unable to create or update component: Error creating PVC for firstvol: unable to parse size: ghjk: quantities must match the regular expression '^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$'

when i checked kubectl get deployment, it had already created the deployment which is waiting for pvc's to get created.
IMO we should validate volume size before creating any kubernetes resources.

Also could we update the error message to something like volume size should be of format <size>Gi e.g. 3Gi

maysunfaisal · 2020-07-20T15:15:16Z

@adisky good point reg validation. I will move it much earlier.

However, I do not think we should make our own validation regex or give an example like "2Gi/2Mi" because there are a lot of units and ways to write storage as evident from the Kube documentation(and also from the kube client api regex error):

You can express memory as a plain integer or as a fixed-point integer using one of these suffixes: E, P, T, G, M, K. You can also use the power-of-two equivalents: Ei, Pi, Ti, Gi, Mi, Ki. For example, the following represent roughly the same value:

128974848, 129e6, 129M, 123Mi

from https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/

I will just use the Kube Client for validation and wrap it around like this:
size 2Girrr for volume component mycomponent is invalid: quantities must match the regular expression '^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$'

adisky · 2020-07-20T15:47:18Z

@maysunfaisal agreed, just give one or two example at the end of regex message.

maysunfaisal · 2020-07-20T15:59:31Z

@adisky how does this sound to you:

$ odo push
 ✗  size 2Girrr for volume component something2 is invalid: quantities must match the regular expression '^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$'. Example - 2Gi, 1024Mi

maysunfaisal · 2020-07-20T16:49:03Z

@adisky @johnmcollier PR updated for review feedback, pls help re-review, thx!

johnmcollier

Changes look good.

maysunfaisal · 2020-07-20T19:32:27Z

/retest

adisky · 2020-07-21T07:21:42Z

pkg/devfile/validate/components.go

-	ErrorNoComponents         = "no components present"
-	ErrorNoContainerComponent = fmt.Sprintf("odo requires atleast one component of type '%s' in devfile", common.ContainerComponentType)
+	ErrorNoComponents              = "no components present"
+	ErrorNoContainerComponent      = fmt.Sprintf("odo requires atleast one component of type '%s' in devfile", common.ContainerComponentType)


can we have proper error types defined? instead of using variables
https://github.com/openshift/odo/pull/3549/files#diff-5fd81db0ba5aa2ae5579e01facb598e8R6

so you're saying we should have a struct for each err types?

pkg/devfile/validate/components.go

mik-dass · 2020-07-21T10:35:58Z

Implicit storage have been removed from the schema devfile/api#101 yesterday. We can drop the support for that too.

maysunfaisal · 2020-07-21T13:37:23Z

@mik-dass oh wow I had implicit volumes with this PR but I checked the schema and there was no implicit volumes and removed it. And now I have to add it back again 😭

EDIT - I meant to say I had non implicit volumes with this PR before it was removed.

adisky · 2020-07-22T05:22:37Z

@maysunfaisal Thanks
/lgtm

maysunfaisal · 2020-07-22T21:25:49Z

/retest

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

johnmcollier

/lgtm

mik-dass · 2020-07-24T03:45:44Z

[odo] ✗ Waiting for component to start [4m]

494

[odo] I0723 22:04:36.264017 10370 occlient.go:1855] Quitting collect events

495

[odo] ✗ waited 4m0s but couldn't find running pod matching selector: 'deploymentconfig=nodejs-app'

/retest

kadel · 2020-07-24T08:49:49Z

/approve

openshift-ci-robot · 2020-07-24T08:50:12Z

[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

openshift-bot · 2020-07-24T09:46:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T10:19:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T10:31:32Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T11:49:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

* Volume components devfile Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com> * Volume Component integration tests Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com> * Vol Component Feedback 1 Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com> * Vol Component Feedback 2 Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com> * Vol Component Feedback 3 Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com> * Container component volMount & volume component dependency Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com> * Fix rebased test Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com> * Remove devfile v1 tests Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com> * Update rebased tests Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Jul 15, 2020

openshift-ci-robot requested review from adisky and prietyc123 July 15, 2020 16:34

maysunfaisal mentioned this pull request Jul 15, 2020

odo storage with devfile components #3398

Closed

4 tasks

maysunfaisal force-pushed the 3407-1 branch from a3442b0 to a3eeee9 Compare July 15, 2020 21:24

girishramnani reviewed Jul 16, 2020

View reviewed changes

maysunfaisal force-pushed the 3407-1 branch from a3eeee9 to c49d285 Compare July 16, 2020 14:39

johnmcollier suggested changes Jul 16, 2020

View reviewed changes

openshift-ci-robot assigned johnmcollier Jul 16, 2020

prietyc123 suggested changes Jul 17, 2020

View reviewed changes

openshift-ci-robot assigned prietyc123 Jul 17, 2020

adisky reviewed Jul 17, 2020

View reviewed changes

pkg/devfile/adapters/common/command.go Show resolved Hide resolved

tests/integration/devfile/cmd_devfile_push_test.go Show resolved Hide resolved

maysunfaisal force-pushed the 3407-1 branch from c49d285 to 4115b93 Compare July 20, 2020 16:48

johnmcollier approved these changes Jul 20, 2020

View reviewed changes

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

adisky reviewed Jul 21, 2020

View reviewed changes

pkg/devfile/validate/components.go Show resolved Hide resolved

maysunfaisal mentioned this pull request Jul 21, 2020

Add in the volume components odo-devfiles/registry#23

Merged

maysunfaisal force-pushed the 3407-1 branch from a1cd35d to 5c09169 Compare July 21, 2020 21:15

openshift-ci-robot assigned adisky Jul 22, 2020

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

maysunfaisal force-pushed the 3407-1 branch from 5c09169 to c30142e Compare July 22, 2020 15:50

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

maysunfaisal added 9 commits July 23, 2020 15:51

Volume components devfile

9ac0a8b

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Volume Component integration tests

22323ba

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Vol Component Feedback 1

fadc143

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Vol Component Feedback 2

53aa6c8

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Vol Component Feedback 3

81f3543

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Container component volMount & volume component dependency

0df7988

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Fix rebased test

b623f0d

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Remove devfile v1 tests

bf3ef4e

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Update rebased tests

137119d

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

maysunfaisal force-pushed the 3407-1 branch from c30142e to 137119d Compare July 23, 2020 20:13

johnmcollier approved these changes Jul 23, 2020

View reviewed changes

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

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

openshift-merge-robot merged commit c89c1ab into redhat-developer:master Jul 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use volume components from the devfile #3584

Use volume components from the devfile #3584

maysunfaisal commented Jul 15, 2020 •

edited

maysunfaisal commented Jul 15, 2020 •

edited

maysunfaisal commented Jul 16, 2020

codecov bot commented Jul 16, 2020 •

edited

girishramnani Jul 16, 2020

maysunfaisal Jul 16, 2020

girishramnani Jul 16, 2020

maysunfaisal Jul 16, 2020

maysunfaisal commented Jul 16, 2020

johnmcollier Jul 16, 2020

maysunfaisal Jul 20, 2020

prietyc123 Jul 17, 2020

maysunfaisal Jul 20, 2020

adisky commented Jul 17, 2020

maysunfaisal commented Jul 20, 2020 •

edited

adisky commented Jul 20, 2020

maysunfaisal commented Jul 20, 2020 •

edited

maysunfaisal commented Jul 20, 2020

johnmcollier left a comment

maysunfaisal commented Jul 20, 2020

adisky Jul 21, 2020

maysunfaisal Jul 21, 2020

maysunfaisal Jul 21, 2020 •

edited

mik-dass commented Jul 21, 2020

maysunfaisal commented Jul 21, 2020 •

edited

adisky commented Jul 22, 2020

maysunfaisal commented Jul 22, 2020

johnmcollier left a comment

mik-dass commented Jul 24, 2020

kadel commented Jul 24, 2020

openshift-ci-robot commented Jul 24, 2020

openshift-bot commented Jul 24, 2020

openshift-bot commented Jul 24, 2020

openshift-bot commented Jul 24, 2020

openshift-bot commented Jul 24, 2020

Use volume components from the devfile #3584

Use volume components from the devfile #3584

Conversation

maysunfaisal commented Jul 15, 2020 • edited

maysunfaisal commented Jul 15, 2020 • edited

maysunfaisal commented Jul 16, 2020

codecov bot commented Jul 16, 2020 • edited

Codecov Report

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

maysunfaisal commented Jul 16, 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

adisky commented Jul 17, 2020

maysunfaisal commented Jul 20, 2020 • edited

adisky commented Jul 20, 2020

maysunfaisal commented Jul 20, 2020 • edited

maysunfaisal commented Jul 20, 2020

johnmcollier left a comment

Choose a reason for hiding this comment

maysunfaisal commented Jul 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Jul 21, 2020 • edited

Choose a reason for hiding this comment

mik-dass commented Jul 21, 2020

maysunfaisal commented Jul 21, 2020 • edited

adisky commented Jul 22, 2020

maysunfaisal commented Jul 22, 2020

johnmcollier left a comment

Choose a reason for hiding this comment

mik-dass commented Jul 24, 2020

kadel commented Jul 24, 2020

openshift-ci-robot commented Jul 24, 2020

openshift-bot commented Jul 24, 2020

openshift-bot commented Jul 24, 2020

openshift-bot commented Jul 24, 2020

openshift-bot commented Jul 24, 2020

maysunfaisal commented Jul 15, 2020 •

edited

maysunfaisal commented Jul 15, 2020 •

edited

codecov bot commented Jul 16, 2020 •

edited

maysunfaisal commented Jul 20, 2020 •

edited

maysunfaisal commented Jul 20, 2020 •

edited

maysunfaisal Jul 21, 2020 •

edited

maysunfaisal commented Jul 21, 2020 •

edited