Allow podman wait to wait on healthy state

[svenstaro]

/kind feature

Description

I'd like for podman wait to be able to wait until a container is healthy. Currently we are using an external blocking command that's exactly the same as the health command and I think this could be made to be more elegant. This scenario crops up if starting a container in detached mode for integration testing, for instance.

I think this might be easy to add considering podman is already internally aware of the health state. I know that podman wait has the tag line "Block until one or more containers stop and then print their exit codes." but why limit it to just that?

Steps to reproduce the issue:

1. podman wait --condition healthy

Describe the results you received:

Currently this is not supported.

Describe the results you expected:

Would be great if podman wait could block until the container is considered healthy.

[rhatdan]

@vrothberg @baude @mheon Thoughts?

[vrothberg]

I like the idea, thanks for proposing

[keonchennl]

May I work on this?

[rhatdan]

Sure

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[svenstaro]

@keonchennl Did you get some work done on this?

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[rhatdan]

@vrothberg care to take this over if you have time or hand it to an intern?

[vrothberg]

Sure, I can take a look.

[vrothberg]

@mheon, are you cool if I add new container states (ContainerStatus{Healthy,Unhealthy})?

Unfortunately, the (remote) API uses []define.ContainerStatus for the waiting conditions instead of strings. Without breaking the API or ugly workarounds, I don't see another way.

[vrothberg]

@mheon ping

[mheon]

Github somehow ate my response. Copying and pasting:

Feels like a bad idea, we'd be enshrining a hack into the API and forcing us to support it for the foreseeable future. Maybe add an extra bool flag for "Healthy" state, so we can wait on ContainerStateRunning and Healthy=true?

[vrothberg]

Feels like a bad idea, we'd be enshrining a hack into the API and forcing us to support it for the foreseeable future. Maybe add an extra bool flag for "Healthy" state, so we can wait on ContainerStateRunning and Healthy=true?

Not sure how to do that without breaking the (remote) API and the bindings:

type WaitOptions struct {                 
        Condition []define.ContainerStatus
        Interval  time.Duration           
        Latest    bool                    
}                                         

[vrothberg]

@rhatdan I am freeing this for interns since I'll be on PTO next week but let's first discuss what needs to be done before re-assigning it.

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[svenstaro]

I still have interest in getting this feature request filled and there seemed to be some interest from upstream to implement it.

[rhatdan]

@ashley-cui @vrothberg @mheon @umohnani8 Something for interns?

[vrothberg]

Maybe but it must be groomed/designed. There is also a request to support restart policies (i.e., restart when unhealthy) which would be considered.

[mheon]

IMO: Add a Healthy bool to the existing struct (or create a new, separate array just of strings, to allow more versatile argument passing), and allow that to be set instead of the existing conditions array. That allows us to not make a breaking change, but support new functionality.

[rhatdan]

Seems like we would have three states, unkown, healthy and unhealthy. So I don't think a bool would work

[mheon]

Feels like a bad idea, we'd be enshrining a hack into the API and forcing us to support it for the foreseeable future. Maybe add an extra bool flag for "Healthy" state, so we can wait on ContainerStateRunning and Healthy=true?

…

On Fri, May 27, 2022 at 7:50 AM Valentin Rothberg ***@***.***> wrote: @mheon <https://github.com/mheon>, are you cool if I add new container states (ContainerStatus{Healthy,Unhealthy})? Unfortunately, the (remote) API uses []define.ContainerStatus for the waiting conditions instead of strings. Without breaking the API or ugly workarounds, I don't see another way. — Reply to this email directly, view it on GitHub <#13627 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCCXJUDGY32YVKSBOOLVMCZGJANCNFSM5RPMVB4Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[svenstaro]

Has there been any movement on this?

[rhatdan]

Not that I am aware of, interested in opening a PR?

[svenstaro]

There seems to have been some back and forth on the design of this and until that's decided on ideally by the project, I think a PR would likely not get merged.

[rhatdan]

I think if you opened a PR matching what @mheon suggested it would be more likely to get merged.

[vrothberg]

I'm no fan of adding a single boolean since it allows for only one state but I think we need means to further extend the wait endpoint to handle more conditions.

Currently, the endpoint uses define.ContainerStatus which @mheon does not want to extend with non-status conditions (e.g., health):

type WaitOptions struct {                 
        Condition []define.ContainerStatus
        Interval  time.Duration           
        Latest    bool                    
}                                         

Hence, I think we should add a new field Conditions []string:

type WaitOptions struct {                 
        // Condition is deprecated, please use the `Conditions` field instead.
        Condition []define.ContainerStatus
        // ...
        Conditions []string
        Interval  time.Duration           
        Latest    bool                    
}  

The untyped string can be easier dealt with and extended.

Since this is blocking #6160, I'd like to push things forward.

@Luap99 @mheon @rhatdan WDYT?

[Luap99]

Adding a new field sounds good to me.

[danishprakash]

@vrothberg are you still looking into this?

[vrothberg]

Yes, currently waiting for feedback from more maintainers. Once the design is agreed on, we can implement it.

[mheon]

Adding a new field SGTM. Was thinking we wouldn't deprecate the old one, but I suppose it doesn't make sense to have two separate ways of requesting conditions, so I won't argue.

[vrothberg]

Great, I'll take a stab at it.

[vrothberg]

Opened #18974