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

Generic Container Timeout support #544

Closed
eak13 opened this issue May 14, 2021 · 11 comments
Closed

Generic Container Timeout support #544

eak13 opened this issue May 14, 2021 · 11 comments
Assignees
Labels
1-Core Relates to airshipctl core components (i.e. go code) enhancement New feature or request priority/medium Default priority for items size l
Milestone

Comments

@eak13
Copy link

eak13 commented May 14, 2021

As laid out in #533, timeouts are currently ignored by the executors. To begin provide this support, this issue asks the following:

  1. Update the Airshiptctl API interface to add a variable to pass a timeout value (likely seconds).
  2. Update the the Generic Container framework accept the timeout variable & then inject it into the container to prompt the operation to finish gracefully. At this point in time it will not force kill the process.
@eak13 eak13 added enhancement New feature or request triage Needs evaluation by project members labels May 14, 2021
@teoyaomiqui
Copy link
Contributor

From the proposed airship design discussion, here are key directions:

airshipctl container executor implementation has to pass the timeout to container implementation:

There are two ways to do so:

  1. Pass timeout as an environment variable to the container implementation as parameters to Run() function. And then inject timeout as ENV variable into the actual container to make it aware of the timeout.
  • The main benefit of this patchset is that if in the future we would want to kill the container by airshipctl forcefully, we can do so almost effortlessly.
  • The downside would be that we need to change Container API interface.
  1. Inject the timeout as an environment variable into the container interface by extending ENV variable slice similarly to the kubeconfig context variable here.
  • The benefit of this approach will not require any changes to Container API interface.
  • The downside would be that if we decide to delete the container forcefully by airshipctl, we will have to extract this environment variable ourselves somehow.

Timeout needs to be respected by the containers that we run.

We use 3 main images for those containers currently:

  1. Toolbox
  1. Cloud-init container for the ephemeral host
  • Container executor YAML definition

  • Image source code

  • Dockerfile

    This is an intermediate container that is used only to prepare files for the image-builder.

    We should remove this container and instead build a wrapper around the image-builder container that would allow us to execute it without the need to have two different phases, one for cloud-init, another one for image-builder

  1. Image-builder

@vladiskuz
Copy link
Contributor

Can someone assign it to me?

@jezogwza jezogwza added this to the v2.1 milestone May 19, 2021
@jezogwza jezogwza added 1-Core Relates to airshipctl core components (i.e. go code) priority/medium Default priority for items and removed triage Needs evaluation by project members labels May 19, 2021
@jezogwza jezogwza modified the milestones: v2.1, Future May 19, 2021
@airshipbot
Copy link

airshipbot commented May 24, 2021

Related Change #792801

Subject: Pass TIMEOUT_SECONDS env variable to generic container API
Link: https://review.opendev.org/c/airship/airshipctl/+/792801
Status: NEW
Owner: Vladislav Kuzmin (vkuzmin@mirantis.com)

Approvals

Code-Review
! None
Verified
+1 Zuul
+1 ATT Airship2.0 CI
Workflow
! None

Last Updated: 2021-05-28 05:39:55 CDT

@lb4368 lb4368 added the size l label Jun 7, 2021
@jezogwza jezogwza modified the milestones: Future, v2.1 Jul 7, 2021
@vladiskuz
Copy link
Contributor

Move cloud-init KRM to image-builder https://review.opendev.org/c/airship/image-builder/+/802046

@joshuaherrera
Copy link
Contributor

It looks like @vladiskuz may be working on something else currently, I can take on finishing this as I have capacity.

@eak13
Copy link
Author

eak13 commented Sep 9, 2021

@joshuaherrera added you to this issue

@vladiskuz
Copy link
Contributor

@joshuaherrera
This issue is almost done. I need to change a minor comment here https://review.opendev.org/c/airship/airshipctl/+/792801
Also we have another patch set for container timeout https://review.opendev.org/c/airship/airshipctl/+/805619

@joshuaherrera
Copy link
Contributor

@vladiskuz Since it is close to completion, I will unassign myself so you can finish this issue. Apologies.

@joshuaherrera joshuaherrera removed their assignment Sep 10, 2021
@michaelfix
Copy link

@vladiskuz , @joshuaherrera , Relates to: #597 is mentioned in the commit message for the PS above - https://review.opendev.org/c/airship/airshipctl/+/805619. Is 805619 related to this 544 issue here, or 597?

@gtsteffaniak
Copy link

please assign to me

@raliev12
Copy link
Contributor

Per Slack discussion, please assign this issue to me for finalizing.

airshipbot pushed a commit that referenced this issue Nov 19, 2021
Allows to get rid of generic containers.

Change-Id: I7f4485ded4dd39ec44a0746904b626b4a07d0f80
Signed-off-by: Ruslan Aliev <raliev@mirantis.com>
Relates-To: #544
Relates-To: #545
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-Core Relates to airshipctl core components (i.e. go code) enhancement New feature or request priority/medium Default priority for items size l
Projects
None yet
Development

No branches or pull requests

10 participants