Skip to content

Revert "Add unit to time in "podman wait" command."#1150

Closed
dtantsur wants to merge 1 commit intoopenshift-metal3:masterfrom
dtantsur:revert-1148-hroyrh-patch-1
Closed

Revert "Add unit to time in "podman wait" command."#1150
dtantsur wants to merge 1 commit intoopenshift-metal3:masterfrom
dtantsur:revert-1148-hroyrh-patch-1

Conversation

@dtantsur
Copy link
Copy Markdown
Member

1000ms is not a valid syntax, an integer is expected there.

$ sudo podman wait -i 1000ms ipa-downloader
Error: invalid argument "1000ms" for "-i, --interval" flag: strconv.ParseUint: parsing "1000ms": invalid syntax

Reverts #1148

@openshift-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign cybertron after the PR has been reviewed.
You can assign the PR to them by writing /assign @cybertron in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

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

@dtantsur
Copy link
Copy Markdown
Member Author

/cc @hroyrh

@openshift-ci-robot
Copy link
Copy Markdown

@dtantsur: GitHub didn't allow me to request PR reviews from the following users: hroyrh.

Note that only openshift-metal3 members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @hroyrh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@hardys
Copy link
Copy Markdown

hardys commented Nov 16, 2020

@dtantsur seems we have different and incompatible behavior with various podman versions - what version are you testing with?

@stbenjam
Copy link
Copy Markdown
Member

This seems to work for me on an updated CentOS 8. And it's required on RHEL 8.3.

The history of this is here: https://bugzilla.redhat.com/show_bug.cgi?id=1897282

If there's conflicting behavior that we may need an if/else dependent on the podman version, not a direct revert.

@dtantsur
Copy link
Copy Markdown
Member Author

I'm using podman on CentOS 8, there were no updates the last time I checked.

@dtantsur
Copy link
Copy Markdown
Member Author

It's still not in CentOS 8:

$ sudo dnf update --refresh
CentOS-8 - AppStream                                                                                                                                                               4.5 kB/s | 4.3 kB     00:00    
CentOS-8 - Base                                                                                                                                                                     14 kB/s | 3.9 kB     00:00    
CentOS-8 - Extras                                                                                                                                                                  5.0 kB/s | 1.5 kB     00:00    
Extra Packages for Enterprise Linux Modular 8 - x86_64                                                                                                                              46 kB/s |  13 kB     00:00    
Extra Packages for Enterprise Linux 8 - x86_64                                                                                                                                     302 kB/s |  14 kB     00:00    
Dependencies resolved.
Nothing to do.
Complete!

$ dnf info podman
Last metadata expiration check: 17:19:16 ago on Mon 16 Nov 2020 10:43:50 AM EST.
Installed Packages
Name         : podman
Version      : 1.6.4
Release      : 10.module_el8.2.0+305+5e198a41
Architecture : x86_64
Size         : 55 M
Source       : podman-1.6.4-10.module_el8.2.0+305+5e198a41.src.rpm
Repository   : @System
From repo    : AppStream
Summary      : Manage Pods, Containers and Container Images
URL          : https://podman.io/
License      : ASL 2.0
Description  : podman (Pod Manager) is a fully featured container engine that is a simple daemonless tool.  podman provides a Docker-CLI comparable command line that eases the transition from other container
             : engines and allows the management of pods, containers and images.  Simply put: alias docker=podman.  Most podman commands can be run as a regular user, without requiring additional privileges.
             : 
             : podman uses Buildah(1) internally to create container images. Both tools share image (not container) storage, hence each can use or manipulate images (but not containers) created by the other.
             : 
             : Manage Pods, Containers and Container Images
             : libpod Simple management tool for pods, containers and images

$ sudo podman wait -i 1000ms 
Error: invalid argument "1000ms" for "-i, --interval" flag: strconv.ParseUint: parsing "1000ms": invalid syntax

Can we please not rely on RHEL-only features that have not been released? :-/

Comment thread 04_setup_ironic.sh
-v $IRONIC_DATA_DIR:/shared ${IRONIC_IPA_DOWNLOADER_LOCAL_IMAGE} /usr/local/bin/get-resource.sh

sudo podman wait -i 1000ms ipa-downloader
sudo podman wait -i 1000 ipa-downloader
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since it seems there's no format currently which works for both centos and RHEL, would you mind putting a conditional here please?

I guess checking the podman version is safest since it's probably only a matter of time before centos picks up the newer podman with the required unit (until the bz mentioned by Stephen is fixed)

@hardys
Copy link
Copy Markdown

hardys commented Nov 17, 2020

Can we please not rely on RHEL-only features that have not been released? :-/

Clearly that wasn't the intention, it's a bug - we'll have to work around it with a conditional until both distros have consistent versions/behavior I think.

@dtantsur
Copy link
Copy Markdown
Member Author

I guess checking the podman version is safest

Do we know the version this feature was introduced?

it's probably only a matter of time before centos picks up the newer podman with the required unit

Not necessary, it may still stay in the non-default 2.0 stream, with 1.6.4 staying the default.

@dtantsur
Copy link
Copy Markdown
Member Author

Closing in favour of #1151

@dtantsur dtantsur closed this Nov 17, 2020
@stbenjam
Copy link
Copy Markdown
Member

Can we please not rely on RHEL-only features that have not been released? :-/

We aren't. CI is running CentOS, and it passed with the units. The broken podman is also in Fedora, this wasn't only released in RHEL.

@stbenjam
Copy link
Copy Markdown
Member

Can we please not rely on RHEL-only features that have not been released? :-/

We aren't. CI is running CentOS, and it passed with the units. The broken podman is also in Fedora, this wasn't only released in RHEL.

Ah this code path only happens with local images. Perhaps we need a CI job to make sure local image code keeps working.

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.

4 participants