Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Device Types re-organise #6135

Merged
merged 8 commits into from Sep 4, 2019

Conversation

@tomponline
Copy link
Member

commented Aug 30, 2019

Since creating the Device and Devices types in the device package (that represent device configuration and sets of devices respectively), there has been an ongoing issue with some parts of the code base using Devices as a native map[string]map[string]string type rather than a map[string]Device as it would logically be.

This has caused type conversion issues and came to a head during the development of the disk device type as this required sorting the devices by path, which required the devices list to be of type Devices.

This PR changes the following:

  • Defines deviceConfig.Devices type as map[string]Device rather than map[string]map[string]string type.
  • Adds conversion functions for Devices to/from native type.
  • Moves validation helper to be attached to the Device type.
  • Updates all usage of old-style devices native type to deviceConfig.Devices type (only inside the lxd/ files).
  • Uses deviceConfig. as an alias for lxd/device/config for new imports (but leaves old imports inconsistent using just config to avoid increasing the delta size).

Part of #5819

@tomponline tomponline force-pushed the tomponline:tp-device-types branch 4 times, most recently from 112fffb to 2ab3550 Aug 30, 2019

@tomponline tomponline referenced this pull request Aug 30, 2019
5 of 5 tasks complete
@stgraber

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

So this feels wrong to me. The api package is really meant to only include the on-wire (JSON) API structures, metadata needed to read/write it as json or yaml and minimal helpers to convert between types (getting the writable data).

Putting the entire Device struct and switching all map[string]map[string]string to map[string]Devices is an API breakage for anyone currently using our Go API client and may also cause some issues with serialization/deserialization of those structs to json/yaml.

We should certainly use device.Device and device.Devices within the daemon codebase (lxd/*) but this should be loaded from/convert-to the simpler map[string]map[string]string when hitting the API.

@tomponline tomponline force-pushed the tomponline:tp-device-types branch 4 times, most recently from cae6d66 to d6c51fe Sep 3, 2019

@tomponline

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

@stgraber I've re-worked this PR this morning to move the device config types back to their original config package. However I've also imported it consistently as deviceConfig. to make it easier to find references to it, rather than just config., and have added conversion to/from native types so that the shared/api package does not need to import the deviceConfig package.

@tomponline tomponline changed the title Device and Devices type move to api package Device Types re-organise Sep 3, 2019

@tomponline tomponline requested a review from stgraber Sep 3, 2019

@tomponline tomponline force-pushed the tomponline:tp-device-types branch from d6c51fe to 45ad387 Sep 3, 2019

@lxc-jenkins

This comment has been minimized.

Copy link

commented Sep 3, 2019

Testsuite passed

@stgraber

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@tomponline Please rebase

@tomponline tomponline force-pushed the tomponline:tp-device-types branch from 45ad387 to 992b2cb Sep 3, 2019

@tomponline

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

@stgraber have rebased thanks

@tomponline tomponline force-pushed the tomponline:tp-device-types branch from 992b2cb to 60747c9 Sep 3, 2019

@tomponline

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

@stgraber rebased again.

@lxc-jenkins

This comment has been minimized.

Copy link

commented Sep 3, 2019

Testsuite passed

@stgraber

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

This branch looks pretty confusing to me still.
I see a whole bunch of places where an existing import of device/config is changed only to be renamed to deviceConfig and then causing all pre-existing functions and type calls to be renamed to match, without any clear reason behind it.

I'm also not sure why lxc-to-lxd for example is being changed at all and some of the commits still refer to api.Devices and api.Device, probably in some intermediate state, despite those struct not existing following the previous round of review.

Can you clean things up a bit so that commits make individual sense and try to limit import renames at least for now so it's easier to review this?

tomponline added 4 commits Sep 4, 2019
device/config/devices: Changes Devices type to map[string]Device
In doing so, also adds functions to convert from native map[string]map[string]string type to Devices type and back to native.

Removes use of DeepCopy and dependency on shared package and replaces with the newly added Device.Clone() function.

Also moves ValidateDevice function to be attached to the Device type as the Validate() function for consistency.

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
device/config/devices/validate: Moves function to be attached to Devi…
…ce type

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
lxc-to-lxd/main/migrate: Removes dependency on deviceConfig package
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
lxd/device: Updates use of Device type
So that device implementations can access the Validate function on the device config.

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>

@tomponline tomponline force-pushed the tomponline:tp-device-types branch from 60747c9 to 448a0c1 Sep 4, 2019

@tomponline

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2019

@stgraber I've redone the branch again from scratch to limit the changes introduced.

I've strictly avoided importing github.com/lxc/lxd/lxd/device/config into any files outside of lxd/.

In some cases I've removed the dependency on github.com/lxc/lxd/lxd/device/config where it was not strictly needed.

@@ -168,7 +167,7 @@ func projectCreateDefaultProfile(tx *db.ClusterTx, project string) error {
profile.Name = "default"
profile.Description = fmt.Sprintf("Default LXD profile for project %s", project)
profile.Config = map[string]string{}
profile.Devices = config.Devices{}
profile.Devices = map[string]map[string]string{}

This comment has been minimized.

Copy link
@stgraber

stgraber Sep 4, 2019

Member

Do we even need to set profile.Config and profile.Devices at all? Could you try just removing those two lines and see if things blow up in tests (I don't see why it would)?

This comment has been minimized.

Copy link
@tomponline

tomponline Sep 4, 2019

Author Member

@stgraber I will try it, map's zero value is nil, so it may blow out but worth a try.

This comment has been minimized.

Copy link
@stgraber

stgraber Sep 4, 2019

Member

Yeah, but we support someone not passing it through the REST API (or we should anyway), so it shouldn't blow up.

This comment has been minimized.

Copy link
@tomponline

tomponline Sep 4, 2019

Author Member

@stgraber made those changes, seems fine.

@@ -48,7 +48,7 @@ func (suite *containerTestSuite) TestContainer_ProfilesMulti() {
Name: "unprivileged",
Description: "unprivileged",
Config: map[string]string{"security.privileged": "true"},
Devices: config.Devices{},
Devices: map[string]map[string]string{},

This comment has been minimized.

Copy link
@stgraber

stgraber Sep 4, 2019

Member

Same as the other commit, I don't see why we need to set this thing in the first place, can you try just removing it?

@stgraber

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

Rest looks good. It feels to me like we could probably cast rather than clone in some cases, but I don't think it matters and cloning has the advantage of preventing modifications.

tomponline added 4 commits Sep 4, 2019
api/project: Removes dependency on deviceConfig package
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
lxd: Updates use of Devices type
Converts back to native type where needed to avoid using the lxd/device/config package outside of the lxd package.

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
lxc-to-lxd/main/migrate/test: Removes dependency of lxd/device/config…
… package

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
lxd: Updates tests to use updated Devices type
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>

@tomponline tomponline force-pushed the tomponline:tp-device-types branch from 448a0c1 to 8402392 Sep 4, 2019

@stgraber stgraber merged commit 097a51f into lxc:master Sep 4, 2019

4 of 5 checks passed

Testsuite Test started
Details
Branch target Branch target is correct
Details
DCO All commits signed-off
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tomponline tomponline deleted the tomponline:tp-device-types branch Sep 4, 2019

@lxc-jenkins

This comment has been minimized.

Copy link

commented Sep 4, 2019

Testsuite passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.