Adds storage Push() for devfile components. #4407

mik-dass · 2021-02-04T13:47:40Z

What type of PR is this?

/kind code-refactoring

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

It also adds CLI messages for creation and deletion of storage.

Which issue(s) this PR fixes:

Fixes part of #4298

PR acceptance criteria:

Unit test
Integration test
Documentation
I have read the test guidelines

How to test changes / Special notes to the reviewer:

Adding storage and removing volumes to the devfile should print log messages during odo push.
Tests should pass.

openshift-ci-robot · 2021-02-04T13:47:53Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2021-02-04T13:47:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mik-dass

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~~ [mik-dass]

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

maysunfaisal

Just glanced through the PR to see how it may affect devfile/library.
A few thoughts from odo's perspective if you will 😉

pkg/storage/kubernetes.go

maysunfaisal · 2021-02-08T16:44:37Z

pkg/devfile/adapters/kubernetes/component/adapter.go

+			continue
+		}
+		volumeNameToPVCName[pvc.Labels[storagelabels.DevfileStorageLabel]] = pvc.Name
+	}

 	// Get PVC volumes and Volume Mounts
 	containers, pvcVolumes, err := storage.GetPVCAndVolumeMount(containers, volumeNameToPVCName, containerNameToVolumes)


so i was looking to extract this logic out, so that the library can return the updated containers with volume mounts and the pvc volumes after reading the devfile.

I have only modified the network calls in this function.

Yep, after this merged, I can look to consolidate this in devfile/library call; so anyone calling devfile/library generator func like generator.GetVolume() will return the pvcVolumes and updated container volumeMounts 👍

pkg/kclient/kclient.go

mik-dass · 2021-03-02T13:27:36Z

• Failure in Spec Setup (JustBeforeEach) [306.915 seconds]
odo service command tests for OperatorHub
/go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:17
  When linking devfile component with Operator backed service
  /go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:372
    should successfully connect and disconnect a component with an existing service [JustBeforeEach]
    /go/src/github.com/openshift/odo/tests/integration/operatorhub/cmd_service_test.go:422
    Timeout after 5 minutes

/retest

pkg/storage/storage.go

pkg/storage/s2i.go

pkg/storage/storage.go

pkg/occlient/occlient.go

pkg/devfile/adapters/kubernetes/component/adapter.go

maysunfaisal · 2021-03-11T21:03:42Z

pkg/devfile/adapters/kubernetes/component/adapter.go

+			continue
+		}
+		volumeNameToPVCName[pvc.Labels[storagelabels.DevfileStorageLabel]] = pvc.Name
+	}

 	// Get PVC volumes and Volume Mounts
 	containers, pvcVolumes, err := storage.GetPVCAndVolumeMount(containers, volumeNameToPVCName, containerNameToVolumes)


Yep, after this merged, I can look to consolidate this in devfile/library call; so anyone calling devfile/library generator func like generator.GetVolume() will return the pvcVolumes and updated container volumeMounts 👍

maysunfaisal

lgtm

maysunfaisal · 2021-03-12T18:59:39Z

pkg/storage/kubernetes.go

 	for _, pvc := range pvcs {
 		found := false
 		for _, volumeMount := range volumeMounts {
 			if volumeMount.Name == pvc.Name+"-vol" {
+				// this volume mount is used by a PVC
+				validVolumeMounts[volumeMount.Name] = true
+
 				found = true


I dont think you need this check at all, if you think about it; you can have a volume component in a devfile(and a pvc for it) and not be mounted on any container component. Is it really an err case?

I think this goes back to the case where odo should only create pvc for volume components that are used in container components but i guess it can be a separate issue on it its own

I dont think you need this check at all, if you think about it; you can have a volume component in a devfile(and a pvc for it) and not be mounted on any container component. Is it really an err case?

Yes, we decided not to support such a case as this may lead to unintentional PVC usage.

openshift-bot · 2021-03-13T03:22:50Z

/retest

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

openshift-bot · 2021-03-13T04:40:52Z

/retest

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

openshift-bot · 2021-03-13T05:06:54Z

/retest

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

openshift-bot · 2021-03-13T05:45:52Z

/retest

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

maysunfaisal

lgtm, good catch on source volume

maysunfaisal · 2021-03-15T15:50:23Z

/retest

mik-dass · 2021-03-15T16:26:05Z

[ssh:Fedora 32]     --- FAIL: TestCreateOrUpdateComponent/Case_4:_Valid_devfile,_already_running_component (0.00s)
[ssh:Fedora 32]         adapter_test.go:112: component adapter create unexpected error invalid configuration: no configuration has been provided
[ssh:Fedora 32]             Please login to your server: 
[ssh:Fedora 32]             
[ssh:Fedora 32]             odo login https://mycluster.mydomain.com

/retest

It also adds CLI messages for creation and deletion of storage. Signed-off-by: mik-dass <mrinald7@gmail.com>

… the kubernetes storage client

…rence()

…nterface and adds some comments.

…ontainers and the source volume

maysunfaisal · 2021-03-15T17:20:52Z

rebased
/lgtm

openshift-ci · 2021-03-15T17:22:16Z

@mik-dass: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/v4.6-integration-e2e	`b1a49ed`	link	`/test v4.6-integration-e2e`
ci/prow/v4.6-e2e-4x-psi	`daf11e5`	link	`/test v4.6-e2e-4x-psi`
ci/prow/v4.7-e2e-4x-psi	`ba6e110`	link	`/test v4.7-e2e-4x-psi`
ci/prow/psi-kubernetes-integration-e2e	`e75b72c`	link	`/test psi-kubernetes-integration-e2e`
ci/prow/psi-openshift-integration-e2e	`e75b72c`	link	`/test psi-openshift-integration-e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot added 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 Feb 4, 2021

openshift-ci-robot requested review from dharmit and prietyc123 February 4, 2021 13:47

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

mik-dass force-pushed the storage_push branch from cb276be to 95fc23f Compare February 5, 2021 08:53

mik-dass marked this pull request as ready for review February 5, 2021 08:54

maysunfaisal reviewed Feb 8, 2021

View reviewed changes

mik-dass force-pushed the storage_push branch 8 times, most recently from d958305 to 97007be Compare March 2, 2021 10:21

mik-dass changed the title ~~[WIP] Adds storage Push() for devfile components.~~ Adds storage Push() for devfile components. Mar 2, 2021

openshift-ci-robot 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 Mar 2, 2021

valaparthvi mentioned this pull request Mar 3, 2021

Add telemetry consent to preference #4456

Merged

4 tasks

mik-dass force-pushed the storage_push branch 3 times, most recently from 95905d8 to 52ed1f6 Compare March 4, 2021 14:15

maysunfaisal reviewed Mar 11, 2021

View reviewed changes

mik-dass force-pushed the storage_push branch from 89aae8d to 88e449e Compare March 12, 2021 11:32

maysunfaisal approved these changes Mar 12, 2021

View reviewed changes

openshift-ci-robot assigned maysunfaisal Mar 12, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 12, 2021

mik-dass added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. label Mar 13, 2021

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 15, 2021

maysunfaisal approved these changes Mar 15, 2021

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 15, 2021

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 Mar 15, 2021

mik-dass added 5 commits March 15, 2021 22:44

Adds storage Push() for devfile components.

c645d3f

It also adds CLI messages for creation and deletion of storage. Signed-off-by: mik-dass <mrinald7@gmail.com>

Adds test for Push(). It also adds tests for Create() and Delete() of…

94c0bc9

… the kubernetes storage client

Removes GetOwnerReference() from the kclient

95c98bd

Renames UpdateStorageOwnerReference() to GetAndUpdateStorageOwnerRefe…

5b4f010

…rence()

Fixes ListFromCluster(), removes the StorageAdapter from the common i…

24b8a6b

…nterface and adds some comments.

mik-dass force-pushed the storage_push branch from ba6e110 to 06649e7 Compare March 15, 2021 17:15

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 15, 2021

Fixes ListFromCluster() by avoiding the volume mounts from the init c…

e75b72c

…ontainers and the source volume

mik-dass force-pushed the storage_push branch from 06649e7 to e75b72c Compare March 15, 2021 17:16

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 15, 2021

openshift-merge-robot merged commit 687e751 into redhat-developer:master Mar 15, 2021

mik-dass deleted the storage_push branch March 16, 2021 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds storage Push() for devfile components. #4407

Adds storage Push() for devfile components. #4407

mik-dass commented Feb 4, 2021 •

edited

Loading

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

maysunfaisal left a comment •

edited

Loading

maysunfaisal Feb 8, 2021

mik-dass Feb 24, 2021

maysunfaisal Mar 11, 2021

mik-dass commented Mar 2, 2021

maysunfaisal Mar 11, 2021

maysunfaisal left a comment

maysunfaisal Mar 12, 2021

mik-dass Mar 15, 2021

openshift-bot commented Mar 13, 2021

openshift-bot commented Mar 13, 2021

openshift-bot commented Mar 13, 2021

openshift-bot commented Mar 13, 2021

maysunfaisal left a comment •

edited

Loading

maysunfaisal commented Mar 15, 2021

mik-dass commented Mar 15, 2021

maysunfaisal commented Mar 15, 2021

openshift-ci bot commented Mar 15, 2021

Adds storage Push() for devfile components. #4407

Adds storage Push() for devfile components. #4407

Conversation

mik-dass commented Feb 4, 2021 • edited Loading

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

maysunfaisal left a comment • edited Loading

Choose a reason for hiding this comment

maysunfaisal Feb 8, 2021

Choose a reason for hiding this comment

mik-dass Feb 24, 2021

Choose a reason for hiding this comment

maysunfaisal Mar 11, 2021

Choose a reason for hiding this comment

mik-dass commented Mar 2, 2021

maysunfaisal Mar 11, 2021

Choose a reason for hiding this comment

maysunfaisal left a comment

Choose a reason for hiding this comment

maysunfaisal Mar 12, 2021

Choose a reason for hiding this comment

mik-dass Mar 15, 2021

Choose a reason for hiding this comment

openshift-bot commented Mar 13, 2021

openshift-bot commented Mar 13, 2021

openshift-bot commented Mar 13, 2021

openshift-bot commented Mar 13, 2021

maysunfaisal left a comment • edited Loading

Choose a reason for hiding this comment

maysunfaisal commented Mar 15, 2021

mik-dass commented Mar 15, 2021

maysunfaisal commented Mar 15, 2021

openshift-ci bot commented Mar 15, 2021

mik-dass commented Feb 4, 2021 •

edited

Loading

maysunfaisal left a comment •

edited

Loading

maysunfaisal left a comment •

edited

Loading