Skip to content

Commit

Permalink
Merge pull request juju#18479 from ycliuhw/fix-model-export
Browse files Browse the repository at this point in the history
juju#18479

This PR resolves an issue with duplicate model data export during model migration. The problem occurred because domain exporters were registered each time the `ExportModel` method was called. These exporters were cached in the coordinator on the apiserver context and reused for all model migration requests, leading to repeated data exports if the migrate command was run multiple times.

## Checklist

- [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

Before: the one block device item has been exported 6 times.
```
 block-devices:
 version: 2
 block-devices:
 - name: nvme0n1p16
 links:
 - /dev/disk/by-diskseq/9-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1-part16
 - /dev/disk/by-id/nvme-uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7-part16
 - /dev/disk/by-label/BOOT
 - /dev/disk/by-partuuid/1b154230-375f-4a23-827a-0d7830f7c0c1
 - /dev/disk/by-path/pci-0000:00:04.0-nvme-1-part16
 - /dev/disk/by-uuid/d726071f-b770-48f2-8f6f-3016a0245f0f
 label: BOOT
 uuid: d726071f-b770-48f2-8f6f-3016a0245f0f
 serial-id: Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1
 wwn: uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7
 size: 913
 fs-type: ext4
 in-use: true
 mount-point: /boot
 - name: nvme0n1p16
 links:
 - /dev/disk/by-diskseq/9-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1-part16
 - /dev/disk/by-id/nvme-uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7-part16
 - /dev/disk/by-label/BOOT
 - /dev/disk/by-partuuid/1b154230-375f-4a23-827a-0d7830f7c0c1
 - /dev/disk/by-path/pci-0000:00:04.0-nvme-1-part16
 - /dev/disk/by-uuid/d726071f-b770-48f2-8f6f-3016a0245f0f
 label: BOOT
 uuid: d726071f-b770-48f2-8f6f-3016a0245f0f
 serial-id: Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1
 wwn: uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7
 size: 913
 fs-type: ext4
 in-use: true
 mount-point: /boot
 - name: nvme0n1p16
 links:
 - /dev/disk/by-diskseq/9-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1-part16
 - /dev/disk/by-id/nvme-uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7-part16
 - /dev/disk/by-label/BOOT
 - /dev/disk/by-partuuid/1b154230-375f-4a23-827a-0d7830f7c0c1
 - /dev/disk/by-path/pci-0000:00:04.0-nvme-1-part16
 - /dev/disk/by-uuid/d726071f-b770-48f2-8f6f-3016a0245f0f
 label: BOOT
 uuid: d726071f-b770-48f2-8f6f-3016a0245f0f
 serial-id: Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1
 wwn: uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7
 size: 913
 fs-type: ext4
 in-use: true
 mount-point: /boot
 - name: nvme0n1p16
 links:
 - /dev/disk/by-diskseq/9-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1-part16
 - /dev/disk/by-id/nvme-uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7-part16
 - /dev/disk/by-label/BOOT
 - /dev/disk/by-partuuid/1b154230-375f-4a23-827a-0d7830f7c0c1
 - /dev/disk/by-path/pci-0000:00:04.0-nvme-1-part16
 - /dev/disk/by-uuid/d726071f-b770-48f2-8f6f-3016a0245f0f
 label: BOOT
 uuid: d726071f-b770-48f2-8f6f-3016a0245f0f
 serial-id: Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1
 wwn: uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7
 size: 913
 fs-type: ext4
 in-use: true
 mount-point: /boot
 - name: nvme0n1p16
 links:
 - /dev/disk/by-diskseq/9-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1-part16
 - /dev/disk/by-id/nvme-uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7-part16
 - /dev/disk/by-label/BOOT
 - /dev/disk/by-partuuid/1b154230-375f-4a23-827a-0d7830f7c0c1
 - /dev/disk/by-path/pci-0000:00:04.0-nvme-1-part16
 - /dev/disk/by-uuid/d726071f-b770-48f2-8f6f-3016a0245f0f
 label: BOOT
 uuid: d726071f-b770-48f2-8f6f-3016a0245f0f
 serial-id: Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1
 wwn: uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7
 size: 913
 fs-type: ext4
 in-use: true
 mount-point: /boot
 - name: nvme0n1p16
 links:
 - /dev/disk/by-diskseq/9-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1-part16
 - /dev/disk/by-id/nvme-uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7-part16
 - /dev/disk/by-label/BOOT
 - /dev/disk/by-partuuid/1b154230-375f-4a23-827a-0d7830f7c0c1
 - /dev/disk/by-path/pci-0000:00:04.0-nvme-1-part16
 - /dev/disk/by-uuid/d726071f-b770-48f2-8f6f-3016a0245f0f
 label: BOOT
 uuid: d726071f-b770-48f2-8f6f-3016a0245f0f
 serial-id: Amazon_Elastic_Block_Store_vol04a3dc4efb35169b7_1
 wwn: uuid.b9fa7db8-4243-56ed-80d0-f8cd79b8e8f7
 size: 913
 fs-type: ext4
 in-use: true
 mount-point: /boot
```

After:
```
 block-devices:
 version: 2
 block-devices:
 - name: nvme0n1p16
 links:
 - /dev/disk/by-diskseq/9-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol00fc4c0ecdbec535c-part16
 - /dev/disk/by-id/nvme-Amazon_Elastic_Block_Store_vol00fc4c0ecdbec535c_1-part16
 - /dev/disk/by-id/nvme-uuid.2446e143-ca43-51f8-91fb-ee0662ec7e50-part16
 - /dev/disk/by-label/BOOT
 - /dev/disk/by-partuuid/1b154230-375f-4a23-827a-0d7830f7c0c1
 - /dev/disk/by-path/pci-0000:00:04.0-nvme-1-part16
 - /dev/disk/by-uuid/d726071f-b770-48f2-8f6f-3016a0245f0f
 label: BOOT
 uuid: d726071f-b770-48f2-8f6f-3016a0245f0f
 serial-id: Amazon_Elastic_Block_Store_vol00fc4c0ecdbec535c_1
 wwn: uuid.2446e143-ca43-51f8-91fb-ee0662ec7e50
 size: 913
 fs-type: ext4
 in-use: true
 mount-point: /boot
```


## Documentation changes

No

## Links


**Jira card:** [JUJU-7267](https://warthogs.atlassian.net/browse/JUJU-7267)



[JUJU-7267]: https://warthogs.atlassian.net/browse/JUJU-7267?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
  • Loading branch information
jujubot authored Dec 10, 2024
2 parents 0b190a9 + bb3d8a1 commit 8f92420
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 14 deletions.
5 changes: 3 additions & 2 deletions internal/migration/migration.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func NewModelExporter(
logger corelogger.Logger,
clock clock.Clock,
) *ModelExporter {
return &ModelExporter{
me := &ModelExporter{
operationExporter: operationExporter,
legacyStateExporter: legacyStateExporter,
scope: scope,
Expand All @@ -102,6 +102,8 @@ func NewModelExporter(
logger: logger,
clock: clock,
}
me.operationExporter.ExportOperations(me.storageRegistryGetter)
return me
}

// ExportModelPartial partially serializes a model description from the
Expand Down Expand Up @@ -129,7 +131,6 @@ func (e *ModelExporter) ExportModel(ctx context.Context, leaders map[string]stri

// Export serializes a model description from the database contents.
func (e *ModelExporter) Export(ctx context.Context, model description.Model) (description.Model, error) {
e.operationExporter.ExportOperations(e.storageRegistryGetter)
if err := e.coordinator.Perform(ctx, e.scope, model); err != nil {
return nil, errors.Trace(err)
}
Expand Down
26 changes: 14 additions & 12 deletions internal/migration/migration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,6 @@ func (s *ExportSuite) TestExportValidates(c *gc.C) {
defer s.setupMocks(c).Finish()

scope := modelmigration.NewScope(nil, nil, nil)
exporter := migration.NewModelExporter(
s.operationsExporter,
nil,
scope,
s.storageRegistryGetter,
s.coordinator,
nil, nil,
)

// The order of the expectations is important here. We expect that the
// validation is the last thing that happens.
Expand All @@ -79,6 +71,15 @@ func (s *ExportSuite) TestExportValidates(c *gc.C) {
s.model.EXPECT().Validate().Return(nil),
)

exporter := migration.NewModelExporter(
s.operationsExporter,
nil,
scope,
s.storageRegistryGetter,
s.coordinator,
nil, nil,
)

_, err := exporter.Export(context.Background(), s.model)
c.Assert(err, jc.ErrorIsNil)
}
Expand All @@ -87,6 +88,11 @@ func (s *ExportSuite) TestExportValidationFails(c *gc.C) {
defer s.setupMocks(c).Finish()

scope := modelmigration.NewScope(nil, nil, nil)

s.operationsExporter.EXPECT().ExportOperations(s.storageRegistryGetter)
s.model.EXPECT().Validate().Return(errors.New("boom"))
s.coordinator.EXPECT().Perform(gomock.Any(), scope, s.model).Return(nil)

exporter := migration.NewModelExporter(
s.operationsExporter,
nil,
Expand All @@ -96,10 +102,6 @@ func (s *ExportSuite) TestExportValidationFails(c *gc.C) {
nil, nil,
)

s.operationsExporter.EXPECT().ExportOperations(s.storageRegistryGetter)
s.model.EXPECT().Validate().Return(errors.New("boom"))
s.coordinator.EXPECT().Perform(gomock.Any(), scope, s.model).Return(nil)

_, err := exporter.Export(context.Background(), s.model)
c.Assert(err, gc.ErrorMatches, "boom")
}
Expand Down

0 comments on commit 8f92420

Please sign in to comment.