Catalog upload feature2 #98

vbauzys · 2018-09-25T13:13:07Z

Second part of missing functionality Related issue: (#50)

Enhanced upload functionality for catalog item. Returns special upload task which allows monitor upload if required and later can wait until vCD import task finishes. Upload happens in background. Other improvements:
Added Missing tests
Code improvements
Removed lock to xml first element
Better error handling
Verified and improved that "sanitizedName" func is enough to cover security issue -> See vmware-archive/pyvcloud#268.
Use new logging system
Error handling return within xml of response
Checked and parse errors when vCD fails after ovf file upload.
Multiupload verified.
Report percentage progress.
Check against nil catalog
Files unarchived to temp folder and cleaned
Checked files: from the tarball header with the size we have extracted
Make upload chunking working as function parameter
Specific check to avoid cryptic error then item already exist.

Note: Exist one question how to handle between AdminCatalog and Catalog object if needed. Current implementation allows working from Catalog object perspective.

hodgesrm

Overall looks good but needs more commentary. I think the biggest requirement is to explain the upload protocol in an accessible way as it's not a simple process and not everyone will have it in their heads when working on this code.

Test cases are great. Thanks taking the care to put them in.

hodgesrm · 2018-09-26T04:42:30Z

govcd/catalog.go

@@ -125,7 +128,7 @@ type Envelope struct {
 	} `xml:"References>File"`
 }

-// If catalogitem is a valid CatalogItem and the call succeds,
+// If catalog item is a valid CatalogItem and the call succeeds,
 // then the function returns a CatalogItem. If the item does not
 // exist, then it returns an empty CatalogItem. If The call fails


Nit: 'If The call' should be 'If the call'

hodgesrm · 2018-09-26T04:43:32Z

govcd/catalog.go

@@ -163,37 +166,47 @@ func (cat *Catalog) FindCatalogItem(catalogitem string) (CatalogItem, error) {

 // uploads an ova file to a catalog. This method only uploads bits to vCD spool area.


Leading sentence should be capitalized.

hodgesrm · 2018-09-26T04:44:18Z

govcd/catalog.go

@@ -163,37 +166,47 @@ func (cat *Catalog) FindCatalogItem(catalogitem string) (CatalogItem, error) {

 // uploads an ova file to a catalog. This method only uploads bits to vCD spool area.
 // Returns errors if any occur during upload from vCD or upload process.


If a catalog upload fails does the client program have to do any cleanup?

Expanded comment.

hodgesrm · 2018-09-26T04:46:10Z

govcd/catalog.go

+
+	for _, catalogItemName := range getExistingCatalogItems(cat) {
+		if catalogItemName == itemName {
+			return UploadTask{}, fmt.Errorf("catalog item by name: '%s' already exist. Upload with different name", itemName)


Message is not fully grammatical. Suggestion: "Catalog item'%s' already exists. Upload with different name."

hodgesrm · 2018-09-26T04:52:54Z

govcd/catalog.go

+		uploadProgress = (float64(bytesUploaded) / float64(totalSize)) * 100
+	}
+
+	go uploadFiles(cat.client, vappTemplate, &ovfFileDesc, tempPath, filesAbsPaths, uploadPieceSize, callBack, tmpDir)


Can you add a comment to explain the concurrent processing you are doing here? IT's not easy to read out of the code.

hodgesrm · 2018-09-26T05:09:07Z

govcd/catalog_test.go

+	catalog, org := findCatalog(vcd, check, catalogName)
+	AddToCleanupList(itemName, "catalogItem", vcd.org.Org.Name, "Test_UploadOvf")
+
+	//execute


Unnecessary comment. (Same applies to verify below.)

hodgesrm · 2018-09-26T05:12:29Z

govcd/uploadtask.go

@@ -0,0 +1,33 @@
+package govcd


Needs a proper Apache 2 header. Please copy from another file. Also any file you touch should have the copyright date 2018 added.

Added and updated year on files I touched everywhere.

hodgesrm · 2018-09-26T05:13:12Z

govcd/uploadtask.go

+	*Task
+}
+
+func NewUploadTask(task *Task, uploadProgress *float64) *UploadTask {


Please add a comment to document public APIs in this file.

hodgesrm · 2018-09-26T05:15:08Z

util/tar.go

-func Unpack(tarFile string) ([]string, error) {
+const TmpDirPrefix = "govcd"
+
+//extract files to system tmp dir with name govcd+random number. Created folder with files isn't deleted.


What is the output of this function? Also 'extract' should be capitalized.

hodgesrm · 2018-09-26T05:16:40Z

util/tar_test.go

+	"testing"
+)
+
+func TestSanitizedName(t *testing.T) {


Please a comment describing what this test is checking.

dataclouder

The tests should conform to the general structure of the other tests.
As for the binaries in the code directory, I'd move them in a separate repository, but I'll let the team decide what's the best place for them.

dataclouder · 2018-09-26T05:51:02Z

govcd/catalog_test.go

+
+const (
+	OvaPath        = "/test_vapp_template.ova"
+	OvaChunkedPath = "/template_with_custom_chunk_size.ova"


I see two problems with these two files:

binary artifacts should not be in the code directory. Our repo will end up in someone else's vendor directory, and many would object to having non-text files mixed up with code.

test components should be referred to in the configuration file. These two items should be added to the config structure in api_vcd_test.go and used accordingly.

I would agree with https://medium.com/@matryer/5-simple-tips-and-tricks-for-writing-unit-tests-in-golang-619653f90742 and move all test to different package - in that away we can avoid more clutter in future and separate stuff. And solve issue mentioned by you as binary files would be in different package. Otherwise we create issue that test exist but they fail for others if they won't checkout special folder with binary files.

Yes, I can move constants to api_vcd_test.go

We have already tests that depend on user defined artifacts. This test should be no different. It may pass with the artifacts that we recommend, but we must see how it behaves with user defined items.

Move binaries to test-resources. Ova paths are configured if empty, tests are skipped.

dataclouder · 2018-09-26T05:55:12Z

govcd/catalog_test.go

@@ -84,3 +96,179 @@ func (vcd *TestVCD) Test_DeleteCatalog(check *C) {
 	check.Assert(catalog, Equals, Catalog{})

 }
+
+func (vcd *TestVCD) Test_UploadOvf(check *C) {
+	checkUploadOvf(vcd, check, getCurrentPath()+OvaPath, TestCreateCatalog, "item1")


Every item created by the tests should have a name that refers to the function that creates it. See the constants in api_vcd_test.go for some examples.
Thus, "item1" should become "TestUploadOvf", and TestCreateCatalog should not be used. If you need to create a new catalog for this function, it should have a function related name.
Is there a reason not to use the catalog indicated in the test configuration file?

ok, I missed this - will correct it.

dataclouder · 2018-09-26T05:56:58Z

govcd/catalog_test.go

+
+func (vcd *TestVCD) Test_UploadOvf_progress_works(check *C) {
+	catalogName := TestCreateCatalog + "3"
+	itemName := "item3"


See the above comments for the name of the items.
Moreover, the upload tests are creating four catalogs. Why?

It's test independence question. Test can't depend on other tests result (e.g. what id did with catalog in previous test).

If more catalogs are needed, we should name them according to their purposes.
In general, we should make tests that work with the material provided by users (i.e the catalog defined in the configuration file).
We should create new catalogs only when testing something relating to catalog creation or if we run an operation that requires two catalogs. Otherwise, we should always use the catalog given by users.

dataclouder · 2018-09-26T05:58:30Z

govcd/catalog_test.go

+	setupCatalog(vcd, check, catalogName)
+
+	catalog, org := findCatalog(vcd, check, catalogName)
+	AddToCleanupList(itemName, "catalogItem", vcd.org.Org.Name, "Test_UploadOvf")


The cleanup procedure in api_vcd_test.go does not recognize "catalogItem" yet. It should be updated to handle this case.

I checked and think we can avoid it as catalog cleanup allows to delete catalog with items. So removing AddToCleanupList should be sufficient.

No. All the tests are removing their artifacts. The cleanup list is the safety net to remove things when the test fails and aborts prematurely.
We should keep it and update the cleanup function accordingly.

CatalogItem cleanup added.

dataclouder · 2018-09-26T06:00:57Z

govcd/catalog_test.go

+	org := getOrg(vcd, check)
+
+	// creating catalog
+	adminCatalog, err := org.CreateCatalog(testCreateCatalog, TestCreateCatalogDesc, true)


If this test is using an AdminCatalog, it should check whether skipAdminTests is set, and skip the test if it is.

this isn't needed cause tests rung fine with org user or admin.

dataclouder · 2018-10-01T08:00:19Z

govcd/catalog_test.go

+	return org
+}
+
+func skipThenOvaPathMissing(vcd *TestVCD, check *C) {


This should be called skipWhenOvaPathMissing

dataclouder · 2018-10-01T08:02:35Z

govcd/catalog_test.go

+	//setupCatalog(vcd, check, testCreateCatalog)
+
+	catalog, org := findCatalog(vcd, check, vcd.config.VCD.Catalog.Name)
+	AddToCleanupList(itemName, "catalogItem", vcd.org.Org.Name+"|"+vcd.config.VCD.Catalog.Name, "Test_UploadOvf")


The call to AddToCleanupList should only happen after the successful creation of an item.

dataclouder · 2018-10-01T08:13:21Z

govcd/catalog.go

@@ -202,58 +223,94 @@ func (cat *Catalog) UploadOvf(ovaFileName, itemName, description string, chunkSi
 	for _, filePath := range filesAbsPaths {


We are assuming here that the extracted file contains a .ovf.
If this is not true, the procedure hangs. This check needs to happen before the item is created.

dataclouder · 2018-10-01T08:16:51Z

govcd/api_vcd_test.go

+				}
+			}
+		}
+		vcd.infoCleanup(removedMsg, entity.EntityType, entity.Name, entity.CreatedBy)


Something is missing in this procedure. When I use a non existing OVA in the test, it still prints Removed catalogItem '<ITEM_NAME>' created by Test_UploadOvf.

dataclouder · 2018-10-01T08:23:39Z

govcd/catalog.go

 	}

-	filesAbsPaths, err := util.Unpack(ovaFileName)
+	filesAbsPaths, tmpDir, err := util.Unpack(ovaFileName)


The unpack should happen before we create the item. If the unpack fails, the item will stay unused and waiting for an upload that won't come.

lvirbalas

Please merge after implementing Giuseppe's feedback.

dataclouder · 2018-10-02T07:39:18Z

govcd/catalog.go

+// Upload files for vCD created upload links. Different approach then vmdk file are
+// chunked (e.g. test.vmdk.000000000, test.vmdk.000000001 or test.vmdk). vmdk files are chunked if
+// in description file attribute ChunkSize is not zero.
+func uploadFiles(client *Client, vappTemplate *types.VAppTemplate, ovfFileDesc *Envelope, tempPath string, filesAbsPaths []string, uploadPieceSize int64, callBack func(bytesUpload, totalSize int64)) error {


This function has too many positional arguments.
Please add documentation about the meaning of every argument. Consider doing the same for every function that has more than three args.

Signed-off-by: Vaidotas Bauzys <[email protected]>

…t with callback when we add it. Signed-off-by: Vaidotas Bauzys <[email protected]>

Signed-off-by: Vaidotas Bauzys <[email protected]>

…ou can check progress and wait for task completion. Signed-off-by: Vaidotas Bauzys <[email protected]>

Signed-off-by: Vaidotas Bauzys <[email protected]>

…00 without explanation. Signed-off-by: Vaidotas Bauzys <[email protected]>

…errors, like: storage quota exceeded Signed-off-by: Vaidotas Bauzys <[email protected]>

Signed-off-by: Vaidotas Bauzys <[email protected]>

… remove api function and test. Moved binaries to test-resources folder. Signed-off-by: Vaidotas Bauzys <[email protected]>

…valid. Signed-off-by: Vaidotas Bauzys <[email protected]>

Signed-off-by: Vaidotas Bauzys <[email protected]>

… catalog item created on various fails anymore. Signed-off-by: Vaidotas Bauzys <[email protected]>

Signed-off-by: Vaidotas Bauzys <[email protected]>

…happend. Signed-off-by: Vaidotas Bauzys <[email protected]>

Signed-off-by: Vaidotas Bauzys <[email protected]>

vmwclabot added the cla-not-required label Sep 25, 2018

vbauzys requested review from lvirbalas, hodgesrm and dataclouder September 25, 2018 13:13

hodgesrm reviewed Sep 26, 2018

View reviewed changes

dataclouder suggested changes Sep 26, 2018

View reviewed changes

dataclouder reviewed Oct 1, 2018

View reviewed changes

lvirbalas reviewed Oct 1, 2018

View reviewed changes

dataclouder mentioned this pull request Oct 1, 2018

Update or add copyright to all code files #101

Merged

lvirbalas mentioned this pull request Oct 1, 2018

Implement Catalog Item resource vmware/terraform-provider-vcd#93

Closed

dataclouder reviewed Oct 2, 2018

View reviewed changes

vbauzys added 15 commits October 2, 2018 15:06

Initial commit.

5f00c94

Signed-off-by: Vaidotas Bauzys <[email protected]>

Updated logging according latest PR.

fab4087

Signed-off-by: Vaidotas Bauzys <[email protected]>

Added upload file splitting to small parts. Will be possible to use i…

10c5501

…t with callback when we add it. Signed-off-by: Vaidotas Bauzys <[email protected]>

Added progress to be monitored as callback function.

b773ce6

Signed-off-by: Vaidotas Bauzys <[email protected]>

Added Special task for upload monitoring and made upload async. Now y…

15304ba

…ou can check progress and wait for task completion. Signed-off-by: Vaidotas Bauzys <[email protected]>

Small fix to avoid crash then xml not returned full.

6ff0d73

Signed-off-by: Vaidotas Bauzys <[email protected]>

Added support for choosing file part size when uploading ova.

868aa20

Signed-off-by: Vaidotas Bauzys <[email protected]>

Check against not found catalog folder.

f0466fa

Signed-off-by: Vaidotas Bauzys <[email protected]>

Added verifying function to extracted files. Check to match size.

9cdb475

Signed-off-by: Vaidotas Bauzys <[email protected]>

Changed untaring to use system temp dir. And added cleanup after upload.

f7e975a

Signed-off-by: Vaidotas Bauzys <[email protected]>

Added check for already existing catalog item name. As inside error 5…

d456506

…00 without explanation. Signed-off-by: Vaidotas Bauzys <[email protected]>

Add error handling returned from vcd and upload can't happen due vCD …

c9e17c6

…errors, like: storage quota exceeded Signed-off-by: Vaidotas Bauzys <[email protected]>

Logging changes (renaming) after PR merge.

694520f

Signed-off-by: Vaidotas Bauzys <[email protected]>

Improved sanitize function to check more cases and add unit test.

4aa7894

Signed-off-by: Vaidotas Bauzys <[email protected]>

Fixed issue with when sanitized is used.

a950bde

Signed-off-by: Vaidotas Bauzys <[email protected]>

vbauzys added 17 commits October 2, 2018 15:06

A few fixes and added support for multi vmdk/iso support in ova.

63e1702

Signed-off-by: Vaidotas Bauzys <[email protected]>

Improved logging.

9410f4b

Signed-off-by: Vaidotas Bauzys <[email protected]>

Improved code reading.

2ddc75c

Signed-off-by: Vaidotas Bauzys <[email protected]>

Added unitest for general case.

ee3a444

Signed-off-by: Vaidotas Bauzys <[email protected]>

Added unit tests.

96b3130

Signed-off-by: Vaidotas Bauzys <[email protected]>

Cleanup and added missing file.

c4c1784

Signed-off-by: Vaidotas Bauzys <[email protected]>

Fixed various comments according review notes.

ec92ad7

Signed-off-by: Vaidotas Bauzys <[email protected]>

Improved tests to comply with testing infrastructure. Add catalogItem…

26f19e8

… remove api function and test. Moved binaries to test-resources folder. Signed-off-by: Vaidotas Bauzys <[email protected]>

Added one more check to not initiate item creation if the file isn't …

89c4aa4

…valid. Signed-off-by: Vaidotas Bauzys <[email protected]>

Missing changes.

80db52e

Signed-off-by: Vaidotas Bauzys <[email protected]>

Changes according PR review. Improved handling of fail cases. Now vCD…

d771ec6

… catalog item created on various fails anymore. Signed-off-by: Vaidotas Bauzys <[email protected]>

Added additional checks to verify ova content before uploading it.

60ed05f

Signed-off-by: Vaidotas Bauzys <[email protected]>

Added possibility to remove upload task in vCD when unexpected error …

80f4e63

…happend. Signed-off-by: Vaidotas Bauzys <[email protected]>

Added comments, small fix.

8776001

Signed-off-by: Vaidotas Bauzys <[email protected]>

Removed unneeded code.

870a318

Signed-off-by: Vaidotas Bauzys <[email protected]>

Changed test cleanup message.

475a56e

Signed-off-by: Vaidotas Bauzys <[email protected]>

Fixed test.

c5cb8de

Signed-off-by: Vaidotas Bauzys <[email protected]>

dataclouder approved these changes Oct 2, 2018

View reviewed changes

vbauzys merged commit 7ff4554 into vmware:master Oct 2, 2018

		@@ -163,37 +166,47 @@ func (cat *Catalog) FindCatalogItem(catalogitem string) (CatalogItem, error) {

		// uploads an ova file to a catalog. This method only uploads bits to vCD spool area.

		@@ -163,37 +166,47 @@ func (cat *Catalog) FindCatalogItem(catalogitem string) (CatalogItem, error) {

		// uploads an ova file to a catalog. This method only uploads bits to vCD spool area.
		// Returns errors if any occur during upload from vCD or upload process.

		@@ -202,58 +223,94 @@ func (cat *Catalog) UploadOvf(ovaFileName, itemName, description string, chunkSi
		for _, filePath := range filesAbsPaths {

Catalog upload feature2 #98

Catalog upload feature2 #98

Conversation

vbauzys commented Sep 25, 2018 • edited Loading

hodgesrm left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbauzys Sep 26, 2018 • edited Loading

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

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

vbauzys Sep 26, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dataclouder left a comment

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lvirbalas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbauzys commented Sep 25, 2018 •

edited

Loading

vbauzys Sep 26, 2018 •

edited

Loading

vbauzys Sep 26, 2018 •

edited

Loading