[JUJU-390] Transfer machine DisplayName through migration #13645

cderici · 2022-01-23T00:09:37Z

Description

This allows the machines (usually provided by OCI or MAAS) to be able to transfer their DisplayName fields through a migration.

Fixes:

LP #1954969.

Requires:

QA Steps

You'll need either Oracle OCI or MAAS controllers for this one. (Other providers don't use display name)
I tested it with the OCI, like as follows (I used the compartment&tenancy ids from within the cloud-city):

juju bootstrap --config compartment-id=$OCID_COMPARTMENT oci-canonical oci-test-controller2

juju bootstrap --config compartment-id=$OCID_COMPARTMENT oci-canonical oci-test-controller1

juju add-model --config compartment-id=$OCID_COMPARTMENT test-model1

juju deploy ubuntu

Note the machine Inst Id in juju status, or do db.instanceData.find().pretty() in mongodb and look for the display-name field.

juju migrate test-model1 oci-test-controller2

Migration should be successful,
After the migration is complete you should be able to see the same id in the juju status or the db.

Notes & Discussion

This PR also includes temporary code that will be removed before the PR lands:

go.mod for external changed dependency (description pkg), and
provider/oci/environ.go for quick fix to allow the PR to run

juanmanuel-tirado · 2022-01-24T10:47:33Z

provider/oci/environ.go

@@ -322,7 +322,7 @@ func (e *Environ) Create(ctx envcontext.ProviderCallContext, params environs.Cre

 // AdoptResources implements environs.Environ.
 func (e *Environ) AdoptResources(ctx envcontext.ProviderCallContext, controllerUUID string, fromVersion version.Number) error {
-	return errors.NotImplementedf("AdoptResources")
+	return nil


Why we do not longer return NotImplemented? Even if this is not necessary for OCI, this is more useful than returning nil.

That NYI error is breaking migration on the OCI, and unrelated to this PR, and returning nil is just a temporary patch to be able to run and test this PR. (I believe the people filed the bug were using maas). See LP #1958459. I might go tackle that afterwards too. But currently returning nil here is just temporary and again, will be reverted before landing.

juanmanuel-tirado · 2022-01-24T11:01:02Z

go.mod

@@ -144,6 +144,8 @@ require (

 replace github.com/altoros/gosigma => github.com/juju/gosigma v0.0.0-20200420012028-063911838a9e

+replace github.com/juju/description/v3 => github.com/cderici/description/machine-display-name


To be changed after juju/description is merged

Yes, this is to allow this PR to use another PR on my description fork, which is a requirement of this PR and will be landed before, so I'll revert this back as well 👍

juanmanuel-tirado

After running QA I'm OK with the changes.

Do not merge before passing tests and replacing go.mod reference to the original description repo.

state/machine.go

hmlanigan

Doing QA now.

Please add updated unit tests migration import and export.

hmlanigan · 2022-01-25T18:54:18Z

state/migration_import.go

@@ -577,6 +577,10 @@ func (i *importer) machineInstanceOp(mdoc *machineDoc, inst description.CloudIns
 	if arch := inst.Architecture(); arch != "" {
 		doc.Arch = &arch
 	}
+	if displayName := inst.DisplayName(); displayName != "" {
+		doc.DisplayName = displayName


Suggestion: since display name is not a pointer, nor is it labeled omit empty in state, you can just do
DisplayName: inst.DisplayName(), above, in ln 571-574.

#13653 #### Description Migration fails on some providers (e.g. oci) because of some not yet implemented/optional features. This unblocks it by returning nil whenever it sees an nyi. All the other errors are raised as usual. Fixes: https://bugs.launchpad.net/juju/+bug/1958459 #### QA steps Migration on oci should succeed. (use the compartment&tenancy ids from within the cloud-city) ```sh juju bootstrap --config compartment-id=$OCID_COMPARTMENT oci-canonical oci-test-controller2 juju bootstrap --config compartment-id=$OCID_COMPARTMENT oci-canonical oci-test-controller1 juju add-model --config compartment-id=$OCID_COMPARTMENT test-model1 juju deploy ubuntu juju migrate test-model1 oci-test-controller2 ``` #### Notes & Discussion This is sort of a requirement for #13645, as we need the migration to be working for that to take effect.

manadart

Minor change required.

manadart · 2022-01-27T08:25:27Z

state/machine.go

@@ -279,6 +279,14 @@ func (m *Machine) HardwareCharacteristics() (*instance.HardwareCharacteristics,
 	return hardwareCharacteristics(instData), nil
 }

+func (m *Machine) DisplayName() (string, error) {


We don't want to add this exported method just for tests. Someone will end up using it in production code and we've got a double access for machine then instance data.

Instead, add a public method, GetInstanceData to package_test.go that wraps the private method getInstanceData and call that to get it directly.

Ah ok that makes sense, I'll make the change 👍 thanks for the review!

I just realized we already have a method called InstanceNames in the machine package that I didn't know before, so I just used that one in the tests instead of redundantly adding a new method. Thanks for the catch!

includes temporary code that will be removed before the PR lands: - go.mod for external changed dependency (description pkg), and - provider/oci/environ.go for quick fix to allow the PR to run

To use the juju/description edge

DisplayName field is not a pointer, nor omitempty, so no need to check creating extra variables

Includes a helper function DisplayName() for state.Machine, normally not available since DisplayName is a field of instanceData (i.e. not Machine)

Using existing Machine.InstanceNames() method instead of exporting a new one

cderici · 2022-01-28T18:19:16Z

$$merge$$

juanmanuel-tirado reviewed Jan 24, 2022

View reviewed changes

juanmanuel-tirado approved these changes Jan 24, 2022

View reviewed changes

manadart requested changes Jan 24, 2022

View reviewed changes

state/machine.go Outdated Show resolved Hide resolved

cderici added 2.9 do not merge Even if a PR has been approved, do not merge the PR! labels Jan 24, 2022

hmlanigan reviewed Jan 25, 2022

View reviewed changes

cderici mentioned this pull request Jan 25, 2022

[JUJU-497] Avoid breaking migration because of NYI #13653

Merged

cderici removed the do not merge Even if a PR has been approved, do not merge the PR! label Jan 26, 2022

cderici force-pushed the migration-transfers-display-name branch from 93fa60e to 7ae1a8f Compare January 26, 2022 17:20

manadart requested changes Jan 27, 2022

View reviewed changes

manadart approved these changes Jan 28, 2022

View reviewed changes

cderici added 7 commits January 28, 2022 10:14

Transfer machine DisplayName through migration

e568938

includes temporary code that will be removed before the PR lands: - go.mod for external changed dependency (description pkg), and - provider/oci/environ.go for quick fix to allow the PR to run

Transfer display-name over CloudInstance instead of MachineDoc

0cb22e5

Update go.mod & go.sum

d76ee75

To use the juju/description edge

Revert back temporary AdoptResources with a TODO

d73d921

Optimize code a bit

48d2a35

DisplayName field is not a pointer, nor omitempty, so no need to check creating extra variables

Add unit tests for migrate_export & import

1728600

Includes a helper function DisplayName() for state.Machine, normally not available since DisplayName is a field of instanceData (i.e. not Machine)

Re-use existing functionality instead of introducing new method

25ad393

Using existing Machine.InstanceNames() method instead of exporting a new one

cderici force-pushed the migration-transfers-display-name branch from 5edc7c4 to 25ad393 Compare January 28, 2022 17:34

jujubot merged commit 4d65952 into juju:2.9 Jan 28, 2022

cderici deleted the migration-transfers-display-name branch February 16, 2022 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JUJU-390] Transfer machine DisplayName through migration #13645

[JUJU-390] Transfer machine DisplayName through migration #13645

cderici commented Jan 23, 2022 •

edited

juanmanuel-tirado Jan 24, 2022

cderici Jan 24, 2022 •

edited

juanmanuel-tirado Jan 24, 2022

cderici Jan 24, 2022

juanmanuel-tirado left a comment

hmlanigan left a comment

hmlanigan Jan 25, 2022

manadart left a comment

manadart Jan 27, 2022

cderici Jan 27, 2022

cderici Jan 27, 2022

cderici commented Jan 28, 2022

		@@ -144,6 +144,8 @@ require (

		replace github.com/altoros/gosigma => github.com/juju/gosigma v0.0.0-20200420012028-063911838a9e

		replace github.com/juju/description/v3 => github.com/cderici/description/machine-display-name

[JUJU-390] Transfer machine DisplayName through migration #13645

[JUJU-390] Transfer machine DisplayName through migration #13645

Conversation

cderici commented Jan 23, 2022 • edited

Description

QA Steps

Notes & Discussion

juanmanuel-tirado Jan 24, 2022

Choose a reason for hiding this comment

cderici Jan 24, 2022 • edited

Choose a reason for hiding this comment

juanmanuel-tirado Jan 24, 2022

Choose a reason for hiding this comment

cderici Jan 24, 2022

Choose a reason for hiding this comment

juanmanuel-tirado left a comment

Choose a reason for hiding this comment

hmlanigan left a comment

Choose a reason for hiding this comment

hmlanigan Jan 25, 2022

Choose a reason for hiding this comment

manadart left a comment

Choose a reason for hiding this comment

manadart Jan 27, 2022

Choose a reason for hiding this comment

cderici Jan 27, 2022

Choose a reason for hiding this comment

cderici Jan 27, 2022

Choose a reason for hiding this comment

cderici commented Jan 28, 2022

cderici commented Jan 23, 2022 •

edited

cderici Jan 24, 2022 •

edited