Container Rename #8955

mheon · 2021-01-12T20:29:02Z

Add initial support for the podman rename command.

Very much in-progress.

Fixes #1925

mheon · 2021-01-12T21:20:28Z

Status: Libpod backend is functional, but needs to be refactored into the database to remove potentially-serious atomicity issues. That can probably wait until a follow-on PR. CLI frontend has been implemented, but needs completions and valid args tweaked. API endpoints for Compat and Libpod have been implemented. Bindings, documentation, and tests are missing.

Luap99 · 2021-01-12T22:17:54Z

cmd/podman/containers/rename.go

+		Long:  renameDescription,
+		RunE:  rename,
+		Args:  cobra.ExactArgs(2), // TODO we can do better here - check that #1 is container ID, #2 is a name
+		//ValidArgsFunction: common.AutocompleteContainers,


I recommend using common.AutocompletePortCommand as ValidArgsFunction. This will complete only the first argument as container name or id.
ExactArgs(2) looks fine. I am not sure why you would want to check for an ID. The code and the example also seem to allow a name as first arg.

openshift-ci-robot · 2021-01-14T15:15:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mheon · 2021-01-14T15:18:15Z

Stripping WIP

rhatdan · 2021-01-14T15:29:46Z

cmd/podman/containers/rename.go

-		Args:  cobra.ExactArgs(2), // TODO we can do better here - check that #1 is container ID, #2 is a name
-		//ValidArgsFunction: common.AutocompleteContainers,
+		Args:  cobra.ExactArgs(2),
+		ValidArgsFunction: common.AutocompletePortCommand,


Is this correct? PortCommand?

@Luap99 recommended it - it completes the first argument as an existing container and does no completion on the second argument

Ok we should get a different name for it then.

Yes I will put this on my TODO list. I just need to find a descriptive name.

Which can probably happen in a separate PR, because this needs to merge in the next 3-ish hours 😄

rhatdan · 2021-01-14T15:30:15Z

Looks great, need to squash some of the commits.

mheon · 2021-01-14T15:43:23Z

Squashed down

cmd/podman/containers/rename.go

TomSweeneyRedHat · 2021-01-14T18:11:24Z

docs/source/markdown/podman-rename.1.md

+## DESCRIPTION
+Rename changes the name of an existing container.
+The old name will be freed, and will be available for use.
+This command can be run on containers that are running without difficulty.


Not sure what "without difficulty" means. Can we list the states that a container can be in instead? Or perhaps if it's easier, which state(s) the container can NOT be in?

TomSweeneyRedHat · 2021-01-14T18:16:45Z

LGTM
assuming happy tests

rhatdan · 2021-01-14T19:48:43Z

/lgtm
/hold

mheon · 2021-01-14T19:49:25Z

This is not going to go green. I am fighting documentation CI (specifically swagger and Ed's manpage checker).

Luap99 · 2021-01-14T19:55:20Z

pkg/api/server/register_containers.go

@@ -1463,5 +1490,34 @@ func (s *APIServer) registerContainersHandlers(r *mux.Router) error {
 	//   500:
 	//     $ref: "#/responses/InternalError"
 	r.HandleFunc(VersionedPath("/libpod/containers/{name}/init"), s.APIHandler(libpod.InitContainer)).Methods(http.MethodPost)
+	// swagger:operation POST /libpod/containers/{name}/rename renameContainer


Suggested change

// swagger:operation POST /libpod/containers/{name}/rename renameContainer

// swagger:operation POST /libpod/containers/{name}/rename libpod libpodRenameContainer

Try this. I have not tested it but I am confident that this fixes the swagger test.

umohnani8 · 2021-01-14T20:11:51Z

LGTM

mheon · 2021-01-14T20:25:04Z

Swagger is still angry

Basic theory: We remove the container, but *only from the DB*. We leave it in c/storage, we leave the lock allocated, we leave it running (if it is). Then we create an identical container with an altered name, and add that back to the database. Theoretically we now have a renamed container. The advantage of this approach is that it doesn't just apply to rename - we can use this to make *any* configuration change to a container that does not alter its container ID. Potential problems are numerous. This process is *THOROUGHLY* non-atomic at present - if you `kill -9` Podman mid-rename things will be in a bad place, for example. Also, we can't rename containers that can't be removed normally - IE, containers with dependencies (pod infra containers, for example). The largest potential improvement will be to move the majority of the work into the DB, with a `RecreateContainer()` method - that will add atomicity, and let us remove the container without worrying about depencies and similar issues. Potential problems: long-running processes that edit the DB and may have an older version of the configuration around. Most notable example is `podman run --rm` - the removal command needed to be manually edited to avoid this one. This begins to get at the heart of me not wanting to do this in the first place... This provides CLI and API implementations for frontend, but no tunnel implementation. It will be added in a future release (just held back for time now - we need this in 3.0 and are running low on time). This is honestly kind of horrifying, but I think it will work. Signed-off-by: Matthew Heon <[email protected]>

mheon · 2021-01-15T00:44:18Z

One test flaked. I think everything else is going to go green.

rhatdan · 2021-01-15T02:38:23Z

/hold cancel

rhatdan · 2021-01-15T02:41:18Z

/lgtm

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 12, 2021

mheon mentioned this pull request Jan 12, 2021

RFE: Add "rename" subcommand for better Docker compatibility #1925

Closed

mheon force-pushed the rename branch from 9ff93c6 to 1e3824b Compare January 12, 2021 21:21

Luap99 reviewed Jan 12, 2021

View reviewed changes

umohnani8 added the 3.0 Features label Jan 13, 2021

mheon force-pushed the rename branch 3 times, most recently from 4d9cec1 to b12932d Compare January 14, 2021 15:15

mheon changed the title ~~[WIP] Container Rename~~ Container Rename Jan 14, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2021

mheon force-pushed the rename branch from 8969cb2 to 18d55f6 Compare January 14, 2021 15:20

rhatdan reviewed Jan 14, 2021

View reviewed changes

mheon force-pushed the rename branch 2 times, most recently from bef861c to 36b45f2 Compare January 14, 2021 15:43

mheon force-pushed the rename branch 6 times, most recently from 594e6ae to 23e767e Compare January 14, 2021 17:04

Luap99 reviewed Jan 14, 2021

View reviewed changes

cmd/podman/containers/rename.go Outdated Show resolved Hide resolved

mheon force-pushed the rename branch 2 times, most recently from d55d34c to 27f33a9 Compare January 14, 2021 17:56

TomSweeneyRedHat reviewed Jan 14, 2021

View reviewed changes

mheon force-pushed the rename branch 5 times, most recently from 1603ba9 to 3a6963c Compare January 14, 2021 19:45

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 14, 2021

openshift-ci-robot assigned rhatdan Jan 14, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2021

Luap99 reviewed Jan 14, 2021

View reviewed changes

mheon force-pushed the rename branch from 3a6963c to bfa52f1 Compare January 14, 2021 20:13

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 14, 2021

mheon force-pushed the rename branch 4 times, most recently from 7b90f11 to 310baca Compare January 14, 2021 21:53

mheon force-pushed the rename branch from 310baca to 997de2f Compare January 14, 2021 23:29

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2021

openshift-merge-robot merged commit 3fcf346 into containers:master Jan 15, 2021

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container Rename #8955

Container Rename #8955

mheon commented Jan 12, 2021

mheon commented Jan 12, 2021

Luap99 Jan 12, 2021

openshift-ci-robot commented Jan 14, 2021

mheon commented Jan 14, 2021

rhatdan Jan 14, 2021

mheon Jan 14, 2021

rhatdan Jan 14, 2021

Luap99 Jan 14, 2021

mheon Jan 14, 2021

rhatdan commented Jan 14, 2021

mheon commented Jan 14, 2021

TomSweeneyRedHat Jan 14, 2021

TomSweeneyRedHat commented Jan 14, 2021

rhatdan commented Jan 14, 2021

mheon commented Jan 14, 2021

Luap99 Jan 14, 2021

umohnani8 commented Jan 14, 2021

mheon commented Jan 14, 2021

mheon commented Jan 15, 2021

rhatdan commented Jan 15, 2021

rhatdan commented Jan 15, 2021

	// swagger:operation POST /libpod/containers/{name}/rename renameContainer
	// swagger:operation POST /libpod/containers/{name}/rename libpod libpodRenameContainer

Container Rename #8955

Container Rename #8955

Conversation

mheon commented Jan 12, 2021

mheon commented Jan 12, 2021

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 14, 2021

mheon commented Jan 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Jan 14, 2021

mheon commented Jan 14, 2021

Choose a reason for hiding this comment

TomSweeneyRedHat commented Jan 14, 2021

rhatdan commented Jan 14, 2021

mheon commented Jan 14, 2021

Choose a reason for hiding this comment

umohnani8 commented Jan 14, 2021

mheon commented Jan 14, 2021

mheon commented Jan 15, 2021

rhatdan commented Jan 15, 2021

rhatdan commented Jan 15, 2021