Change action for config with `restart` in container_manage to `created` #743

cubeek · 2024-08-30T20:00:03Z

Don't start container if restarted is set

If the container_manage is called with restart set, it means the
systemd handles the restart. In such case, we do not want containers to
be started because they will be started later by the systemd.

Resolves: OSPRH-9812

bogdando · 2024-09-02T12:44:15Z

/lgtm

slagle

LGTM. the containers are started by the container_systemd task that follows this task, so everything will be started by systemd

cubeek · 2024-09-05T15:14:11Z

I'm going to remove the WIP tag as this seems to be good to go.

openshift-ci · 2024-09-05T16:39:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cubeek, slagle

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~~ [slagle]

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

slagle · 2024-09-05T16:39:35Z

/retest

rabi · 2024-09-06T04:45:21Z

plugins/modules/edpm_container_manage.py

@@ -283,7 +283,7 @@ def exec_container(self, name, config):
    def manage_container(self, name, config):
        opts = {
            'name': name,
-            'state': "started",
+            'state': "created",


Maybe we should add opts['state'] = "created" after L311 instead (i.e when systemd handles the restart) unless all containers in edpm are expected to be managed by systemd?

This would also make start_order for containers irrelevant, though it seems we concurently manage every container individually with podman_container_manage and start_order we've in the configs is kind of useless.

You mean to override to created if the config comes with restart?

Yes, that is used to filter service configs for creating systemd units. https://github.com/openstack-k8s-operators/edpm-ansible/blob/main/roles/edpm_container_manage/tasks/create.yml#L45

I'm not sure I understand. How I read the linked code is that it filters out configs from all_containers_hash based on restart=[some specific values]. But the all_containers_hash map comes from container_config_data module: https://github.com/openstack-k8s-operators/edpm-ansible/blob/main/roles/edpm_container_manage/tasks/main.yml#L30

As for the start_order - it confuses me again how is this related. The goal of this patch is to not start the containers after they are created because systemd will start them through its service files.

But the all_containers_hash map comes from container_config_data module

Thats right.. But, let me try to explain it better...

step 1: all_containers_hash comes from container_config_data module
step 2: edpm_container_manage module transforms that a container config and calls PodmanManager to create or start the containers.
step 3: container_systemd action plugin called to create the systemd units by fitering the containers that has restart=something.

What I'm suggesting is that rather than assuming that all containers would require systemd units and hence should only be created (state=created) by PodmanManager, we should only set state=created only for those configs that has restart=<always, unless-stopped> as those are the only ones where we would create systemd units and let PodmanManager start the other containers with (state=started).

Hope that calrifies.

As for the start_order - it confuses me again how is this related

start_order is relevant as container configs are given to PodmanManager sorted by start_order . If you start all of the containers with systemd (and not by PodmanManager) there would be no order. However, start_order is not important atm in edpm-ansible (all configs have start_order 0) and I had removed them from all service container configs, as we start all services as standalone services individually one after the other and there is no order expected.

I'm still missing why would we need to override. The state is a parameter that can be passed from the caller. This patch just changes it to created by default but it still can be overridden by passing "state"="restarted" by the caller and it will be updated on L299. Those who do not want to manage container by the systemd can still pass "restarted". By setting the state after the ops.update() call we would take this functionality away, no?

This patch just changes it to created by default in edpm-ansible

We're changing the default and not only for cases where we expect services to be managed by systemd. The default in podman module is started[1] and normally services don't specify state in config and this the behavior from tripleo-ansibe.

By setting the state after the ops.update() call we would take this functionality away, no?

When we want services to be managed by systemd, passing state='started/stopped' to this module has no meaning and for cleanup (state=absent), there is a separate role 'edpm_container_rm` that does not use this module[2].

If we still want to change this default, we should document that "By default containers are to be managed by systemd and have to provide restart=<>".Also, users have to use state=started when using this module if they don't want containers to be managed by systemd". If by chance user does not set restart=<> and state=started, the container would just be created and neither podman nor systemd would start it.

[1] https://github.com/containers/ansible-podman-collections/blob/master/plugins/module_utils/podman/podman_container_lib.py#L22
[2] https://github.com/openstack-k8s-operators/edpm-ansible/blob/main/roles/edpm_container_rm/tasks/edpm_podman_container_rm.yml#L87-L89

Thanks Rabi for your patience. I hope I got it right

rabi

/lgtm

rabi · 2024-10-15T11:06:28Z

The commit message would need a change and also there is a merge commit in the PR.

cubeek · 2024-10-16T12:15:57Z

The commit message would need a change and also there is a merge commit in the PR.

Not sure if you meant really the commit message or the text in the PR. I copied the message from the commit message to be the same.

If the container_manage is called with `restart` set, it means the systemd handles the restart. In such case, we do not want containers to be started because they will be started later by the systemd. Resolves: OSPRH-9812 Signed-off-by: Jakub Libosvar <[email protected]>

rabi · 2024-10-16T12:34:30Z

/lgtm

softwarefactory-project-zuul · 2024-10-16T16:20:43Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/f0fc96b47c444185a2d9f8895bbc2d26

fao89 · 2024-10-16T17:57:57Z

recheck

openshift-ci bot added the do-not-merge/work-in-progress label Aug 30, 2024

openshift-ci bot requested review from frenzyfriday and stuggi August 30, 2024 20:00

karelyatin mentioned this pull request Sep 2, 2024

Don't use log files on EDPM nodes for networking related services #726

Merged

openshift-ci bot assigned bogdando Sep 2, 2024

openshift-ci bot added the lgtm label Sep 2, 2024

slagle reviewed Sep 5, 2024

View reviewed changes

cubeek changed the title ~~WIP: Change default action of container_manage to created~~ Change default action of container_manage to created Sep 5, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 5, 2024

cubeek force-pushed the OSPRH-9812 branch from 80e26ca to 8325197 Compare September 5, 2024 15:15

openshift-ci bot removed the lgtm label Sep 5, 2024

slagle approved these changes Sep 5, 2024

View reviewed changes

openshift-ci bot assigned slagle Sep 5, 2024

openshift-ci bot added the lgtm label Sep 5, 2024

openshift-ci bot added the approved label Sep 5, 2024

rabi reviewed Sep 6, 2024

View reviewed changes

cubeek force-pushed the OSPRH-9812 branch from 8325197 to 192bf2f Compare October 8, 2024 14:21

openshift-ci bot removed the lgtm label Oct 8, 2024

fao89 requested a review from rabi October 15, 2024 11:03

rabi reviewed Oct 15, 2024

View reviewed changes

openshift-ci bot assigned rabi Oct 15, 2024

openshift-ci bot added the lgtm label Oct 15, 2024

rabi removed the lgtm label Oct 15, 2024

cubeek force-pushed the OSPRH-9812 branch from 94bab42 to 4cfb909 Compare October 16, 2024 12:16

rabi changed the title ~~Change default action of container_manage to created~~ Change action for config with restart in container_manage to created Oct 16, 2024

openshift-ci bot added the lgtm label Oct 16, 2024

openshift-merge-bot bot merged commit d2f53c0 into openstack-k8s-operators:main Oct 16, 2024
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change action for config with `restart` in container_manage to `created` #743

Change action for config with `restart` in container_manage to `created` #743

cubeek commented Aug 30, 2024 •

edited

Loading

bogdando commented Sep 2, 2024

slagle left a comment

cubeek commented Sep 5, 2024

openshift-ci bot commented Sep 5, 2024

slagle commented Sep 5, 2024

rabi Sep 6, 2024

rabi Sep 6, 2024 •

edited

Loading

cubeek Sep 9, 2024

rabi Sep 10, 2024 •

edited

Loading

cubeek Oct 1, 2024

rabi Oct 3, 2024

rabi Oct 3, 2024 •

edited

Loading

cubeek Oct 4, 2024

rabi Oct 5, 2024

cubeek Oct 8, 2024

rabi left a comment

rabi commented Oct 15, 2024

cubeek commented Oct 16, 2024

rabi commented Oct 16, 2024

softwarefactory-project-zuul bot commented Oct 16, 2024

fao89 commented Oct 16, 2024

Change action for config with restart in container_manage to created #743

Change action for config with restart in container_manage to created #743

Conversation

cubeek commented Aug 30, 2024 • edited Loading

bogdando commented Sep 2, 2024

slagle left a comment

Choose a reason for hiding this comment

cubeek commented Sep 5, 2024

openshift-ci bot commented Sep 5, 2024

slagle commented Sep 5, 2024

rabi Sep 6, 2024

Choose a reason for hiding this comment

rabi Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

cubeek Sep 9, 2024

Choose a reason for hiding this comment

rabi Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

cubeek Oct 1, 2024

Choose a reason for hiding this comment

rabi Oct 3, 2024

Choose a reason for hiding this comment

rabi Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

cubeek Oct 4, 2024

Choose a reason for hiding this comment

rabi Oct 5, 2024

Choose a reason for hiding this comment

cubeek Oct 8, 2024

Choose a reason for hiding this comment

rabi left a comment

Choose a reason for hiding this comment

rabi commented Oct 15, 2024

cubeek commented Oct 16, 2024

rabi commented Oct 16, 2024

softwarefactory-project-zuul bot commented Oct 16, 2024

fao89 commented Oct 16, 2024

Change action for config with `restart` in container_manage to `created` #743

Change action for config with `restart` in container_manage to `created` #743

cubeek commented Aug 30, 2024 •

edited

Loading

rabi Sep 6, 2024 •

edited

Loading

rabi Sep 10, 2024 •

edited

Loading

rabi Oct 3, 2024 •

edited

Loading