Add support for ARM shapes on Oracle OCI #16277

nvinuesa · 2023-09-18T13:01:30Z

By correctly parsing the version of the ubuntu cloud images returned by OCI's API, we can then create different bases for ubuntu aarch64 (the cloud image needed to target arm-based shapes).

It is important to note that we will not be supporting Minimal aarch64 ubuntu images, based on what is documented for using ARM-based shapes: https://docs.oracle.com/en-us/iaas/Content/Compute/References/images.htm

...For Arm-based shapes, use the Ubuntu image, not Minimal Ubuntu.

Bonus: By moving the retrieve of instanceTypes() after we correctly parsed the InstanceImage, we avoid useless calls to the API to retrieve the shapes for images that we don't support anyways.

Bonus 2: Fixed the Makefile for building cross-compiled versions (thx to @hmlanigan and @tlm).

Bonus 3: Fixed the TestRefreshImageCacheWithInvalidImage test which had incorrect logic.

Checklist

Code style: imports ordered, good names, simple structure, etc
Comments saying why design decisions were made
Go unit tests, with comments saying what you're testing
~~Integration tests, with comments saying what you're testing~~
~~doc.go added or updated in changed packages~~

QA steps

Build

rm -rf _build/
make go-install
GOOS=linux GOARCH=arm64 make go-build
AGENT_PACKAGE_PLATFORMS=linux/arm64 make simplestreams
export JUJU_METADATA_SOURCE="/home/nicolas/workspace/canonical/juju/_build/simplestreams"

Bootstrap and add model

juju bootstrap --debug --constraints arch=arm64 --config compartment-id=ocid1.compartment.oc1..aaaaaaaanvu2racnlczevenu73dlcf3nokgsjpdkbdgp4xbrz3lb2y4giyjq oci-canonical c
juju add-model m

Add machines

juju add-machine --constraints "arch=arm64"
juju add-machine --constraints "arch=arm64 cores=3 mem=11G"

If you log in the OCI dashboard, you should see 3 VM.Standard.A1.Flex instances (one for the controller, two for the added machines) with 1 Ocpu for two of them and 3 Ocpus/ 11GB for the third one.

Links

Jira card: JUJU-4514

hmlanigan

Images supporting Ubuntu 22.04 with amd64 are getting lost in the processing somewhere, not sure where: Image ocid1.image.oc1.phx.aaaaaaaapj3tk2to46hrihgab6aytk3xfv5tcdbdms4k2ixzoz64zxlpmjfq with String Canonical-Ubuntu-22.04-aarch64-2023.08.23 is being parsed as Canonical Ubuntu 22.04 per MM yesterday

Makefile

hmlanigan · 2023-09-18T13:54:23Z

provider/oci/images.go

+// Specifically, for ubuntu, we need to create a different base when the image
+// name contains "aarch64", which are the images that target ARM-based shapes.
+func NewInstanceImage(img ociCore.Image, compartmentID *string) (InstanceImage, error) {
+	logger.Warningf("*** CREATING INSTANCE IMAGE FROM '%s'//'%s'", *img.OperatingSystem, *img.OperatingSystemVersion)


Make this a trace or debug, it's noise as a warning

sure, I had forgotten to clean up after debugging, done

hmlanigan · 2023-09-18T13:55:42Z

provider/oci/images.go

-	// This code is very brittle. It's highly dependent on the strings currently
-	// returned by the oracle sdk. The best option is to have
-	// PlatformConfigOptions as they have types we can rely on.


We need a warning like this for developers somewhere.

but we are no longer using PlatformConfigOptions, what comment should we keep from that?

hmlanigan · 2023-09-18T18:43:10Z

provider/oci/images.go

+	UbuntuMinimalBase = "ubuntu-minimal"
+	UbuntuARMBase     = "ubuntu-" + arch.ARM64


These are not a valid OS in a Base in juju proper, but an Oracle implementation detail. If they leak out of the provider into state, juju won't be able to handle them appropriately.

I changed the logic, this is gone now

hmlanigan · 2023-09-18T18:45:09Z

provider/oci/images.go

+	UbuntuPostfix               UbuntuVersionPostfix = ""
+	UbuntuMinimalPostfix        UbuntuVersionPostfix = "Minimal"
+	UbuntuAARCH64Postfix        UbuntuVersionPostfix = "aarch64"
+	UbuntuMinimalAARCH64Postfix UbuntuVersionPostfix = "Minimal aarch64"


UbuntuMinimalAARCH64Postfix is unused.

cleaned it up, done

hmlanigan · 2023-09-18T18:46:59Z

provider/oci/images.go

@@ -116,6 +127,21 @@ func (i *InstanceImage) SetInstanceTypes(types []instances.InstanceType) {
 	i.InstanceTypes = types
 }

+// UniqueArchs returns the list of unique supported architectures by the
+// InstanceImage. This is necessary because an InstanceImage can support
+// multiple shapes, and therefore supporting multiple architectures.


"and therefore supporting multiple architectures." is incorrect for oracle.

oh yes, I removed it thanks

hmlanigan · 2023-09-18T18:51:44Z

provider/oci/images.go

+	uniqueVals := make(map[string]bool)
+	var archs []string
+	for _, val := range i.InstanceTypes {
+		if !uniqueVals[val.Arch] {
+			archs = append(archs, val.Arch)
+		}
+		uniqueVals[val.Arch] = true
+	}
+	return archs


set.Strings from github.com/juju/collections does this for you, nor does it allow you to have duplicate strings.

Suggested change

uniqueVals := make(map[string]bool)

var archs []string

for _, val := range i.InstanceTypes {

if !uniqueVals[val.Arch] {

archs = append(archs, val.Arch)

}

uniqueVals[val.Arch] = true

}

return archs

archs := set.NewStrings()

for _, val := range i.InstanceTypes {

arches.Add(val.Arch)

}

return archs.Values()

oh nice! done

hmlanigan · 2023-09-18T20:02:10Z

provider/oci/environ.go

+	// Since the images for ARM machine will include "aarch64" on their
+	// name, we need to make sure we select the correct base that was
+	// taken from the parsed image name.
+	if arch == corearch.ARM64 {
+		args.InstanceConfig.Base.OS = UbuntuARMBase
+	}
+


I have concerns about overloading the defined juju base.OS values with architecture details and the values ending up in state. Nor am I clear how this interacts with the architecture constraints.

What about updating imgCache.SupportedShapes to take an architecture value as well. The actual imgCache is local to the Oracle provider.

indeed, this was a very bad idea. I changed the implementation, now we pass the architecture as parameter to the image functions. Also, we keep track of the image's architecture on the cache so we can select them using the passed arch parameter.

hmlanigan · 2023-09-18T20:03:53Z

provider/oci/images.go

-// normaliseArchAndInstType returns the Juju architecture and instance type
-// corresponding to a shape's reported architecture and instance type based
-// off ShapePlatformConfigOptionsTypeEnum.
-func normaliseArchAndInstType(val ociCore.ShapePlatformConfigOptionsTypeEnum) (string, string) {


You still need to handle the InstanceTypes here, VM, BM etc for use.

this is done in instTypeByShapeName() which I didn't touch

hmlanigan · 2023-09-21T19:47:46Z

@nvinuesa once a review has started, please add new commits with changes rather than force pushing an amended commit (excepting rebases). It makes it more difficult for reviewers. They can be squashed before merge.

hmlanigan · 2023-09-22T13:00:10Z

This PR is blocked while @nvinuesa investigates improved image and instance modeling for juju where Flexible instances exist in a cloud.

#16357 In the context of adding ARM support for Oracle OCI (#16277), we must fist support oracle flexible shapes because the only ARM shape on OCI is a flexible one (https://docs.oracle.com/en-us/iaas/Content/Compute/References/computeshapes.htm#flexible). In order to support flexible (https://docs.oracle.com/en-us/iaas/Content/Compute/References/computeshapes.htm#flexible) shapes we must pass some configuration `ShapeConfig` (at least the number of cpu-cores) at the moment of launching the instance. This configuration value will be either the user-passed constraint(s) cpu-cores and memory, else a default minimum cpu-cores. We also define a new constant for minimum cpu-cores that is matched on `instances.MatchingInstanceTypes`. Also, we cannot pass this `ShapeConfig` value if the selected shape is not flexible so we have to be able to distinct between flexible and non-flexible shapes. This was done by adding `MaxCpuCores` and `MaxMem` on `InstanceType` _Bonus: some cleanup on tests._ ## Checklist *If an item is not applicable, use `~strikethrough~`.* - [X] Code style: imports ordered, good names, simple structure, etc - [X] Comments saying why design decisions were made - [X] Go unit tests, with comments saying what you're testing - [ ] ~[Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~ - [ ] ~[doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~ ## QA steps ``` juju bootstrap --debug --config compartment-id=ocid1.compartment.oc1..aaaaaaaanvu2racnlczevenu73dlcf3nokgsjpdkbdgp4xbrz3lb2y4giyjq oci-canonical c ``` check OCIs console and make sure the `VM.Standard1.1` shape was selected as the controller running instance. ## Links **Jira card:** JUJU-[4694]

hmlanigan · 2023-10-04T19:03:20Z

@nvinuesa with the qa instructions, I fail to bootstrap with:

18:36:11 ERROR juju.cmd.juju.commands bootstrap.go:988 failed to bootstrap model: cannot use agent built for "arm64" using a machine running on "amd64"
18:36:11 DEBUG juju.cmd.juju.commands bootstrap.go:989 (error details: [{github.com/juju/juju/cmd/juju/commands.(*bootstrapCommand).Run:1087: failed to bootstrap model} {github.com/juju/juju/environs/bootstrap.Bootstrap:732: } {cannot use agent built for "arm64" using a machine running on "amd64"}])

hmlanigan

I need more time to review. The parsing is complex. The qa did not work as written.

provider/oci/images.go

hmlanigan · 2023-10-04T19:15:49Z

provider/oci/images.go

-			VirtType: string(val.ImageType),
+		// Iterate over the list of supported architectures of the
+		// image.
+		for _, arch := range val.Archs() {


Why do we need to iterate over val.Arches(), if val can only be 1 arch?

the issue is that we cannot enforce that. Since the instanceTypes are created from the returned list of shapes from the API, we can't be sure that it will always be the case. You suggest that we throw an error if len(val.Archs()) > 1?

hmlanigan · 2023-10-04T19:18:59Z

provider/oci/images.go

+// struct returned by oci's API. Also, it returns the architecture that is
+// detected from the image OperatingSystemVersion and it's name.


The 2nd sentence doesn't match the function return values.

s/it's/its/

I just rephrased it

hmlanigan · 2023-10-04T19:21:58Z

provider/oci/images.go

+	// we need to split the provided OperatingSystemVersion, since
+	// it can contain information that does not belong to the base's
+	// channel.
+	// E.g: Canonical-Ubuntu-22.04-Minimal-aarch64-2023.08.27-0


More examples of possible OperatingSystemVersion values would be helpful.

I added a second example, with more detailed explanation

hmlanigan · 2023-10-04T19:25:40Z

provider/oci/images.go

+	// becomes:
+	//        OperatingSystem:        Canonical Ubuntu
+	//        OperatingSystemVersion: 22.04 Minimal aarch64


This was copied from older code, and should not be valid with your fixes. Please validate.

I rephrased this comment

nvinuesa · 2023-10-04T21:00:34Z

@hmlanigan regarding the QA, I suspect you already had something in _build/ probably another simplestreams? can you try running:

rm -rf _build
make go-install
GOOS=linux GOARCH=arm64 make go-build
AGENT_PACKAGE_PLATFORMS=linux/arm64 make simplestreams
export JUJU_METADATA_SOURCE="/home/nicolas/workspace/canonical/juju/_build/simplestreams"

please?

hmlanigan

This one took a while. It was worth it to improve how juju models instance selections for newer types of cloud machines.

provider/oci/images.go

By correctly parsing the version of the ubuntu cloud images returned by OCI's API, we can then create different bases for ubuntu aarch64 (the cloud image needed to target arm-based shapes). Bonus: fixed a test that had incorrect logic.

Since OCI image parsing is becoming complex, more examples were needed.

…res don't match the image's architecture

nvinuesa · 2023-10-06T13:23:16Z

/merge

#16396 Merges: - #16368 - #16357 - #16135 - #16391 - #16277 - #16296 Conflicts: - deleted by us: api/state_test.go - both modified: apiserver/admin_test.go - both modified: apiserver/facades/client/client/client.go - both modified: apiserver/facades/client/client/status.go - both modified: apiserver/facades/client/client/status_test.go - both modified: apiserver/facades/client/storage/storage.go - both modified: cmd/juju/ssh/ssh_machine.go - both modified: cmd/juju/ssh/ssh_machine_test.go - both modified: cmd/juju/status/export_test.go - both modified: cmd/juju/status/status.go - both modified: cmd/juju/status/status_internal_test.go - both modified: cmd/juju/status/status_test.go - deleted by them: core/charm/computedseries.go - both modified: state/application.go - both modified: state/application_test.go - both modified: state/state.go - both added: testcharms/charm-repo/bionic/mysql/manifest.yaml All conflicts originating from: - #16135 - #16296

nvinuesa added the 3.3 label Sep 18, 2023

nvinuesa force-pushed the juju-4514 branch from 75419d2 to 690024e Compare September 18, 2023 13:22

nvinuesa requested review from hmlanigan and jack-w-shaw September 18, 2023 13:56

hmlanigan requested changes Sep 18, 2023

View reviewed changes

nvinuesa force-pushed the juju-4514 branch 3 times, most recently from a9e0722 to 8db2897 Compare September 21, 2023 13:35

nvinuesa requested a review from hmlanigan September 21, 2023 13:56

hmlanigan added the do not merge Even if a PR has been approved, do not merge the PR! label Sep 22, 2023

nvinuesa mentioned this pull request Sep 29, 2023

Support flexible shapes on Oracle OCI #16357

Merged

5 tasks

nvinuesa force-pushed the juju-4514 branch from 8db2897 to 254ceaa Compare October 3, 2023 07:51

nvinuesa removed the do not merge Even if a PR has been approved, do not merge the PR! label Oct 3, 2023

hmlanigan reviewed Oct 4, 2023

View reviewed changes

nvinuesa requested a review from hmlanigan October 4, 2023 21:34

hmlanigan approved these changes Oct 5, 2023

View reviewed changes

provider/oci/images.go Show resolved Hide resolved

nvinuesa added 3 commits October 6, 2023 14:12

Add support for ARM shapes on Oracle OCI

78a67fe

By correctly parsing the version of the ubuntu cloud images returned by OCI's API, we can then create different bases for ubuntu aarch64 (the cloud image needed to target arm-based shapes). Bonus: fixed a test that had incorrect logic.

Add examples and more detailed comments on OCI image parsing

5069fcc

Since OCI image parsing is becoming complex, more examples were needed.

Remove iterating on architectures, log if returned shapes' architectu…

c326813

…res don't match the image's architecture

nvinuesa force-pushed the juju-4514 branch from 3ece6da to c326813 Compare October 6, 2023 12:12

jujubot merged commit 53ac4b4 into juju:3.3 Oct 6, 2023
18 of 19 checks passed

jack-w-shaw mentioned this pull request Oct 6, 2023

3.3 into main #16396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ARM shapes on Oracle OCI #16277

Add support for ARM shapes on Oracle OCI #16277

nvinuesa commented Sep 18, 2023 •

edited

hmlanigan left a comment

hmlanigan Sep 18, 2023

nvinuesa Sep 21, 2023

hmlanigan Sep 18, 2023

nvinuesa Oct 3, 2023

hmlanigan Sep 18, 2023

nvinuesa Sep 21, 2023

hmlanigan Sep 18, 2023

nvinuesa Sep 21, 2023

hmlanigan Sep 18, 2023

nvinuesa Oct 3, 2023

hmlanigan Sep 18, 2023

nvinuesa Sep 21, 2023

hmlanigan Sep 18, 2023

nvinuesa Sep 21, 2023

hmlanigan Sep 18, 2023

nvinuesa Sep 21, 2023

hmlanigan commented Sep 21, 2023

hmlanigan commented Sep 22, 2023

hmlanigan commented Oct 4, 2023

hmlanigan left a comment

hmlanigan Oct 4, 2023

nvinuesa Oct 4, 2023

hmlanigan Oct 4, 2023

nvinuesa Oct 4, 2023

hmlanigan Oct 4, 2023

nvinuesa Oct 4, 2023

hmlanigan Oct 4, 2023

nvinuesa Oct 4, 2023

nvinuesa commented Oct 4, 2023

hmlanigan left a comment

nvinuesa commented Oct 6, 2023

		UbuntuMinimalBase = "ubuntu-minimal"
		UbuntuARMBase = "ubuntu-" + arch.ARM64

		// struct returned by oci's API. Also, it returns the architecture that is
		// detected from the image OperatingSystemVersion and it's name.

Add support for ARM shapes on Oracle OCI #16277

Add support for ARM shapes on Oracle OCI #16277

Conversation

nvinuesa commented Sep 18, 2023 • edited

Checklist

QA steps

Links

hmlanigan 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

hmlanigan commented Sep 21, 2023

hmlanigan commented Sep 22, 2023

hmlanigan commented Oct 4, 2023

hmlanigan 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

nvinuesa commented Oct 4, 2023

hmlanigan left a comment

Choose a reason for hiding this comment

nvinuesa commented Oct 6, 2023

nvinuesa commented Sep 18, 2023 •

edited