Skip to content
This repository was archived by the owner on Jun 23, 2020. It is now read-only.

Dynamic Dedicated FSS #154

Merged
merged 5 commits into from
Aug 15, 2018
Merged

Dynamic Dedicated FSS #154

merged 5 commits into from
Aug 15, 2018

Conversation

MadalinaPatrichi
Copy link
Member

@MadalinaPatrichi MadalinaPatrichi commented Jul 31, 2018

An FSS filesystem will be dynamically created to satisfy a Persistent Volume Claim. The created File System Storage is treated by Kubernetes as an NFSVolumeSource. This helps users as they do not need to create the filesystem or the MountTarget (if it does not exist) ahead of time.

The following cases are covered in the current implementation:

  1. Mount Target ID is provided by the user in the storage class file. Subsequent PVCs referencing the storage claim will attach the newly created filesystem to the given Mount Target
  2. Subnet ID is provided by the user in the storage class file. Subsequent PVCs referencing the storage claim will attach the newly created filesystem to a Mount Target dynamically created on the provided subnet.
  3. None of the above mentioned parameters are provided in the storage class. Subsequent PVCs referencing the storage claim will attach the newly created filesystem to a Mount Target inside the given compartment chosen at random.

In addition to the implementation of the Dynamic Dedicated FSS implementation, an initial refactoring of the system tests has been included to simplify the testing process of the feature.

NFS: &v1.NFSVolumeSource{
// Randomnly select IP address associated with the mount target to use for attachment
Server: serverIP,
Path: *common.String("/" + *response.FileSystem.Id),
Copy link
Member

@owainlewis owainlewis Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*common.String isn't needed here

Spec: v1.PersistentVolumeSpec{
PersistentVolumeReclaimPolicy: options.PersistentVolumeReclaimPolicy,
AccessModes: options.PVC.Spec.AccessModes,
//FIXME: fs storage doesn't enforce quota, capacity is meaningless here.
Copy link
Member

@owainlewis owainlewis Aug 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we propagate mountOptions here?

kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: standard
provisioner: kubernetes.io/aws-ebs
parameters:
  type: gp2
reclaimPolicy: Retain
mountOptions:
  - debug

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly, what are they useful for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should leave this for another commit? This is getting fairly big

@@ -0,0 +1,41 @@
// Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright (c) 2018

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, bad copy/pasting on my part haha

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all the files need to be updated?

Copy link
Contributor

@prydie prydie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't reviewed the Python yet and will likely have more feedback on the Go next time around but overall great work 👍

Gopkg.toml Outdated
@@ -41,6 +41,6 @@
name = "k8s.io/kubernetes"

[[constraint]]
version = "v1.0.0"
version = "v1.5.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version 2.1 of the SDK is out.

@@ -0,0 +1,14 @@
kind: PersistentVolumeClaim
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be in examples/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I create a separate folder for template files? I followed the model used by the block volume

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should live in a directory with the tests I think.

@@ -0,0 +1,20 @@
apiVersion: v1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be in examples/

name: oci-fss
provisioner: oracle.com/oci-fss
parameters:
mntTargetId: {{MNT_TARGET_OCID}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing \n

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,8 @@
# Storage class with subnet OCID to create mount target on
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be in examples/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to separate folder


func TestCreateVolumeWithFSS(t *testing.T) {
// test creating a volume on a file system storage
options := controller.VolumeOptions{PVName: "dummyVolumeOptions",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options := controller.VolumeOptions{
	PVName: "dummyVolumeOptions",
	PVC: &v1.PersistentVolumeClaim{
		ObjectMeta: metav1.ObjectMeta{},
	},
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"github.com/oracle/oci-go-sdk/common"
"github.com/oracle/oci-go-sdk/identity"
"github.com/oracle/oci-volume-provisioner/pkg/helpers"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package ordering / separation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VS code does some ordering for me, but not completely. What are the conventions?

MountTargetId: common.String(mountTargetID),
})
if err != nil {
glog.Errorf("Failed to retrieve mount point: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 		glog.Errorf("Failed to retrieve mount point mountTargetId=%q: %v", mountTargetID, err) 

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"github.com/oracle/oci-go-sdk/common"
"github.com/oracle/oci-go-sdk/core"
"github.com/oracle/oci-go-sdk/filestorage"
"github.com/oracle/oci-go-sdk/identity"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import ordering / grouping

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Path: common.String("/" + *response.FileSystem.Id),
},
})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\n

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Makefile Outdated
DOCKER_REGISTRY_TENANCY ?= oracle
DOCKER_REGISTRY_USERNAME ?= oracle
DOCKER_REGISTRY_TENANCY ?= spinnaker
DOCKER_REGISTRY_USERNAME ?= spinnaker/ioana-madalina.patrichi@oracle.com
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to revert this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this

storageClassName: "oci-fss"
selector:
matchLabels:
oci-availability-domain: "PHX-AD-1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the older deprecated label. Can use failure-domain.beta.kubernetes.io/zone: "PHX-AD-1" instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

}
serverIP := ""
if len(mntTargetResp.PrivateIpIds) != 0 {
privateIPID := mntTargetResp.PrivateIpIds[rand.Int()%len(mntTargetResp.PrivateIpIds)]
Copy link
Member

@owainlewis owainlewis Aug 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a space for readability

rand.Int() % len(mntTargetResp.PrivateIpIds)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@MadalinaPatrichi MadalinaPatrichi force-pushed the mp/fileStorageNew branch 4 times, most recently from fcdbcee to c267b61 Compare August 10, 2018 09:52
Gopkg.toml Outdated
@@ -43,4 +43,3 @@
[[constraint]]
version = "v1.6.0"
name = "github.com/oracle/oci-go-sdk"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@MadalinaPatrichi MadalinaPatrichi force-pushed the mp/fileStorageNew branch 10 times, most recently from df5ec1d to e687e3e Compare August 13, 2018 14:04
@MadalinaPatrichi MadalinaPatrichi changed the title [WIP] Dedicated FSS Dedicated FSS Aug 13, 2018
@oracle oracle deleted a comment from MadalinaPatrichi Aug 13, 2018
@oracle oracle deleted a comment from MadalinaPatrichi Aug 13, 2018
"github.com/oracle/oci-volume-provisioner/pkg/provisioner/plugin"
)

const (
// ProvisionerNameBlock name to deploy block provisioner by
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProvisionerNameBlock is the name of the OCI block volume provisioner

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"github.com/oracle/oci-volume-provisioner/pkg/provisioner/plugin"
)

const (
// ProvisionerNameBlock name to deploy block provisioner by
ProvisionerNameBlock = "oracle.com/oci"
// ProvisionerNameFss name to deploy block provisioner by
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProvisionerNameFss is the name of the OCI FSS dedicated storage provisioner

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

var _ plugin.ProvisionerPlugin = &filesystemProvisioner{}

// NewFilesystemProvisioner creates a new file system provisioner that creates
// filsystems using OCI file system service.
Copy link
Member

@owainlewis owainlewis Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filsystems using OCI file system service. => filesystems using OCI File Storage Service.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

func getOrCreateMountTarget(ctx context.Context, availabilityDomain *string, subnetID string, compartmentID string, fileStorageClient client.FileStorage) (id *filestorage.MountTarget, err error) {
// Check if there there already is a mount target in the existing compartment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo Check if there is already a mount target in the existing compartment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

},
})
if err != nil {
glog.Errorf("Failed to create a file system options:%#v, %s", options, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glog.Errorf("Failed to create a file system. Options: %#v, %s", options, err)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

.gitignore Outdated
@@ -5,3 +5,6 @@ env.sh
*.log
test/system/venv/
test/system/run-test-image.yaml*
examples/*.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a strange .gitignore rule

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll modify it to point to the templates folder. If the test are run locally and interrupted half way through, the generated yaml files are not being deleted

@@ -92,6 +91,14 @@ func main() {
glog.Fatal("env variable NODE_NAME must be set so that this provisioner can identify itself")
}

// Decides what type of provider to deploy, either block or fss
provisionerType := os.Getenv("PROVISIONER_TYPE")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we decided that we were going to run both provisioners by default and allow disabling specific provisioners behind a command line flag (see: here).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke to @owainlewis and he was more in favour of having separate provisioners

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss offline then

"github.com/oracle/oci-go-sdk/core"
"github.com/oracle/oci-go-sdk/filestorage"
"github.com/oracle/oci-go-sdk/identity"
"github.com/oracle/oci-volume-provisioner/pkg/oci/client"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import grouping

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// See the License for the specific language governing permissions and
// limitations under the License.

package helpers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Packages like "util" and "helpers" are generally to be avoided where possible. Generally mocks should be colocated with the packages that they are mocking which should remove the need for this pkg.

Copy link
Member Author

@MadalinaPatrichi MadalinaPatrichi Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to reuse it across provisioners unit tests. I moved it within the provisioners package and removed helpers

"github.com/oracle/oci-volume-provisioner/pkg/provisioner/plugin"
)

const (
// ProvisionerNameBlock name to deploy block provisioner by
ProvisionerNameBlock = "oracle.com/oci"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block should be the default method of provisioning, however, we should update the name and explicitly implement defaulting.

"github.com/oracle/oci-volume-provisioner/pkg/helpers"
)

func TestCreateVolumeWithFSS(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need more unit tests

if err != nil {
t.Fatalf("Failed to provision volume from block storage: %v", err)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\n

}

// getMountTargetFromID retrieves mountTarget from given mountTargetID
func getMountTargetFromID(ctx context.Context, mountTargetID string, fileStorageClient client.FileStorage) (*filestorage.MountTarget, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than pass in the client I'd make this a private method on the struct

glog.Infof("Deleting export for filesystemID %v", filesystemID)
if _, err := filesystem.client.FileStorage().DeleteExport(ctx, filestorage.DeleteExportRequest{
ExportId: &exportID,
}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to check if the error is that it doesn't exist and continue if that's the case. Otherwise Delete() isn't idempotent.

_, err := filesystem.client.FileStorage().DeleteFileSystem(ctx, filestorage.DeleteFileSystemRequest{
FileSystemId: &filesystemID,
})
return err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above re checking if it's a does not exist error.

@oracle oracle deleted a comment from MadalinaPatrichi Aug 13, 2018
@oracle oracle deleted a comment from MadalinaPatrichi Aug 13, 2018
glog.Infof("Deleting export for filesystemID %v", filesystemID)
if _, err := fsp.client.FileStorage().DeleteExport(ctx, filestorage.DeleteExportRequest{
ExportId: &exportID,
}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs check for not found error (see: here in ccm)

@@ -0,0 +1,134 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backupVolSystemTest.py -> backup_vol_system_test.py

@@ -0,0 +1,54 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blockSystemTest.py -> block_system_test.py

@@ -0,0 +1,48 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

canaryMetrics.py -> canary_metrics.py

@@ -0,0 +1,88 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fssSystemTest.py -> fss_system_test.py

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goes against everything I believe in, but, okay ... haha

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filesystemID, ok := volume.Annotations[ociVolumeID]
if !ok {
return fmt.Errorf("%q annotation not found on PV", ociVolumeID)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Break up w/ \n

if !ok {
return fmt.Errorf("%q annotation not found on PV", ociVolumeID)
}
ctx, cancel := context.WithTimeout(fsp.client.Context(), fsp.client.Timeout())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're sharing the timeout across multiple requests here which is probably not what you want.

}

func (fsp *filesystemProvisioner) Provision(options controller.VolumeOptions, ad *identity.AvailabilityDomain) (*v1.PersistentVolume, error) {
ctx, cancel := context.WithTimeout(fsp.client.Context(), fsp.client.Timeout())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're sharing the timeout here across multiple requests which probably isn't what you want.

return nil, fmt.Errorf("failed to find server IDs associated with the mount target")
}
privateIPID := target.PrivateIpIds[rand.Int()%len(target.PrivateIpIds)]
virtualNetworkClient := fsp.client.VirtualNetwork()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of assigning a local variable rename fsp.client.VirtualNetwork() to fsp.client.VCN() and just use directly?

ctx, cancel := context.WithTimeout(fsp.client.Context(), fsp.client.Timeout())
defer cancel()

fileStorageClient := fsp.client.FileStorage()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of assigning to a local variable maybe rename to fsp.client.FSS() and use directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that we're instantiating so many FSS clients should not have a huge effect on memory usage or anything else, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fsp.client.FSS() should just return a pointer to an already initialised client so will be nice and cheap.

@MadalinaPatrichi MadalinaPatrichi force-pushed the mp/fileStorageNew branch 4 times, most recently from 2b5f553 to e5437aa Compare August 15, 2018 09:09
-Correctly mock out FileStorageClient
-Start separate volume provisioner for fss case _ start of impl of list mount target
-Add check for listing mount targets + add .yaml files to gitignore
-Remove reference to non existing pvc and update the oci provisioner fss config
-Retrieve pirvate ip for mount target and fixed system tests
-Ignore linting mock_interfaces
-Fix for config file for creating rc with volume claim
@MadalinaPatrichi MadalinaPatrichi changed the title Dedicated FSS Dynamic Dedicated FSS Aug 15, 2018
Copy link
Contributor

@prydie prydie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly there 👍

return mtResponse.Items, nil
}

func (fsp *filesystemProvisioner) getOrCreateMountTarget(ad *string, subnetID *string, compartmentID *string) (*filestorage.MountTarget, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't pass pointers to strings around; grab their values from responses and then use those as we don't want to spread the SDKs anti-patterns.

// ProvisionerClient is passed to all sub clients to provision a volume
type ProvisionerClient interface {
BlockStorage() BlockStorage
Identity() Identity
FSS() FSS
VCN() VCN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
mtResponse.Items = append(mtResponse.Items, resp.Items...)
if resp.OpcNextPage != nil {
return fsp.listAllMountTargets(ad, mtResponse)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a for-loop based pagination implementation is a bit cleaner (see: here in ccm).

return mtResponse.Items, nil
}

func (fsp *filesystemProvisioner) getOrCreateMountTarget(mtID *string, ad *string, subnetID *string) (*filestorage.MountTarget, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't pass around pointers to primitives. We don't want the sdk's anti-patterns to seep too far into our code.

// See the License for the specific language governing permissions and
// limitations under the License.

package filestorage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably name the package fss as multi-word names aren't encouraged in go and missing out the "system" somewhat mischaracterises the package.

}

// getMountTargetFromID retrieves mountTarget from given mountTargetID
func (fsp *filesystemProvisioner) getMountTargetFromID(mountTargetID *string) (*filestorage.MountTarget, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't pass around pointers to primitives

// Mount target already specified in the configuration file, find it in the list of mount targets
return fsp.getMountTargetFromID(mtID)
}
ctx, cancel := context.WithTimeout(fsp.client.Context(), fsp.client.Timeout())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant

return &resp.MountTarget, nil
}

func (fsp *filesystemProvisioner) Provision(options controller.VolumeOptions, ad *identity.AvailabilityDomain) (*v1.PersistentVolume, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

func (fsp *filesystemProvisioner) Provision(options controller.VolumeOptions, ad *identity.AvailabilityDomain) (*v1.PersistentVolume, error) {
	// Create the FileSystem.
	var fsID string
	{
		ctx, cancel := context.WithTimeout(fsp.client.Context(), fsp.client.Timeout())
		defer cancel()
		resp, err := fsp.client.FSS().CreateFileSystem(ctx, filestorage.CreateFileSystemRequest{
			CreateFileSystemDetails: filestorage.CreateFileSystemDetails{
				AvailabilityDomain: ad.Name,
				CompartmentId:      common.String(fsp.client.CompartmentOCID()),
				DisplayName:        common.String(fmt.Sprintf("%s%s", os.Getenv(volumePrefixEnvVarName), options.PVC.Name)),
			},
		})
		if err != nil {
			glog.Errorf("Failed to create a file system options=%#v: %v", options, err)
			return nil, err
		}
		fsID = *resp.FileSystem.Id
	}

	target, err := fsp.getOrCreateMountTarget(common.String(options.Parameters[mntTargetID]), ad.Name, common.String(options.Parameters[subnetID]))
	if err != nil {
		glog.Errorf("Failed to retrieve mount target: %s", err)
		return nil, err
	}

	glog.V(6).Infof("Creating export set")

	// Creaete the Export.
	var exportSetID string
	{
		ctx, cancel := context.WithTimeout(fsp.client.Context(), fsp.client.Timeout())
		defer cancel()
		resp, err := fsp.client.FSS().CreateExport(ctx, filestorage.CreateExportRequest{
			CreateExportDetails: filestorage.CreateExportDetails{
				ExportSetId:  target.ExportSetId,
				FileSystemId: &fsID,
				Path:         common.String("/" + fsID),
			},
		})
		if err != nil {
			glog.Errorf("Failed to create export: %v", err)
			return nil, err
		}
		exportSetID = *resp.Export.Id
	}

	if len(target.PrivateIpIds) == 0 {
		glog.Errorf("Failed to find server IDs associated with the Mount Target (OCID %q) to provision a persistent volume", *target.Id)
		return nil, errors.Errorf("failed to find server IDs associated with the Mount Target with OCID %q", *target.Id)
	}

	// Get PrivateIP.
	var serverIP string
	{
		ctx, cancel := context.WithTimeout(fsp.client.Context(), fsp.client.Timeout())
		defer cancel()
		id := target.PrivateIpIds[rand.Int()%len(target.PrivateIpIds)]
		getPrivateIPResponse, err := fsp.client.VCN().GetPrivateIp(ctx, core.GetPrivateIpRequest{
			PrivateIpId: &id,
		})
		if err != nil {
			glog.Errorf("Failed to retrieve IP address for mount target privateIpID=%q: %v", id, err)
			return nil, err
		}
		serverIP = *getPrivateIPResponse.PrivateIp.IpAddress
	}

	glog.Infof("Creating persistent volume on mount target with private IP address %q", serverIP)

	return &v1.PersistentVolume{
		ObjectMeta: metav1.ObjectMeta{
			Name: fsID,
			Annotations: map[string]string{
				ociVolumeID: fsID,
				ociExportID: exportSetID,
			},
			Labels: map[string]string{},
		},
		Spec: v1.PersistentVolumeSpec{
			PersistentVolumeReclaimPolicy: options.PersistentVolumeReclaimPolicy,
			AccessModes:                   options.PVC.Spec.AccessModes,
			//FIXME: fs storage doesn't enforce quota, capacity is meaningless here.
			Capacity: v1.ResourceList{
				v1.ResourceName(v1.ResourceStorage): options.PVC.Spec.Resources.Requests[v1.ResourceName(v1.ResourceStorage)],
			},
			PersistentVolumeSource: v1.PersistentVolumeSource{
				NFS: &v1.NFSVolumeSource{
					// Randomnly select IP address associated with the mount target to use for attachment
					Server:   serverIP,
					Path:     "/" + fsID,
					ReadOnly: false,
				},
			},
		},
	}, nil
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, looks much better!

func (fsp *filesystemProvisioner) Delete(volume *v1.PersistentVolume) error {
exportID, ok := volume.Annotations[ociExportID]
if !ok {
return fmt.Errorf("%q annotation not found on PV", ociExportID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use pkg/errors's errors.Errorf so we retain the stacktrace.


filesystemID, ok := volume.Annotations[ociVolumeID]
if !ok {
return fmt.Errorf("%q annotation not found on PV", ociVolumeID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use pkg/errors's errors.Errorf so we retain the stacktrace.

-Added unit test for mount target ocid specified
-Rebased from master and refactored provisioner deployment
@MadalinaPatrichi MadalinaPatrichi merged commit 00cc911 into master Aug 15, 2018
@prydie prydie deleted the mp/fileStorageNew branch September 4, 2018 10:35
rjtsdl pushed a commit to rjtsdl/oci-volume-provisioner that referenced this pull request Dec 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants