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.

Sign up for GitHub

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
Draft

Add option to clone containers #1270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 34 additions & 2 deletions src/Containers.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import PruneUnusedContainersModal from './PruneUnusedContainersModal.jsx';

const _ = cockpit.gettext;

const ContainerActions = ({ container, healthcheck, onAddNotification, version, localImages, updateContainerAfterEvent }) => {
const ContainerActions = ({ container, containerDetail, healthcheck, onAddNotification, version, localImages, updateContainerAfterEvent, copyContainer }) => {
const Dialogs = useDialogs();
const [isActionsKebabOpen, setActionsKebabOpen] = useState(false);
const isRunning = container.State == "running";
Expand Down Expand Up @@ -256,6 +256,15 @@ const ContainerActions = ({ container, healthcheck, onAddNotification, version,
}
}

if (container && containerDetail && localImages) {
actions.push(
<DropdownItem key="clone-container"
onClick={() => copyContainer(container, containerDetail, localImages)}>
{_("Clone")}
</DropdownItem>
);
}

actions.push(<DropdownSeparator key="separator-1" />);
actions.push(
<DropdownItem key="commit"
Expand Down Expand Up @@ -361,6 +370,8 @@ class Containers extends React.Component {
onDownloadContainer = onDownloadContainer.bind(this);
onDownloadContainerFinished = onDownloadContainerFinished.bind(this);

this.copyContainer = this.copyContainer.bind(this);

window.addEventListener('resize', this.onWindowResize);
}

Expand Down Expand Up @@ -429,7 +440,11 @@ class Containers extends React.Component {
];

if (!container.isDownloading) {
columns.push({ title: <ContainerActions version={this.props.version} container={container} healthcheck={healthcheck} onAddNotification={this.props.onAddNotification} localImages={localImages} updateContainerAfterEvent={this.props.updateContainerAfterEvent} />, props: { className: "pf-c-table__action" } });
columns.push({
title: <ContainerActions version={this.props.version} container={container} containerDetail={containerDetail} healthcheck={healthcheck} onAddNotification={this.props.onAddNotification}
localImages={localImages} updateContainerAfterEvent={this.props.updateContainerAfterEvent} copyContainer={this.copyContainer} />,
props: { className: "pf-c-table__action" }
});
}

const tty = containerDetail ? !!containerDetail.Config.Tty : undefined;
Expand Down Expand Up @@ -575,6 +590,23 @@ class Containers extends React.Component {
this.setState({ showPruneUnusedContainersModal: true });
};

copyContainer(container, containerDetail, localImages) {
this.context.show(<ImageRunModal user={this.props.user}
localImages={localImages}
pod={this.props.pods[container.Pod + container.isSystem]}
registries={this.props.registries}
selinuxAvailable={this.props.selinuxAvailable}
podmanRestartAvailable={this.props.podmanRestartAvailable}
userServiceAvailable={this.props.userServiceAvailable}
systemServiceAvailable={this.props.systemServiceAvailable}
onAddNotification={this.props.onAddNotification}
version={this.props.version}
container={container}
containerDetail={containerDetail}
prefill
/>);
}

render() {
const Dialogs = this.context;
const columnTitles = [
Expand Down
2 changes: 1 addition & 1 deletion src/DynamicListForm.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export class DynamicListForm extends React.Component {
constructor(props) {
super(props);
this.state = {
list: [],
list: this.props.prefill ?? [],
};
this.keyCounter = 0;
this.removeItem = this.removeItem.bind(this);
Expand Down
138 changes: 121 additions & 17 deletions src/ImageRunModal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { Popover } from "@patternfly/react-core/dist/esm/components/Popover";
import { MinusIcon, OutlinedQuestionCircleIcon } from '@patternfly/react-icons';
import * as dockerNames from 'docker-names';

import { ErrorNotification } from './Notification.jsx';
import { ErrorNotification, WarningNotification } from './Notification.jsx';
import * as utils from './util.js';
import * as client from './client.js';
import rest from './rest.js';
Expand Down Expand Up @@ -54,10 +54,10 @@ const units = {

// healthchecks.go HealthCheckOnFailureAction
const HealthCheckOnFailureActionOrder = [
{ value: 0, label: _("No action") },
{ value: 3, label: _("Restart") },
{ value: 4, label: _("Stop") },
{ value: 2, label: _("Force stop") },
{ value: 0, label: _("No action"), apiName: "none" },
{ value: 3, label: _("Restart"), apiName: "restart" },
{ value: 4, label: _("Stop"), apiName: "stop" },
{ value: 2, label: _("Force stop"), apiName: "kill" },
];

const handleEnvValue = (key, value, idx, onChange, additem, itemCount, companionField) => {
Expand Down Expand Up @@ -175,6 +175,9 @@ export class ImageRunModal extends React.Component {
componentDidMount() {
this._isMounted = true;
this.onSearchTriggered(this.state.searchText);

if (this.props.prefill)
this.prefillModal();
}

componentWillUnmount() {
Expand Down Expand Up @@ -635,6 +638,75 @@ export class ImageRunModal extends React.Component {
return owner === systemOwner;
};

prefillModal() {
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


if (containerDetail.Config.CreateCommand) {
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.

}

const env = containerDetail.Config.Env.filter(variable => {
if (image.Env.includes(variable)) {
return false;
}

return !variable.match(/((HOME|TERM|HOSTNAME)=.*)|container=podman/);
}).map((variable, index) => {
const split = variable.split('=');
return { key: index, envKey: split[0], envValue: split[1] };
});

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.

})
: [];

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.

return { key: index, containerPath: mount.Destination, hostPath: mount.Source, mode: (mount.RW ? 'rw' : 'ro'), selinux: '' };
});

// check if memory and cpu limitations or healthcheck are used
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

? 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


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

containerName: container.Names[0] + "_copy",
env,
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

cpuShares: cpuSharesConfigure ? containerDetail.HostConfig.CpuShares : 1024,
memoryConfigure,
cpuSharesConfigure,
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

selectedImage: image,
healthcheck_command: healthcheck ? containerDetail.Config.Healthcheck.Test.join(' ') : "",
// convert to seconds
healthcheck_interval: healthcheck ? (containerDetail.Config.Healthcheck.Interval / 1000000000) : 30,
healthcheck_timeout: healthcheck ? (containerDetail.Config.Healthcheck.Timeout / 1000000000) : 30,
healthcheck_start_period: healthcheck ? (containerDetail.Config.Healthcheck.StartPeriod / 1000000000) : 0,
healthcheck_retries: healthcheck ? containerDetail.Config.Healthcheck.Retries : 3,
healthcheck_action: healthcheck ? healthCheckOnFailureAction : 0,
});
}

render() {
const Dialogs = this.context;
const { image } = this.props;
Expand Down Expand Up @@ -688,6 +760,7 @@ export class ImageRunModal extends React.Component {

const defaultBody = (
<Form>
{this.state.dialogWarning && <WarningNotification warningMessage={this.state.dialogWarning} warningDetail={this.state.dialogWarningDetail} />}
{this.state.dialogError && <ErrorNotification errorMessage={this.state.dialogError} errorDetail={this.state.dialogErrorDetail} />}
<FormGroup fieldId='run-image-dialog-name' label={_("Name")} className="ct-m-horizontal">
<TextInput id='run-image-dialog-name'
Expand Down Expand Up @@ -938,6 +1011,7 @@ export class ImageRunModal extends React.Component {
actionLabel={_("Add port mapping")}
onChange={value => this.onValueChanged('publish', value)}
default={{ IP: null, containerPort: null, hostPort: null, protocol: 'tcp' }}
prefill={this.state.publish}
itemcomponent={ <PublishPort />} />

<DynamicListForm id='run-image-dialog-volume'
Expand All @@ -948,6 +1022,7 @@ export class ImageRunModal extends React.Component {
onChange={value => this.onValueChanged('volumes', value)}
default={{ containerPath: null, hostPath: null, mode: 'rw' }}
options={{ selinuxAvailable: this.props.selinuxAvailable }}
prefill={this.state.volumes}
itemcomponent={ <Volume />} />

<DynamicListForm id='run-image-dialog-env'
Expand All @@ -958,6 +1033,7 @@ export class ImageRunModal extends React.Component {
onChange={value => this.onValueChanged('env', value)}
default={{ envKey: null, envValue: null }}
helperText={_("Paste one or more lines of key=value pairs into any field for bulk import")}
prefill={this.state.env}
itemcomponent={ <EnvVar />} />
</Tab>
<Tab eventKey={2} title={<TabTitleText>{_("Health check")}</TabTitleText>} id="create-image-dialog-tab-healthcheck" className="pf-c-form pf-m-horizontal">
Expand Down Expand Up @@ -1089,6 +1165,44 @@ export class ImageRunModal extends React.Component {
</Tabs>
</Form>
);

const cardFooter = () => {
let createRunText = _("Create and run");
let createText = _("Create");

if (this.props.prefill) {
createRunText = _("Clone and run");
createText = _("Clone");
}

return (
<>
<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

{createText}
</Button>
<Button variant='link' className='btn-cancel' onClick={Dialogs.close}>
{_("Cancel")}
</Button>
</>
);
};

const modalTitle = () => {
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

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

};

return (
<Modal isOpen
position="top" variant="medium"
Expand All @@ -1101,18 +1215,8 @@ export class ImageRunModal extends React.Component {
Dialogs.close();
}
}}
title={this.props.pod ? cockpit.format(_("Create container in $0"), this.props.pod.Name) : _("Create container")}
footer={<>
<Button variant='primary' id="create-image-create-run-btn" onClick={() => this.onCreateClicked(true)} isDisabled={!image && selectedImage === ""}>
{_("Create and run")}
</Button>
<Button variant='secondary' id="create-image-create-btn" onClick={() => this.onCreateClicked(false)} isDisabled={!image && selectedImage === ""}>
{_("Create")}
</Button>
<Button variant='link' className='btn-cancel' onClick={Dialogs.close}>
{_("Cancel")}
</Button>
</>}
title={modalTitle()}
footer={cardFooter()}
>
{defaultBody}
</Modal>
Expand Down
8 changes: 8 additions & 0 deletions src/Notification.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,11 @@ export const ErrorNotification = ({ errorMessage, errorDetail, onDismiss }) => {
</Alert>
);
};

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.

</Alert>
);
};
Loading