use containers/storage/pkg/fileutils/(Exists,Lexists) #22334

giuseppe · 2024-04-10T16:56:12Z

use a faster alternative to os.Stat and os.Lstat to check whether a path is present

Does this PR introduce a user-facing change?

None

packit-as-a-service · 2024-04-10T18:11:56Z

Ephemeral COPR build failed. @containers/packit-build please check.

mheon · 2024-04-10T18:42:40Z

Lot of complaining from CI about os.IsNotExist. Changes LGTM otherwise.

Luap99 · 2024-04-11T14:58:31Z

test/e2e/pull_test.go

@@ -172,7 +172,7 @@ var _ = Describe("Podman pull", func() {
 		session := podmanTest.Podman([]string{"pull", "-q", "--authfile", "/tmp/nonexistent", ALPINE})
 		session.WaitWithDefaultTimeout()
 		Expect(session).To(ExitWithError())
-		Expect(session.ErrorToString()).To(Equal("Error: credential file is not accessible: stat /tmp/nonexistent: no such file or directory"))
+		Expect(session.ErrorToString()).To(MatchRegexp("Error: credential file is not accessible: (faccessat|stat) /tmp/nonexistent: no such file or directory"))


Why can this both? This makes testing the error message much more complicated.

wanted to reduce tests failures in case we end up bisecting c/storage, but probably not worth the cost of dealing with regex.

on Windows we still use stat. Is it something to worry about?

We do not run these outside of linux so not really IMO. I really do not like to add additional complexity to the tests.
For example see #22346 where it would add quite a bit of work to add regex logic.

I still think this is a problem, I don't like having to write regex for all the error messages even when we know we always expect faccessat as we only run the tests on linux but I like to know what @edsantiago thinks about this.

Under what circumstances will tests see stat? I too am strongly against the use of regex. It will disasterify a lot of my upcoming test cleanup.

Never, my understanding stat is only used on native windows platforms. Everwhere else it uses faccess (Linux, Darwin) Probably freebsd as well.

giuseppe · 2024-04-12T08:15:50Z

we need containers/buildah#5469 first

rhatdan · 2024-04-12T10:50:30Z

Is this something we should meantion to RHIVOS as a potentially speedup of podman container start?

Luap99 · 2024-04-12T11:19:39Z

Is this something we should meantion to RHIVOS as a potentially speedup of podman container start?

I doubt that this has a measurable impact over the whole container start duration.

packit-as-a-service · 2024-04-12T12:26:47Z

Cockpit tests failed for commit f86b67a6f1299c4bbf8d42422d1ef1753993fac0. @martinpitt, @jelly, @mvollmer please check.

giuseppe · 2024-04-12T12:43:42Z

setting bloat_approved as the additional size depends on the vendoring

podman 5.1 [1], by way of [2], adds a "StartInterval" element to the `.Config.Healthcheck` API field. This is an API breakage, but accept either form until 5.1 is available on all OSes. [1] containers/podman#22334 [2] containers/image@b6afa8c

martinpitt · 2024-04-15T12:46:55Z

OK, I sent cockpit-project/cockpit-podman#1666 to accept either output format. This is an API break, let's hope it doesn't affect actual users..

podman 5.1 [1], by way of [2], adds a "StartInterval" element to the `.Config.Healthcheck` API field. This is an API breakage, but accept either form until 5.1 is available on all OSes. [1] containers/podman#22334 [2] containers/image@b6afa8c

martinpitt · 2024-04-15T13:21:04Z

That cockpit-podman PR landed, so please retry the cockpit-devdeps tests or force-push. (Or decide that it's an API break and not actually land it, of course :grin)

Luap99 · 2024-04-15T13:32:53Z

OK, I sent cockpit-project/cockpit-podman#1666 to accept either output format. This is an API break, let's hope it doesn't affect actual users..

This is not an API break! This is adding a new field to a structure, if client code cannot handle that then it's parsing is broken. We add new fields all the time. If you want specific fields only then you should query only the fields you needs to check, i.e. --format "{{.Config.Healthcheck.Test}}\n{{.Config.Healthcheck.Test.Interval}} ..."

martinpitt · 2024-04-15T13:40:44Z

Ack -- the PR landed now, so a /packit retest-failed (or next force-push) should go green.

giuseppe · 2024-04-18T10:13:02Z

restarted the tests

The change in healthcheck_run_test.go, depends on the containers/image change: commit b6afa8ca7b324aca8fd5a7b5b206fc05c0c04874 Author: Mikhail Sokolov <msokolov@evolution.com> Date: Fri Mar 15 13:37:44 2024 +0200 Add support for Docker HealthConfig.StartInterval (v25.0.0+) Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

packit-as-a-service · 2024-04-19T08:01:35Z

Ephemeral COPR build failed. @containers/packit-build please check.

Luap99

LGTM

openshift-ci · 2024-04-19T09:33:51Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [Luap99,giuseppe]

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

mheon · 2024-04-19T12:29:32Z

/lgtm

openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 10, 2024

giuseppe added No New Tests Allow PR to proceed without adding regression tests and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 10, 2024

github-actions bot added machine kind/api-change Change to remote API; merits scrutiny labels Apr 10, 2024

giuseppe force-pushed the use-faccessat branch from 182aeaa to c5f2139 Compare April 10, 2024 19:03

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2024

giuseppe force-pushed the use-faccessat branch 5 times, most recently from dc8a4df to 5aad981 Compare April 11, 2024 08:22

giuseppe added the jira label Apr 11, 2024

giuseppe force-pushed the use-faccessat branch 3 times, most recently from 8851e9e to 5a22fc6 Compare April 11, 2024 11:08

Luap99 mentioned this pull request Apr 11, 2024

ExitWithError() - continue tightening #22346

Merged

Luap99 reviewed Apr 11, 2024

View reviewed changes

giuseppe force-pushed the use-faccessat branch 2 times, most recently from 14ae0f3 to cdb5d44 Compare April 12, 2024 08:15

giuseppe force-pushed the use-faccessat branch from cdb5d44 to f86b67a Compare April 12, 2024 11:39

giuseppe added the bloat_approved Approve a PR in which binary file size grows by over 50k label Apr 12, 2024

martinpitt mentioned this pull request Apr 15, 2024

test: Adjust expected HealthCheck API format cockpit-project/cockpit-podman#1666

Merged

giuseppe added 12 commits April 19, 2024 09:52

cmd: use fileutils.(Le|E)xists

90304dd

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

libpod: use fileutils.(Le|E)xists

5656ad4

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

pkg/api: use fileutils.(Le|E)xists

bd00c6f

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

pkg/domain: use fileutils.(Le|E)xists

cbffddc

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

pkg/machine: use fileutils.(Le|E)xists

af24326

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

pkg/rootless: use fileutils.(Le|E)xists

aab06ac

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

pkg/specgen: use fileutils.(Le|E)xists

1991990

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

pkg/trust: use fileutils.(Le|E)xists

0e8d1ad

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

pkg/util: use fileutils.(Le|E)xists

94c6991

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

pkg/bindings: use fileutils.(Le|E)xists

acc78af

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

pkg/systemd: use fileutils.(Le|E)xists

e58972c

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the use-faccessat branch from d4ddd19 to e58972c Compare April 19, 2024 07:52

Luap99 approved these changes Apr 19, 2024

View reviewed changes

openshift-ci bot assigned mheon Apr 19, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2024

openshift-merge-bot bot merged commit db93e2c into containers:main Apr 19, 2024
90 of 92 checks passed

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jul 19, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use containers/storage/pkg/fileutils/(Exists,Lexists) #22334

use containers/storage/pkg/fileutils/(Exists,Lexists) #22334

giuseppe commented Apr 10, 2024

packit-as-a-service bot commented Apr 10, 2024

mheon commented Apr 10, 2024

Luap99 Apr 11, 2024

giuseppe Apr 11, 2024

giuseppe Apr 11, 2024

Luap99 Apr 12, 2024

Luap99 Apr 18, 2024

edsantiago Apr 18, 2024

rhatdan Apr 18, 2024

giuseppe commented Apr 12, 2024

rhatdan commented Apr 12, 2024

Luap99 commented Apr 12, 2024 •

edited

Loading

packit-as-a-service bot commented Apr 12, 2024

giuseppe commented Apr 12, 2024

martinpitt commented Apr 15, 2024

martinpitt commented Apr 15, 2024

Luap99 commented Apr 15, 2024

martinpitt commented Apr 15, 2024

giuseppe commented Apr 18, 2024

packit-as-a-service bot commented Apr 19, 2024

Luap99 left a comment

openshift-ci bot commented Apr 19, 2024

mheon commented Apr 19, 2024

use containers/storage/pkg/fileutils/(Exists,Lexists) #22334

use containers/storage/pkg/fileutils/(Exists,Lexists) #22334

Conversation

giuseppe commented Apr 10, 2024

Does this PR introduce a user-facing change?

packit-as-a-service bot commented Apr 10, 2024

mheon commented Apr 10, 2024

Luap99 Apr 11, 2024

Choose a reason for hiding this comment

giuseppe Apr 11, 2024

Choose a reason for hiding this comment

giuseppe Apr 11, 2024

Choose a reason for hiding this comment

Luap99 Apr 12, 2024

Choose a reason for hiding this comment

Luap99 Apr 18, 2024

Choose a reason for hiding this comment

edsantiago Apr 18, 2024

Choose a reason for hiding this comment

rhatdan Apr 18, 2024

Choose a reason for hiding this comment

giuseppe commented Apr 12, 2024

rhatdan commented Apr 12, 2024

Luap99 commented Apr 12, 2024 • edited Loading

packit-as-a-service bot commented Apr 12, 2024

giuseppe commented Apr 12, 2024

martinpitt commented Apr 15, 2024

martinpitt commented Apr 15, 2024

Luap99 commented Apr 15, 2024

martinpitt commented Apr 15, 2024

giuseppe commented Apr 18, 2024

packit-as-a-service bot commented Apr 19, 2024

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 19, 2024

mheon commented Apr 19, 2024

Luap99 commented Apr 12, 2024 •

edited

Loading