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

[v4.7] Fix env-file regression + more backports #20269

Merged
merged 7 commits into from
Oct 5, 2023

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Oct 5, 2023

Backports of

#20268
#20256
#20250
#20056

Does this PR introduce a user-facing change?

Fix regression in --env-file handling.
Fixed a bug where podman inspect would fail when stat'ing a device failed.
The network list compat API endpoint is now much faster.

Luap99 added 7 commits October 5, 2023 12:13
This reverts commit 7ce654f.

see containers#19565

Signed-off-by: Paul Holzinger <[email protected]>
This reverts commit 170a786.

This was a breaking change and users are hitting it,
see containers#19565

Fixes containers#19565

Signed-off-by: Paul Holzinger <[email protected]>
Now that the newline env file change is reverted we have to adapt the
tests.

Signed-off-by: Paul Holzinger <[email protected]>
When we walk the /dev tree we need to lookup all device paths. Now in
order to get the major and minor version we have to actually stat each
device. This can again fail of course. There is at least a race between
the readdir at stat call so it must ignore ENOENT errors to avoid
the race condition as this is not a user problem. Second, we should
also not return other errors and just log them instead, returning an
error means stopping the walk and returning early which means inspect
fails with an error which would be bad.

Also there seems to be cases were ENOENT will be returned all the time,
e.g. when a device is forcefully removed. In the reported bug this is
triggered with iSCSI devices.

Because the caller does already lookup the device from the created map
it reports a warning there if the device is missing on the host so it
is not a problem to ignore a error during lookup here.

[NO NEW TESTS NEEDED] Requires special device setup to trigger
consistentlyand we cannot do that in CI.

Fixes https://issues.redhat.com/browse/RHEL-11158

Signed-off-by: Paul Holzinger <[email protected]>
The network list compat API requires us to include all containers with
their ip addresses for the selected networks. Because we have no network
-> container mapping in the db we have to go through all containers
every time. However the old code did it in the most ineffective way
possible, it quered the containers from the db for each individual
network. The of course is extremely expensive. Now the other expensive
call is calling Inspect() on the container each time. Inspect does for
more than we need.

To fix this we fist query containers only once for the API call, then
replace the inspect call with directly accessing the network status.
This will speed things up a lot!
The reported scenario includes 100 containers and 25 networks,
previously it took 1.5s for the API call not it takes 24ms, that is a
more than a 62x improvement. (tested with curl)

[NO NEW TESTS NEEDED] We have no timing tests.

Fixes containers#20035

Signed-off-by: Paul Holzinger <[email protected]>
@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 5, 2023
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Oct 5, 2023
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, vrothberg

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:

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

@rhatdan
Copy link
Member

rhatdan commented Oct 5, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 5, 2023
@openshift-ci openshift-ci bot merged commit c3efd25 into containers:v4.7 Oct 5, 2023
95 checks passed
@Luap99 Luap99 deleted the v4.7 branch October 5, 2023 12:45
@Luap99 Luap99 changed the title [v4.7] Fix env-file regssion + more backports [v4.7] Fix env-file regression + more backports Oct 5, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jan 4, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants