-
Notifications
You must be signed in to change notification settings - Fork 243
Adding container flag for storage and fixing code to work with it #4595
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.
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
Adding container flag for storage and fixing code to work with it #4595
Conversation
pkg/envinfo/storage.go
Outdated
} | ||
|
||
// if there are patherrors at this point give generic error | ||
if len(pathErrorContainers) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we check for this error once we exit the for loop here https://github.com/openshift/odo/pull/4595/files#diff-0e87e996b20e349a7984630c2a6cc872741eacef94e7a6e49336abd2091b5ac5R81?
pkg/envinfo/storage.go
Outdated
if volumeExists { | ||
ei.devfileObj.Data.UpdateComponent(volumeComponent) | ||
} else { | ||
err = ei.devfileObj.Data.AddComponents([]devfilev1.Component{volumeComponent}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as we are checking pathErrorContainers
below this, we might be adding duplicate volume mounts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to below the loop. I will push the updated commit shortly.
I am not sure though how we will add duplicate volume mounts. We only add volume mount, if the container in question does not already have it, and similarly, we add the global volume component only if it has not already been added. Otherwise, we update the existing volume component
393dfd3
to
4d2336e
Compare
/refresh |
66f788c
to
9aea7db
Compare
@mohammedzee1000 Thanks for taking this ahead. This feature needs documentation. Please take a look at where it makes most sense to add this on https://odo.dev. It might make sense to add it to Using persistent storage doc. |
/hold Remove hold after documentation is added. |
Will do |
45b23bd
to
b34f4a8
Compare
13ae176
to
2f956c9
Compare
Running locally with ginkgo -v gives me (latest run)
|
I am unable to tell why push is timing out here |
Issue is tracked here: #3256 |
PR #4625 should fix kubernetes issue.
/test psi-kubernetes-integration-e2e |
Ah thanks for the info i guess this pr is now up for review |
if storage.Container == "" || (storage.Container != "" && c.Name == storage.Container) { | ||
if err := ei.devfileObj.Data.AddVolumeMounts(c.Name, vm); err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to check if the given container exists or not.
[mrinaldas@localhost project]$ odo storage create --container fredajdkjsadkj
✓ Added storage nodejs-project-yvzb-ffnp to nodejs-project-yvzb
Please use `odo push` command to make the storage accessible to the component
In the above example, fredajdkjsadkj
is an invalid container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check was added and will be push shortly
pkg/envinfo/storage.go
Outdated
} else { | ||
ei.devfileObj.Data.UpdateComponent(vc[0]) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to update the container as the having a volume based container with the same name will error out in validation.
https://github.com/openshift/odo/blob/cdd92ef3a5051f340df6427aadc46c17d6a99309/pkg/envinfo/storage.go#L35
We should throw an error here instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated else to return error
@@ -41,7 +41,7 @@ type LocalStorage struct { | |||
Size string `yaml:"Size,omitempty"` | |||
// Path of the storage to which it will be mounted on the container | |||
Path string `yaml:"Path,omitempty"` | |||
// Container is the container name on which the storage will be mounted | |||
// Container is the container name on which storage is mounted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this storage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, will push shortly
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
5e7aeb8
to
47557b7
Compare
pkg/envinfo/storage.go
Outdated
// get all volume mount paths in current container | ||
pt, err := ei.devfileObj.Data.GetVolumeMountPaths(storageName, c.Name) | ||
if err != nil { | ||
return "", err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed this earlier but this will fail when the storage is mounted on the second container and not the first
odo storage list
The component 'nodejs-project-dmjd' has the following storage attached:
NAME SIZE PATH CONTAINER STATE
red 1Gi /red runtime-1 Not Pushed
red1 1Gi /data runtime-1 Not Pushed
red11 3Gi /data-1 runtime-1 Not Pushed
odo storage delete red11
✗ volume red11 not mounted to component runtime
We can continue here in case of an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed and also added test for this case. Pushing shortly
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
Signed-off-by: Mohammed Zeeshan Ahmed <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@mohammedzee1000: The following tests failed, say
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. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mik-dass, mohammedzee1000 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:
Approvers can indicate their approval by writing |
Prow issue |
/retest Please review the full test history for this PR and help us cut down flakes. |
Signed-off-by: Mohammed Zeeshan Ahmed [email protected]
What type of PR is this?
/kind feature
What does does this PR do / why we need it:
This PR attempts to fix need for container flag in storage create command
Which issue(s) this PR fixes:
Fixes #4105
PR acceptance criteria:
Unit test
Integration test
Documentation
I have read the test guidelines
How to test changes / Special notes to the reviewer:
WIP