Adds utility to reuse a single PowerShell session #1775

venilnoronha · 2017-08-18T00:33:14Z

Description

This PR adds the powershell utility that reuses a single session to execute commands over the plugin's lifetime.

Tests

The test-e2e-windows target passes locally, and following is the tail'd output.

2017/08/18 00:02:38 Creating volume [option_volume_247591] with options [ -o access=read-write] on VM [10.162.24.4]
2017/08/18 00:04:31 Checking volume [clone_src_volume_582587 option_volume_917152 option_volume_363414 option_volume_867138 option_volume_259466 option_volume_886045 option_volume_461698 option_volume_571036 option_volume_842558 option_volume_327807 option_volume_247591] availability from VM [10.162.24.4]
2017/08/18 00:04:32 END: VolumeCreateTestSuite.TestValidOptions
2017/08/18 00:04:32 Destroying volume [clone_src_volume_582587 option_volume_917152 option_volume_363414 option_volume_867138 option_volume_259466 option_volume_886045 option_volume_461698 option_volume_571036 option_volume_842558 option_volume_327807 option_volume_247591]
OK: 5 passed
--- PASS: Test (318.41s)
PASS
ok  	github.com/vmware/vsphere-storage-for-docker/tests/e2e	318.415s
make: Leaving directory `/go/src/github.com/vmware/vsphere-storage-for-docker/client_plugin'

msterin · 2017-08-18T00:52:44Z

what's the impact on time consumption ?

shaominchen · 2017-08-18T01:00:07Z

client_plugin/utils/fs/fs_windows.go

-	powershell           = "powershell"
-	diskNotFound         = "DiskNotFound"
+	// maxDiskAttachWaitSec is the max time to wait for a disk to be attached.
+	// TODO: Reduce disk attach wait time once parallel disk identification


Do you plan to address this TODO in a follow-up PR? If not, can you please file an issue?

I've filed an issue in #1780.

I've updated the comment in 0653f74.

Addressing via #1991

shaominchen · 2017-08-18T01:11:53Z

client_plugin/utils/powershell/powershell.go

+	if err != nil {
+		log.WithField("err", err).Fatal("Failed to create a PowerShell session")
+		fmt.Println("Failed to create a PowerShell session")
+		os.Exit(1)


Can this error be a temporary issue that can be recovered later? If the answer is yes, we should not exit the plugin. Instead, we can log the error, and retry in the next attempt. That means you may need to move this out of the init function.

Since this is happening in init, the Windows service itself will fail to start. Therefore, I suggest we still keep this as is.

pshahzeb

Looks Good. See comments inline.

pshahzeb · 2017-08-18T21:32:17Z

client_plugin/utils/fs/device_watcher_windows.go

 		default:
-			log.WithFields(log.Fields{"err": err, "out": string(out)}).Warn("Couldn't emit error, continuing.. ")
+			log.WithFields(log.Fields{"err": err, "stdout": stdout,


log.Fields{"err": err
Is this gonna work in case where err isn't valid?

There's a err != nil check, and it would print nil even if that wasn't in place.

pshahzeb · 2017-08-18T21:40:34Z

client_plugin/utils/powershell/powershell.go

+	"sync"
+
+	log "github.com/Sirupsen/logrus"
+	ps "github.com/gorillalabs/go-powershell"


A bit concerned about this usage. The last checkin was on Feb 6th. What other alternatives do we have?

Mark has approved use of that package. The package is very small in itself, so it shouldn't be of too much concern.

This commit adds the powershell utility that reuses a single session to execute commands over the plugin's lifetime.

venilnoronha · 2017-08-18T22:17:23Z

@msterin: @pshahzeb will be posting the stats.

venilnoronha self-assigned this Aug 18, 2017

venilnoronha requested review from msterin, shaominchen and pshahzeb August 18, 2017 00:33

vmwclabot added the cla-not-required label Aug 18, 2017

shaominchen reviewed Aug 18, 2017

View reviewed changes

venilnoronha mentioned this pull request Aug 18, 2017

Skylight performance improvement - Investigate to reduce disk-attach wait time in the Windows plugin #1780

Closed

shaominchen approved these changes Aug 18, 2017

View reviewed changes

pshahzeb reviewed Aug 18, 2017

View reviewed changes

shuklanirdesh82 force-pushed the master branch from 60f4a50 to ee3ef33 Compare August 18, 2017 21:53

venilnoronha force-pushed the powershell-util branch from 0653f74 to 6eef93c Compare August 18, 2017 22:03

venilnoronha added 2 commits August 18, 2017 15:08

Adds powershell utility

c97a243

This commit adds the powershell utility that reuses a single session to execute commands over the plugin's lifetime.

Updates todo comment

53d5e13

venilnoronha force-pushed the powershell-util branch from 6eef93c to 53d5e13 Compare August 18, 2017 22:08

pshahzeb approved these changes Aug 18, 2017

View reviewed changes

venilnoronha merged commit ff1d8d4 into vmware-archive:master Aug 19, 2017

shuklanirdesh82 mentioned this pull request Nov 16, 2017

Reduce disk-attach wait time in the Windows plugin (fixes #1780) #1991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds utility to reuse a single PowerShell session #1775

Adds utility to reuse a single PowerShell session #1775

venilnoronha commented Aug 18, 2017

msterin commented Aug 18, 2017

shaominchen Aug 18, 2017

venilnoronha Aug 18, 2017

venilnoronha Aug 18, 2017

shuklanirdesh82 Nov 16, 2017

shaominchen Aug 18, 2017

venilnoronha Aug 18, 2017

pshahzeb left a comment

pshahzeb Aug 18, 2017

venilnoronha Aug 18, 2017

pshahzeb Aug 18, 2017

venilnoronha Aug 18, 2017

venilnoronha commented Aug 18, 2017

Adds utility to reuse a single PowerShell session #1775

Adds utility to reuse a single PowerShell session #1775

Conversation

venilnoronha commented Aug 18, 2017

Description

Tests

msterin commented Aug 18, 2017

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

pshahzeb 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

venilnoronha commented Aug 18, 2017