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

Add option to clone containers #1270

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tomasmatus
Copy link
Member

@tomasmatus tomasmatus commented Apr 17, 2023

Implemets: #363
Adds 'Clone' button to container actions which opens prefilled create container modal.

src/ImageRunModal.jsx Fixed Show fixed Hide fixed
: [];

const volumes = containerDetail.Mounts.map((mount, index) => {
// podman does not expose SELinux labels
Copy link
Member

Choose a reason for hiding this comment

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

I thought it did not, but then I thought I was proven wrong. Is there an issue for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm right, there's MountLabel. I just need to figure out what it means.

this.setState({
dialogWarning: _("This container was not created by cockpit"),
dialogWarningDetail: _("Some options may not be copied to the new container."),
});
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this something we need to always show? We won't support all options podman has as we won't ever have a feature complete UI.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's okay to not show it when the container is created through cockpit. On the other hand the "check" if it is made by cockpit isn't 100% robust as it only checks whether it was created through commandline or podman API.

@martinpitt
Copy link
Member

/packit build

@tomasmatus tomasmatus force-pushed the copy_container branch 3 times, most recently from 9c2f8d3 to 371704c Compare April 19, 2023 13:43
const memoryConfigure = containerDetail.HostConfig.Memory > 0;
const cpuSharesConfigure = containerDetail.HostConfig.CpuShares > 0;
const healthcheck = !!containerDetail.Config.Healthcheck;
const healthCheckOnFailureAction = (this.props.version.split(".")) >= [4, 3, 0]

Check warning

Code scanning / CodeQL

Implicit operand conversion

This expression will be implicitly converted from object to number or string.
Copy link
Member Author

Choose a reason for hiding this comment

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

lgtm

@KKoukiou KKoukiou self-requested a review April 27, 2023 09:59
@KKoukiou KKoukiou force-pushed the copy_container branch 2 times, most recently from ac2abc6 to 01fd85d Compare April 28, 2023 09:25
src/ImageRunModal.jsx Outdated Show resolved Hide resolved
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

This just creates a new container by copying the configuration from another container. However podman offers the 'clone' API, which does this exact thing internally.
Did we consider using that instead?

@garrett
Copy link
Member

garrett commented May 3, 2023

I discussed this a while back (in 2021) @ #363 (comment):

It basically comes down to this; there are basically three things people want to do:

  1. "Clone" a container, where it's almost the same, but different. The original would still exist. (This is also almost like a template.)

    • Mandatory editing with a prefilled version of the create modal dialog, with some values needing to be changed (name) and others should be changed if the container would run at the same time (ports and read/write volumes).
    • Duplicates the container, but with changes.
  2. "Edit" a conainer's settings (recreate the same container again, but with a small change, replacing the original container)

    • Editing with a prefilled version of the create modal dialog. Changes are all optional.
    • Replaces the original container.
  3. "Update" a container by using the exact same settings, but on a newer version of the image. (It might be slightly edited, but usually not.)

    • Editing would be optional.
    • Pulls new image.
    • Replaces the original container.

All three items replicate an existing container's configuration. The differences are all around if it's in addition to and if the configuration changes.

There's an additional 4th option when pods are concerned (now that we have pod support):

  1. "Move" a container into (or out of) a pod group.
    • Don't edit.
    • Remove the original container.

All of these should probably run the container if the container was running before performing the action. (That is: If the container is running, queue up what we're going to do, perform the action, and make sure it's running after. If it was stopped before, then don't start it.)

However podman offers the 'clone' API, which does this exact thing internally.

Yes, podman container clone makes sense for some of the above, such as moving. If we only support a limited amount of "editing" (as podman clone doesn't let you adjust everything... but it does let you change the name, CPU settings, and other minor things), then it could make sense for cloning too. It doesn't make sense for editing.

As for updating? podman container clone might actually make sense, if I understand it correctly. https://docs.podman.io/en/latest/markdown/podman-container-clone.1.html:

podman container clone [options] container name image

This command takes three arguments: the first being the container ID or name to clone, the second argument in this command can change the name of the clone from the default of $ORIGINAL_NAME-clone, and the third is a new image to use in the cloned container.

The last example is this one:

# podman container clone 2d4d4fca7219b4437e0d74fcdc272c4f031426a6eacd207372691207079551de new_name fedora
Resolved "fedora" as an alias (/etc/containers/registries.conf.d/shortnames.conf)
Trying to pull registry.fedoraproject.org/fedora:latest...
Getting image source signatures
Copying blob c6183d119aa8 done
Copying config e417cd49a8 done
Writing manifest to image destination
Storing signatures
5a9b7851013d326aa4ac4565726765901b3ecc01fcbc0f237bc7fd95588a24f9

...but what if the new name is the same as the old name? And --destroy was given to destroy the original? Would that replace the container with a refreshed version?

Or is there a better way in Podman to recreate a container with the exact same settings but with a new image? There is https://docs.podman.io/en/latest/markdown/podman-auto-update.1.html but it relies on systemd and is supposed to be automatic with a system service based on a timer, instead of a manual update. It does have a way to check if there's a new version with podman auto-update --dry-run --format "{{.Image}} {{.Updated}}" it seems. Perhaps we can do some behind-the-scenes stuff to manually update with podman-auto-update? 🤷


Anyway, back to this PR and @KKoukiou's question about cloning with editing versus podman container clone: I think this PR is trying to answer what I listed above as # 1, whereas podman container clone does not let you edit values outside of the name (which would be mandatory regardless) and some basic settings (such as some CPU and IO related ones).

@jelly jelly mentioned this pull request May 24, 2023
@tomasmatus tomasmatus force-pushed the copy_container branch 3 times, most recently from 3c713c3 to dd89f1e Compare June 21, 2023 08:22
Adds 'Clone' button to container actions which opens
prefilled create container modal.
const container = this.props.container;
const containerDetail = this.props.containerDetail;
const image = this.props.localImages.find(img => img.Id === container.ImageID);
const owner = container.isSystem ? 'system' : this.props.user;
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test. Details

const healthcheck = !!containerDetail.Config.Healthcheck;
const healthCheckOnFailureAction = (this.props.version.split(".")) >= [4, 3, 0]
? HealthCheckOnFailureActionOrder.find(item => item.apiName === containerDetail.Config.HealthcheckOnFailureAction).value
: null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test. Details

: null;

this.setState({
command: container.Command ? container.Command.join(' ') : "",
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test. Details

hasTTY: containerDetail.Config.Tty,
publish,
// memory in MB
memory: memoryConfigure ? (containerDetail.HostConfig.Memory / 1000000) : 512,
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test. Details

volumes,
owner,
// unless-stopped: Identical to always
restartPolicy: containerDetail.HostConfig.RestartPolicy.Name === 'unless-stopped' ? 'always' : containerDetail.HostConfig.RestartPolicy.Name,
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test. Details

<Button variant='primary' id="create-image-create-run-btn" onClick={() => this.onCreateClicked(true)} isDisabled={(!image && selectedImage === "")}>
{createRunText}
</Button>
<Button variant='secondary' id="create-image-create-btn" onClick={() => this.onCreateClicked(false)} isDisabled={(!image && selectedImage === "")}>
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test. Details

let titleText = _("Create container");

if (this.props.prefill && this.props.pod)
titleText = _("Clone container in $0");
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test. Details

if (this.props.prefill && this.props.pod)
titleText = _("Clone container in $0");
else if (this.props.prefill)
titleText = _("Clone container");
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test. Details

else if (this.props.pod)
titleText = _("Create container in $0");

return this.props.pod ? cockpit.format(titleText, this.props.pod.Name) : titleText;
Copy link
Contributor

Choose a reason for hiding this comment

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

This added line is not executed by any test. Details

Copy link
Member

@jelly jelly left a comment

Choose a reason for hiding this comment

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

I understand the use case of this feature, but this is also somewhat of a maintenance burden to as every new feature needs to be supported in the cloning feature as well. I'm not sure if there is a good way to keep track of that.


const publish = container.Ports
? container.Ports.map((port, index) => {
return { key: index, IP: port.hostIP || port.host_ip, containerPort: port.containerPort || port.container_port, hostPort: port.hostPort || port.host_port, protocol: port.protocol };
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to split this off in multiple lines to make it a bit more readable.

export const WarningNotification = ({ warningMessage, warningDetail }) => {
return (
<Alert isInline variant='warning' title={warningMessage}>
{ warningDetail && <p> {_("Warning message")}: <samp>{warningDetail}</samp> </p> }
Copy link
Member

Choose a reason for hiding this comment

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

image

I don't think we need to show Warning message: this looks strange

Copy link
Member

Choose a reason for hiding this comment

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

Also not sure if we should use samp, we don't use that anywhere. And actually, maybe we should use

pkg/lib/cockpit-components-inline-notification.jsx instead so basically ModalError.

Copy link
Member

Choose a reason for hiding this comment

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

The warning isn't that it was not created by cockpit, the warning is that some options may not be able to be copied. But: Which options? Is it possible to find out?

<!> Some options might not be copied to the new container (View details)

...And then it'd expand to show which options exist but aren't copied.

We don't need to mention anything about Cockpit.

Copy link
Member

Choose a reason for hiding this comment

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

It is possible, but that makes this even more complex problem. We have the full command line - and if we know that it was created though cmdline then we show this message. Parsing it is no easy job. We could maybe show the command line for reminder how the container was created?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the warning message needs to be better. This "check" works in a way that it only sees if container was created using the API or from a commandline. Showing which options are not copied would require parsing the original command...

Cockpit shouldn't be mentioned at all as it's supposed to stay unbranded.

We could maybe show the command line for reminder how the container was created?

maybe this is a good idea?

Copy link
Member

Choose a reason for hiding this comment

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

Note that we don't use the command line to create containers, nor can you find it out for an existing one. You'd have to create an "inspect json → command line" translator, and that's a big no-no for cockpit-podman.

But also, even if you ignore the container state, you cannot just duplicate the command line. There is no general way to treat volumes during a "clone". It's reasonable to kill the original container and then start a new one with the same volumes, but you can't have two of them at the same time.

if auth:
# Add restart policy
b.set_val("#run-image-dialog-restart-policy", "on-failure")
b.set_input_text('#run-image-dialog-restart-retries input', '2')
Copy link
Member

Choose a reason for hiding this comment

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

Restart policy set to 2, below we seem to check for 5?

Copy link
Member

Choose a reason for hiding this comment

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

image

image

So this seems like a bug

b.wait_visible("div.pf-c-modal-box")

# container was not created through cockpit, warning should be present
b.wait_visible(".pf-c-form .pf-c-alert")
Copy link
Member

Choose a reason for hiding this comment

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

This should also verify what's in the alert.

self.waitContainerRow("busybox_copy")

ports = self.execute(auth, "podman inspect --format '{{.NetworkSettings.Ports}}' busybox_copy")
self.assertEqual(ports, 'map[]\n')
Copy link
Member

Choose a reason for hiding this comment

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

Instead of checking for \n. Please call .strip()

@martinpitt martinpitt marked this pull request as draft August 23, 2023 03:24
@martinpitt
Copy link
Member

@tomasmatus are you still working on this, or should we close this due to being conceptually too difficult?

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.

7 participants