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

Storage Create Instance From Image #6386

Merged
merged 18 commits into from Nov 2, 2019

Conversation

@tomponline
Copy link
Member

tomponline commented Nov 1, 2019

No description provided.

tomponline added 5 commits Nov 1, 2019
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
…mage

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline tomponline requested a review from stgraber Nov 1, 2019
// tomp TODO currently passing false to isBlockBackend argument, which only affects
// user hint on how to increase disk space if not enough to unpack image. Ask about
// removing this argument entirely.
return ImageUnpack(imagePath, instanceMountPath, false, b.state.OS.RunningInUserNS, tracker)

This comment has been minimized.

Copy link
@tomponline

tomponline Nov 1, 2019

Author Member

@stgraber I've got a question with this bit, the ImageUnpack take a boolean argument indicating whether or not the backend is block or not (but not a block device that we will be using for VM), the only use of this boolean is to alter the error message returned to the user when there isn't enough room in the volume to unpack the image. It adds to the error "(consider increasing your pool's volume.size)" to the error message if true is passed.

I was wondering whether we should just remove this or make it always return this part because it could also apply if using project quotas.

This comment has been minimized.

Copy link
@stgraber

stgraber Nov 1, 2019

Member

Project quotas apply to the container, not to the image though.

That's why we only show it for block based storage as that's the only case where we will create something of a fixed size and possibly attempt to unpack something larger in it.

For all other cases, the image itself isn't size constrained, the container created from it is.

This comment has been minimized.

Copy link
@tomponline

tomponline Nov 2, 2019

Author Member

@stgraber ah I see OK makes sense.

@@ -423,7 +423,7 @@ test_basic_usage() {
lxc profile create unconfined
lxc profile set unconfined security.privileged true
lxc init testimage foo2 -p unconfined -s "lxdtest-$(basename "${LXD_DIR}")"
[ "$(stat -L -c "%a" "${LXD_DIR}/containers/foo2")" = "700" ]
[ "$(stat -L -c "%a" "${LXD_DIR}/containers/foo2")" = "100" ]

This comment has been minimized.

Copy link
@tomponline

tomponline Nov 1, 2019

Author Member

@stgraber the introduction of CreateMountPath above has resulted in this test needing to be changed.

This comment has been minimized.

Copy link
@tomponline

tomponline Nov 1, 2019

Author Member

@stgraber this change has also broken tests for the old storage drivers that are creating them with 700. Shall I update them to create as 100?

This comment has been minimized.

Copy link
@stgraber

stgraber Nov 1, 2019

Member

Yes, that's probably the easiest. We already have a check in place to validate the 100 mode later on (in security) but this test is running before that logic ever applies :)

This comment has been minimized.

Copy link
@tomponline

tomponline Nov 2, 2019

Author Member

@stgraber thanks

tomponline added 9 commits Nov 1, 2019
…Path()

To ensure perms are applied consistently and correctly across all drivers.

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

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

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline tomponline force-pushed the tomponline:tp-storage-instance-create-from-image branch from 8a60d78 to adb24bd Nov 1, 2019
return nil
}

// CreateInstanceFromImage creates a new volume for an instance populated with the image requested.
func (b *lxdBackend) CreateInstanceFromImage(inst Instance, fingerprint string, op *operations.Operation) error {

This comment has been minimized.

Copy link
@stgraber

stgraber Nov 1, 2019

Member

Hmm, so the logic you have in there is fine for dir but won't work for others.

The image unpacking logic (the filler) should be moved into a private function on lxdBackend and then have it wired in ImageCreate so that all the backends with OptimizedImages may work.

Then CreateInstanceFromImage needs to grow a few checks:

  • If we don't support OptimizedImages, it should call what you have now, using the external filler function
  • If we have OptimizedImages, then it should call driver.HasVolume to see if we've already unpacked it, if we didn't, call ImageCreate and then call CreateVolumeFromCopy to create the volume from the now existing image volume.
@stgraber

This comment has been minimized.

Copy link
Member

stgraber commented Nov 1, 2019

Overall looks good, just a few things that need tweaking:

  • Reshuffle code between CreateContainerFromImage and CreateImage (sharing the filler)
  • Switch to 0100 in the legacy code so the test is happy
  • Remove TODO from the UnpackImage call as we need to keep the distinction and should just base it off a driver.Info field to tell us if volumes are blocks or fs by default.
stgraber added 2 commits Nov 2, 2019
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
@stgraber

This comment has been minimized.

Copy link
Member

stgraber commented Nov 2, 2019

I've now done those changes, lets see what Jenkins thinks.

stgraber added 2 commits Nov 2, 2019
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
Signed-off-by: Stéphane Graber <stgraber@ubuntu.com>
@stgraber stgraber force-pushed the tomponline:tp-storage-instance-create-from-image branch from 9d468f1 to e7249b2 Nov 2, 2019
@stgraber

This comment has been minimized.

Copy link
Member

stgraber commented Nov 2, 2019

missed a few bits for perms, going with a more complete approach now

@brauner brauner merged commit 0437e4f into lxc:master Nov 2, 2019
4 of 5 checks passed
4 of 5 checks passed
Testsuite Build finished.
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
}
} else {
if !b.driver.HasVolume(drivers.VolumeTypeImage, fingerprint) {
err := b.CreateImage(fingerprint, op)

This comment has been minimized.

Copy link
@tomponline

tomponline Nov 2, 2019

Author Member

@stgraber thanks for doing this. As b.CreateImage checks for HasVolume inside it, we could remove the HasVolume check above and simplify this part somewhat.

This comment has been minimized.

Copy link
@stgraber

stgraber Nov 2, 2019

Member

Oh, indeed, we could just always call it. Can you put that in your next branch?

This comment has been minimized.

Copy link
@tomponline

tomponline Nov 4, 2019

Author Member

@stgraber will do

@tomponline tomponline deleted the tomponline:tp-storage-instance-create-from-image branch Nov 2, 2019
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.