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

Require destination name when copying an instance on the same server #12447

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

MusicDin
Copy link
Member

@MusicDin MusicDin commented Oct 24, 2023

Removes unused code that determines generated name on copy or move (using copy).
It requires destination name to be provided, with only exception being when moving an instance to another remote (for example: lxc move c1 remote2:). In such case, the destination name is set to the source name.

Fixes #12338

@ru-fu
Copy link
Contributor

ru-fu commented Oct 24, 2023

lxc/copy.go Outdated
@@ -153,6 +154,9 @@ func (c *cmdCopy) copyInstance(conf *config.Config, sourceResource string, destR
return fmt.Errorf(i18n.G("--instance-only can't be passed when the source is a snapshot"))
}

// Remove snapshot suffix from destination name.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment say what but doesn't explain the why.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this one.

Snapshot suffix is removed to ensure consistency when copying/moving an instance (or snapshot) to a different remote (lxc move c1 remote2: or lxc move c1/snap0 remote2:).

However, I missed the fact, that this will trim the suffix even if the user provides the snapshot name in the destination name.

For example, the following commands produced the same error, although they should not:

lxc copy c1/snap0 c1/snap1
lxc copy c1/snap0 c1

# Error: Failed creating instance record: Add instance info to the database: This "instances" entry already exists`

Now, the snapshot name is trimmed only when destName is set from the source.


A bit unrelated, but is there a reason we do not output the instance name in the above error? (something like Database entry for instance "c1" already exists)

Signed-off-by: Din Music <din.music@canonical.com>
Signed-off-by: Din Music <din.music@canonical.com>
@MusicDin
Copy link
Member Author

MusicDin commented Oct 24, 2023

@ru-fu I've checked the docs and to me everything seems up to date. It is most likely that the PR title is misleading.

This PR requires the destination name to be provided if remote is not specified, as the command would fail anyway because the instance with that name already exists.

lxc copy c1

And it fixes the situation where an instance is created from a snapshot on a different remote:

lxc copy c1/snap0 remote2:

The above command would previously fail with Error: The character "/" is reserved for snapshots, so now we trim the snapshot name when destination name is not provided.


We had some code related to determining the generated instance name, which is no longer being used. To me it seems that lxc copy used to generate an instance name if destination name is not provided (similar to lxc init when instance name is not provided). I was not able to find anything related to that in the docs, but just to emphasize it here if any leftovers are found.

@MusicDin MusicDin changed the title copy: Require destination name Require destination name when copying an instance on the same server Oct 24, 2023
@ru-fu
Copy link
Contributor

ru-fu commented Oct 24, 2023

OK, great!

So lxc move c1 --target server1 is still going to work?

@MusicDin
Copy link
Member Author

I've double checked and it is working

// If the instance is being copied to a different remote and no destination name is
// specified, use the source name with snapshot suffix trimmed (in case a new instance
// is being created from a snapshot).
if destName == "" && destResource != "" && c.flagTarget == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to consider copying a snapshot with --target specified too?

@tomponline tomponline merged commit a673b99 into canonical:main Oct 24, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lxc copy should be smarter when copying from a snapshot
3 participants