alihan/odo
1
0
Fork 0
mirror of https://github.com/redhat-developer/odo.git synced 2025-10-19 03:06:19 +03:00
Code Issues Packages Projects Releases Wiki Activity

Adding container flag for storage and fixing code to work with it (#4595)

Browse Source 
* Adding container flag for storage and fixing code to work with it

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Moving the patherror check to after the for loop

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Fixing gofmt errors

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Adding devfile with multiple containers

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Adding test for --container command

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Fixing --context flag in test

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Ensuring that volume mount to specific container is actually happening

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Adding missing arg to odo push

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Adding docs for storage with contaner

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Fixing code after rebase

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Fixing gofmt errors

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Adding mount sources to multi container devfile

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Adding check for specified container exists in storage create

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Adding `this storage to comment`

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Fixing usage of container flag

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Fixing spelling of storage in doc

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Return error if volume already exists

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Fixing `GetStorageMountPath`

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Adding storage delete check

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Removing nodejs devfile with multiple containers

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Updating test to use springboot devfile

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Fixing volume spelling

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Removing unneeded logic as per comments

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Check if a container exists, only if container is specified

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Gramatical fixup

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Simplifying logic for validate storage

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Fixing delete for storage on containers other than 1st one

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>

* Moving test to previous suite

Signed-off-by: Mohammed Zeeshan Ahmed <mohammed.zee1000@gmail.com>
This commit is contained in:
Mohammed Ahmed
2021-04-21 07:33:54 +05:30
committed by GitHub
parent 0307191508
commit 2a0cb0dafd
5 changed files with 189 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
docs/public/using-storage.adoc
View File

@@ -37,4 +37,55 @@ We can delete a storage volume using `odo storage delete`
Deleted storage store from nodejs-project-ufyy
Please use `odo push` command to delete the storage from the cluster
----
----
== Adding storage to specific container
If your devfile has multiple containers, you can specify to which container you want the
storage to attach to using the `--container` flag in the `odo storage create` command.
For example:
Let us take a devfile with multiple containers that looks like (partial excerpt):
[source, yaml]
----
...
components:
- name: runtime
container:
image: registry.access.redhat.com/ubi8/nodejs-12:1-36
memoryLimit: 1024Mi
endpoints:
- name: "3000-tcp"
targetPort: 3000
mountSources: true
- name: funtime
container:
image: registry.access.redhat.com/ubi8/nodejs-12:1-36
memoryLimit: 1024Mi
...
----
Here, we have 2 containers, with names `runtime` and `funtime`. To attach a storage, only to the `funtime` container, we can do
[source, sh]
----
$ odo storage create store --path /data --size 1Gi --container funtime
✓ Added storage store to nodejs-testing-xnfg
Please use `odo push` command to make the storage accessible to the component
----
You can list the same, using `odo storage list` command
[source, sh]
----
$ odo storage list
The component 'nodejs-testing-xnfg' has the following storage attached:
NAME SIZE PATH CONTAINER STATE
store 1Gi /data funtime Not Pushed
----

110
pkg/envinfo/storage.go
View File

@@ -2,8 +2,8 @@ package envinfo
import (
"fmt"
"github.com/devfile/library/pkg/devfile/parser/data/v2/common"
"k8s.io/klog/v2"
devfilev1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
"github.com/openshift/odo/pkg/localConfigProvider"
@@ -37,6 +37,26 @@ func (ei *EnvInfo) ValidateStorage(storage localConfigProvider.LocalStorage) err
return fmt.Errorf("storage with name %s already exists", storage.Name)
}
}
if storage.Container == "" {
return nil
}
//check if specified container exists or not (if specified)
containerExists := false
containers, err := ei.GetContainers()
if err != nil {
return err
}
for _, c := range containers {
if c.Name == storage.Container {
containerExists = true
}
}
if !containerExists {
return fmt.Errorf("specified container %s does not exist", storage.Container)
}
return nil
}
@@ -56,26 +76,14 @@ func (ei *EnvInfo) GetStorage(name string) (*localConfigProvider.LocalStorage, e
// CreateStorage sets the storage related information in the local configuration
func (ei *EnvInfo) CreateStorage(storage localConfigProvider.LocalStorage) error {
// Get all the containers in the devfile
containers, err := ei.GetContainers()
if err != nil {
return err
//initialize volume mount and volume container
vm := []devfilev1.VolumeMount{
{
Name: storage.Name,
Path: storage.Path,
},
}
// Add volumeMount to all containers in the devfile
for _, c := range containers {
if err := ei.devfileObj.Data.AddVolumeMounts(c.Name, []devfilev1.VolumeMount{
{
Name: storage.Name,
Path: storage.Path,
},
}); err != nil {
return err
}
}
// Add volume component to devfile. Think along the lines of a k8s pod spec's volumeMount and volume.
err = ei.devfileObj.Data.AddComponents([]devfilev1.Component{{
vc := []devfilev1.Component{{
Name: storage.Name,
ComponentUnion: devfilev1.ComponentUnion{
Volume: &devfilev1.VolumeComponent{
@@ -84,11 +92,47 @@ func (ei *EnvInfo) CreateStorage(storage localConfigProvider.LocalStorage) error
},
},
},
}})
}}
volumeExists := false
// Get all the containers in the devfile
containers, err := ei.GetContainers()
if err != nil {
return err
}
// Add volumeMount to all containers if no container is specified else to specified container(s) in the devfile
for _, c := range containers {
if storage.Container == "" || (storage.Container != "" && c.Name == storage.Container) {
if err := ei.devfileObj.Data.AddVolumeMounts(c.Name, vm); err != nil {
return err
}
}
}
// Get all the components to check if volume component exists
components, err := ei.devfileObj.Data.GetComponents(common.DevfileOptions{})
if err != nil {
return err
}
// check if volume component already exists
for _, component := range components {
if component.Volume != nil && component.Name == storage.Name {
volumeExists = true
}
}
// Add volume component to devfile. Think along the lines of a k8s pod spec's volumeMount and volume.
// Add if volume does not exist, otherwise update
if !volumeExists {
err = ei.devfileObj.Data.AddComponents(vc)
if err != nil {
return err
}
} else {
return fmt.Errorf("volume with name %s already exists", storage.Name)
}
err = ei.devfileObj.WriteYamlDevfile()
if err != nil {
return err
@@ -166,20 +210,18 @@ func (ei *EnvInfo) GetStorageMountPath(storageName string) (string, error) {
return "", fmt.Errorf("invalid devfile: components.container: required value")
}
// since all container components have same volume mounts, we simply refer to the first container in the list
// refer https://github.com/openshift/odo/issues/4105 for addressing "all containers have same volume mounts"
paths, err := ei.devfileObj.Data.GetVolumeMountPaths(storageName, containers[0].Name)
if err != nil {
return "", err
// go over all containers
for _, c := range containers {
// get all volume mount paths in current container
pt, err := ei.devfileObj.Data.GetVolumeMountPaths(storageName, c.Name)
if err != nil {
klog.V(2).Infof("Failed to get volume mount paths for storage %s in container %s: %s", storageName, c.Name, err.Error())
}
if len(pt) > 0 {
return pt[0], nil
}
}
// TODO: Below "if" condition needs to go away when https://github.com/openshift/odo/issues/4105 is addressed.
if len(paths) > 0 {
return paths[0], nil
}
// Sending empty string will lead to bad UX as user will be shown an empty value for the mount path
// that's supposed to be deleted through "odo storage delete" command.
// This and the above "if" condition need to go away when we address https://github.com/openshift/odo/issues/4105
// TODO: Below "if" storage needs to be mounted on multiple containers, then the return will have to be an array.
return "", nil
}

2
pkg/localConfigProvider/localConfigProvider.go
View File

@@ -40,7 +40,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 this storage is mounted
Container string `yaml:"-" json:"-"`
}

11
pkg/odo/cli/storage/create.go
View File

@@ -31,7 +31,8 @@ type CreateOptions struct {
storagePath string
componentContext string
storage localConfigProvider.LocalStorage
container string // container to which this storage belongs
storage localConfigProvider.LocalStorage
*genericclioptions.Context
}
@@ -59,9 +60,10 @@ func (o *CreateOptions) Complete(name string, cmd *cobra.Command, args []string)
}
o.storage = localConfigProvider.LocalStorage{
Name: o.storageName,
Size: o.storageSize,
Path: o.storagePath,
Name: o.storageName,
Size: o.storageSize,
Path: o.storagePath,
Container: o.container,
}
o.Context.LocalConfigProvider.CompleteStorage(&o.storage)
@@ -110,6 +112,7 @@ func NewCmdStorageCreate(name, fullName string) *cobra.Command {
storageCreateCmd.Flags().StringVar(&o.storageSize, "size", "", "Size of storage to add")
storageCreateCmd.Flags().StringVar(&o.storagePath, "path", "", "Path to mount the storage on")
storageCreateCmd.Flags().StringVar(&o.container, "container", "", "Name of container to attach the storage to in devfile")
genericclioptions.AddContextFlag(storageCreateCmd, &o.componentContext)
completion.RegisterCommandFlagHandler(storageCreateCmd, "context", completion.FileCompletionHandler)

53
tests/integration/devfile/cmd_devfile_storage_test.go
View File

@@ -66,6 +66,59 @@ var _ = Describe("odo devfile storage command tests", func() {
}
})
It("should create storage and attach to specified container successfully and list it correctly", func() {
args := []string{"create", "java-springboot", cmpName, "--context", commonVar.Context, "--project", commonVar.Project}
helper.CmdShouldPass("odo", args...)
helper.CopyExample(filepath.Join("source", "devfiles", "springboot", "project"), commonVar.Context)
helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "springboot", "devfile.yaml"), filepath.Join(commonVar.Context, "devfile.yaml"))
storageName := helper.RandString(5)
pathName := "/data1"
size := "1Gi"
helper.CmdShouldPass("odo", "storage", "create", storageName, "--path", pathName, "--context", commonVar.Context, "--container", "tools", "--size", size)
storageList := helper.CmdShouldPass("odo", "storage", "list", "--context", commonVar.Context)
helper.MatchAllInOutput(storageList, []string{pathName, "tools", storageName, size})
helper.CmdShouldPass("odo", "push", "--context", commonVar.Context)
storageList = helper.CmdShouldPass("odo", "storage", "list", "--context", commonVar.Context)
helper.MatchAllInOutput(storageList, []string{pathName, "tools", storageName})
// check the volume name and mount paths for the funtime container
volumesMatched := 0
volNamesAndPaths := commonVar.CliRunner.GetVolumeMountNamesandPathsFromContainer(cmpName, "tools", commonVar.Project)
volNamesAndPathsArr := strings.Fields(volNamesAndPaths)
for _, volNamesAndPath := range volNamesAndPathsArr {
volNamesAndPathArr := strings.Split(volNamesAndPath, ":")
if strings.Contains(volNamesAndPathArr[0], storageName) && volNamesAndPathArr[1] == pathName {
volumesMatched++
}
}
Expect(volumesMatched).To(Equal(1))
// check the volume name and mount path Not present in runtime container
volumesMatched = 0
volNamesAndPaths = commonVar.CliRunner.GetVolumeMountNamesandPathsFromContainer(cmpName, "runtime", commonVar.Project)
volNamesAndPathsArr = strings.Fields(volNamesAndPaths)
for _, volNamesAndPath := range volNamesAndPathsArr {
volNamesAndPathArr := strings.Split(volNamesAndPath, ":")
if strings.Contains(volNamesAndPathArr[0], storageName) && volNamesAndPathArr[1] == pathName {
volumesMatched++
}
}
Expect(volumesMatched).To(Equal(0))
helper.CmdShouldPass("odo", "storage", "delete", "-f", "--context", commonVar.Context, storageName)
helper.CmdShouldPass("odo", "push", "--context", commonVar.Context)
storageList = helper.CmdShouldPass("odo", "storage", "list", "--context", commonVar.Context)
helper.DontMatchAllInOutput(storageList, []string{pathName, "tools", storageName, size})
storageName2 := helper.RandString(5)
helper.CmdShouldPass("odo", "storage", "create", storageName2, "--path", pathName, "--context", commonVar.Context, "--container", "runtime", "--size", size)
helper.CmdShouldPass("odo", "push", "--context", commonVar.Context)
helper.CmdShouldPass("odo", "storage", "delete", "-f", "--context", commonVar.Context, storageName2)
helper.CmdShouldPass("odo", "push", "--context", commonVar.Context)
})
It("should create a storage with default size when --size is not provided", func() {
args := []string{"create", "nodejs", cmpName, "--context", commonVar.Context, "--project", commonVar.Project}
helper.CmdShouldPass("odo", args...)