Add OCI provider [7/7] #8541

gabriel-samfira · 2018-03-27T13:57:26Z

Adds storage implementation.

Depends on: #8540

jameinel · 2018-06-26T08:51:18Z

Does this depend on the Plug collection patch? Is there an order that we should review them?

jameinel

some early comments as we get started reviewing this

jameinel · 2018-06-26T08:59:59Z

provider/oci/environ_test.go

-		expect.Times(times)
-	}
-}
+// func (e *environSuite) setupListInstancesExpectations(instanceId string, state ociCore.InstanceLifecycleStateEnum, times int) {


commented out code

jameinel · 2018-06-26T09:11:46Z

provider/oci/storage.go

+	// minVolumeSizeInGB represents the minimum size in GiB for
+	// a single volume. For more information please see:
+	// https://docs.oracle.com/cloud/latest/stcomputecs/STCSA/op-storage-volume--post.html#request
+	minVolumeSizeInGB = 50


I realize this is a different storage, but how does this compare with the ones we use for root disk? Can these values be shared, should they be caveated so that it is clear which one is used when?
minRootVolumeSize, etc?
And then its a little odd that one is in MB and one is in GB

jameinel · 2018-06-26T09:26:45Z

provider/oci/storage_volumes.go

+
+func makeVolumeInfo(vol ociCore.Volume) storage.VolumeInfo {
+	var size uint64
+	if vol.SizeInMBs != nil {


The response can have either? or it always has both, or ? Is there a reason to prefer MB? (presumably just better fidelity?)

jameinel · 2018-06-26T09:27:31Z

provider/oci/storage_volumes.go

+			}
+			response, nestedErr := v.storageAPI.DeleteVolume(context.Background(), req)
+			if nestedErr != nil && !v.env.isNotFound(response.RawResponse) {
+				logger.Warningf("failed to cleanup volume: %s", *details.Id)


we should probably also log the nestedErr content, since it is being suppressed.

jameinel · 2018-06-26T09:27:57Z

provider/oci/storage_volumes.go

+				string(ociCore.VolumeLifecycleStateTerminated),
+				5*time.Minute)
+			if nestedErr != nil && !errors.IsNotFound(nestedErr) {
+				logger.Warningf("failed to cleanup volume: %s", *details.Id)


manadart · 2018-06-29T10:38:36Z

There is a race condition here.

WARNING: DATA RACE
Read at 0x00c4201752a0 by goroutine 106:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances.func1()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:647 +0xf8

Previous write at 0x00c4201752a0 by goroutine 108:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances.func1()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:647 +0x197

Goroutine 106 (running) created at:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:644 +0x1ce
  github.com/juju/juju/provider/oci.(*Environ).StopInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:631 +0x1fd
  github.com/juju/juju/provider/oci_test.(*environSuite).TestStopInstancesMultipleFail()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ_test.go:635 +0x2a5
  runtime.call32()
      /snap/go/2130/src/runtime/asm_amd64.s:573 +0x3a
  reflect.Value.Call()
      /snap/go/2130/src/reflect/value.go:308 +0xc0
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:772 +0x954
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:666 +0x89

Goroutine 108 (finished) created at:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:644 +0x1ce
  github.com/juju/juju/provider/oci.(*Environ).StopInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:631 +0x1fd
  github.com/juju/juju/provider/oci_test.(*environSuite).TestStopInstancesMultipleFail()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ_test.go:635 +0x2a5
  runtime.call32()
      /snap/go/2130/src/runtime/asm_amd64.s:573 +0x3a
  reflect.Value.Call()
      /snap/go/2130/src/reflect/value.go:308 +0xc0
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:772 +0x954
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:666 +0x89
==================
==================
WARNING: DATA RACE
Read at 0x00c420298320 by goroutine 106:
  runtime.growslice()
      /snap/go/2130/src/runtime/slice.go:89 +0x0
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances.func1()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:647 +0x376

Previous write at 0x00c420298320 by goroutine 108:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances.func1()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:647 +0x158

Goroutine 106 (running) created at:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:644 +0x1ce
  github.com/juju/juju/provider/oci.(*Environ).StopInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:631 +0x1fd
  github.com/juju/juju/provider/oci_test.(*environSuite).TestStopInstancesMultipleFail()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ_test.go:635 +0x2a5
  runtime.call32()
      /snap/go/2130/src/runtime/asm_amd64.s:573 +0x3a
  reflect.Value.Call()
      /snap/go/2130/src/reflect/value.go:308 +0xc0
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:772 +0x954
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:666 +0x89

Goroutine 108 (finished) created at:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:644 +0x1ce
  github.com/juju/juju/provider/oci.(*Environ).StopInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:631 +0x1fd
  github.com/juju/juju/provider/oci_test.(*environSuite).TestStopInstancesMultipleFail()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ_test.go:635 +0x2a5
  runtime.call32()
      /snap/go/2130/src/runtime/asm_amd64.s:573 +0x3a
  reflect.Value.Call()
      /snap/go/2130/src/reflect/value.go:308 +0xc0
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:772 +0x954
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:666 +0x89
==================
==================
WARNING: DATA RACE
Read at 0x00c420175200 by goroutine 106:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances.func1()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:648 +0x1db

Previous write at 0x00c420175200 by goroutine 108:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances.func1()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:648 +0x279

Goroutine 106 (running) created at:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:644 +0x1ce
  github.com/juju/juju/provider/oci.(*Environ).StopInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:631 +0x1fd
  github.com/juju/juju/provider/oci_test.(*environSuite).TestStopInstancesMultipleFail()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ_test.go:635 +0x2a5
  runtime.call32()
      /snap/go/2130/src/runtime/asm_amd64.s:573 +0x3a
  reflect.Value.Call()
      /snap/go/2130/src/reflect/value.go:308 +0xc0
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:772 +0x954
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:666 +0x89

Goroutine 108 (finished) created at:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:644 +0x1ce
  github.com/juju/juju/provider/oci.(*Environ).StopInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:631 +0x1fd
  github.com/juju/juju/provider/oci_test.(*environSuite).TestStopInstancesMultipleFail()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ_test.go:635 +0x2a5
  runtime.call32()
      /snap/go/2130/src/runtime/asm_amd64.s:573 +0x3a
  reflect.Value.Call()
      /snap/go/2130/src/reflect/value.go:308 +0xc0
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:772 +0x954
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:666 +0x89
==================
==================
WARNING: DATA RACE
Read at 0x00c420298330 by goroutine 106:
  runtime.growslice()
      /snap/go/2130/src/runtime/slice.go:89 +0x0
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances.func1()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:648 +0x309

Previous write at 0x00c420298330 by goroutine 108:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances.func1()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:648 +0x233

Goroutine 106 (running) created at:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:644 +0x1ce
  github.com/juju/juju/provider/oci.(*Environ).StopInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:631 +0x1fd
  github.com/juju/juju/provider/oci_test.(*environSuite).TestStopInstancesMultipleFail()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ_test.go:635 +0x2a5
  runtime.call32()
      /snap/go/2130/src/runtime/asm_amd64.s:573 +0x3a
  reflect.Value.Call()
      /snap/go/2130/src/reflect/value.go:308 +0xc0
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:772 +0x954
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:666 +0x89

Goroutine 108 (finished) created at:
  github.com/juju/juju/provider/oci.(*Environ).terminateInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:644 +0x1ce
  github.com/juju/juju/provider/oci.(*Environ).StopInstances()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ.go:631 +0x1fd
  github.com/juju/juju/provider/oci_test.(*environSuite).TestStopInstancesMultipleFail()
      /workspace/_build/src/github.com/juju/juju/provider/oci/environ_test.go:635 +0x2a5
  runtime.call32()
      /snap/go/2130/src/runtime/asm_amd64.s:573 +0x3a
  reflect.Value.Call()
      /snap/go/2130/src/reflect/value.go:308 +0xc0
  gopkg.in/check%2ev1.(*suiteRunner).forkTest.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:772 +0x954
  gopkg.in/check%2ev1.(*suiteRunner).forkCall.func1()
      /workspace/_build/src/gopkg.in/check.v1/check.go:666 +0x89
==================
OK: 55 passed
--- FAIL: TestPackage (7.80s)
	testing.go:730: race detected during execution of test
FAIL
FAIL	github.com/juju/juju/provider/oci	14.811s

manadart · 2018-06-29T10:47:58Z

The Windows build has also been failing since 6/7 landed. Not sure exactly what is going on here.

go build github.com/oracle/oci-go-sdk/core: C:\tools\go\pkg\tool\windows_amd64\compile.exe: fork/exec C:\tools\go\pkg\tool\windows_amd64\compile.exe: The filename or extension is too long.

gabriel-samfira · 2018-07-02T10:02:03Z

The error message is probably not the real cause of the issue. Investigating.

manadart · 2018-07-02T10:21:38Z

Resolved the Windows error in CI. It's a known Go issue, slated for 1.11 fix:
golang/go#18468

manadart · 2018-07-31T11:34:56Z

provider/oci/storage.go

@@ -39,19 +78,46 @@ func (s *storageProvider) Releasable() bool {
 }

 func (s *storageProvider) DefaultPools() []*storage.Config {
-	return nil
+	iscsiPool, _ := storage.NewConfig("iscsi", ociStorageProviderType, map[string]interface{}{


I'd rename this so it doesn't conflict with the const name.

manadart · 2018-08-27T12:29:04Z

provider/oci/storage_volumes.go

+	return errs, nil
+}
+
+func (v *volumeSource) ReleaseVolumes(ctx envcontext.ProviderCallContext, volIds []string) ([]error, error) {


I know these are interface implementations, with comments on the interface itself, but it would be good to get comments on all these public methods - something to summarise the actions taken. In this case that releasing is disassociating the volumes from the model and controller by removing them from the volume tag collections.

gabriel-samfira · 2018-09-25T13:10:47Z

!!build!!

gabriel-samfira · 2018-09-25T13:56:47Z

!!build!!

manadart · 2018-09-26T08:24:13Z

$$merge$$

gabriel-samfira · 2018-09-26T12:04:17Z

$$merge$$

manadart · 2018-09-26T12:35:45Z

FAIL: storage_volumes_test.go:189: storageVolumeSuite.TestListVolumes

[LOG] 0:00.000 INFO juju.provider.oci opening model "testmodel"
storage_volumes_test.go:200:
    c.Assert(volumes[0], gc.Equals, "fakeVolumeId")
... obtained string = "fakeVolumeId2"
... expected string = "fakeVolumeId"

!!build!!

gabriel-samfira · 2018-09-26T13:45:10Z

Yeah, appears the order in which the API returns is not always the same. Replaced with a deepequals. Current failure seems something else.

gabriel-samfira · 2018-09-26T14:51:44Z

$$merge$$

gabriel-samfira · 2018-09-26T16:04:14Z

$$merge$$

gabriel-samfira changed the title ~~Add OCI provider [7/8]~~ Add OCI provider [7/7] Mar 27, 2018

gabriel-samfira force-pushed the oci-provider-storage branch 6 times, most recently from 2794e54 to 1c23b6c Compare April 11, 2018 15:09

gabriel-samfira force-pushed the oci-provider-storage branch 7 times, most recently from 91b7e57 to a6c7d93 Compare April 26, 2018 12:09

gabriel-samfira force-pushed the oci-provider-storage branch 3 times, most recently from 55a3d72 to cab8b62 Compare May 16, 2018 14:29

gabriel-samfira force-pushed the oci-provider-storage branch from cab8b62 to cd15550 Compare May 17, 2018 12:08

jameinel reviewed Jun 26, 2018

View reviewed changes

gabriel-samfira force-pushed the oci-provider-storage branch from cd15550 to c08ac23 Compare July 3, 2018 12:04

manadart reviewed Jul 31, 2018

View reviewed changes

gabriel-samfira force-pushed the oci-provider-storage branch 2 times, most recently from c98d856 to 4e624b5 Compare August 3, 2018 14:48

manadart reviewed Aug 27, 2018

View reviewed changes

gabriel-samfira force-pushed the oci-provider-storage branch from 4e624b5 to e93f8af Compare September 20, 2018 09:56

gabriel-samfira force-pushed the oci-provider-storage branch from e93f8af to 9fcceaf Compare September 25, 2018 13:29

gabriel-samfira force-pushed the oci-provider-storage branch 2 times, most recently from b0982e1 to a07d063 Compare September 25, 2018 14:45

gabriel-samfira force-pushed the oci-provider-storage branch from a07d063 to c5bcf1e Compare September 26, 2018 11:31

gabriel-samfira force-pushed the oci-provider-storage branch from c5bcf1e to f968abd Compare September 26, 2018 15:25

Adds storage implementation

30716fd

gabriel-samfira force-pushed the oci-provider-storage branch from f968abd to 30716fd Compare September 26, 2018 15:35

jujubot merged commit a7c9e43 into juju:develop Sep 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OCI provider [7/7] #8541

Add OCI provider [7/7] #8541

gabriel-samfira commented Mar 27, 2018

jameinel commented Jun 26, 2018

jameinel left a comment

jameinel Jun 26, 2018

jameinel Jun 26, 2018

jameinel Jun 26, 2018

jameinel Jun 26, 2018

jameinel Jun 26, 2018

manadart commented Jun 29, 2018

manadart commented Jun 29, 2018

gabriel-samfira commented Jul 2, 2018

manadart commented Jul 2, 2018

manadart Jul 31, 2018

manadart Aug 27, 2018

gabriel-samfira commented Sep 25, 2018

gabriel-samfira commented Sep 25, 2018

manadart commented Sep 26, 2018

gabriel-samfira commented Sep 26, 2018

manadart commented Sep 26, 2018

gabriel-samfira commented Sep 26, 2018

gabriel-samfira commented Sep 26, 2018

gabriel-samfira commented Sep 26, 2018

Add OCI provider [7/7] #8541

Add OCI provider [7/7] #8541

Conversation

gabriel-samfira commented Mar 27, 2018

jameinel commented Jun 26, 2018

jameinel left a comment

Choose a reason for hiding this comment

jameinel Jun 26, 2018

Choose a reason for hiding this comment

jameinel Jun 26, 2018

Choose a reason for hiding this comment

jameinel Jun 26, 2018

Choose a reason for hiding this comment

jameinel Jun 26, 2018

Choose a reason for hiding this comment

jameinel Jun 26, 2018

Choose a reason for hiding this comment

manadart commented Jun 29, 2018

manadart commented Jun 29, 2018

gabriel-samfira commented Jul 2, 2018

manadart commented Jul 2, 2018

manadart Jul 31, 2018

Choose a reason for hiding this comment

manadart Aug 27, 2018

Choose a reason for hiding this comment

gabriel-samfira commented Sep 25, 2018

gabriel-samfira commented Sep 25, 2018

manadart commented Sep 26, 2018

gabriel-samfira commented Sep 26, 2018

manadart commented Sep 26, 2018

gabriel-samfira commented Sep 26, 2018

gabriel-samfira commented Sep 26, 2018

gabriel-samfira commented Sep 26, 2018