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

Vendor c/image after https://github.com/containers/image/pull/1816 #17221

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Jan 25, 2023

WIP, this is primarily to expose the code to flakes, and to collect production-environment error values and other data.

Look for BODYREADER DEBUG messages in logs, they are intentionally very loud right now

Does this PR introduce a user-facing change?

Automatically transparently resumes aborted pull connections (in some strictly limited situations)

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 25, 2023
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jan 25, 2023

Cc: @edsantiago

@mtrmac
Copy link
Collaborator Author

mtrmac commented Jan 27, 2023

@edsantiago the test logs (very reasonably) don’t include output from Podman commands ran as part of the tests if the tests succeed. Do you know if there is some pre-existing, reasonably convenient, trick I could use to log things from deep inside Podman code in a way that would then be possible to view on test success? Maybe syslog?

@mtrmac mtrmac force-pushed the eof-range-requests branch from 7d41d9e to 6ad3e48 Compare January 27, 2023 19:48
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jan 27, 2023

dial tcp: lookup cdn03.quay.io: no such host are quite frequent, and not the target of this PR.

I mean, are we supposed to detect and workaround consistently incorrect DNS in redirect targets? It’s probably possible (after we successfully ping quay.io/v2/, any future “DNS not found” errors can be assumed to be infrastructure), but … seriously?

@edsantiago
Copy link
Member

some pre-existing, reasonably convenient, trick I could use to log things from deep inside Podman code in a way that would then be possible to view on test success? Maybe syslog?

From a BATS script, you can echo to fd3, prepending a hash:

    echo "# debug: $output" >&3

From podman itself, you can't, because I take pains to close fd3 before running podman. But journal logs are available in CI, via a series of clicks on the Cirrus page (near the bottom of the accordions).

I mean, are we supposed to detect and workaround consistently incorrect DNS in redirect targets?

I reported the flake to quay on Thursday. Haven't made it past the "works for me" phase yet.

@mtrmac mtrmac force-pushed the eof-range-requests branch 2 times, most recently from e6a3ec5 to a4987ad Compare January 30, 2023 17:50
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jan 30, 2023

One caught failure:

BODYREADER DEBUG: Reading blob body, decision inputs: lastRetryOffset 0, offset 60631823, error &errors.errorString{s:\"unexpected EOF\"}

Good, so the heuristics did match at least in this one case.

@mtrmac mtrmac force-pushed the eof-range-requests branch from a4987ad to ec90b82 Compare January 30, 2023 18:44
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jan 30, 2023

BODYREADER DEBUG: Reading blob body, decision inputs: lastRetryOffset 0, offset 260352, error &errors.errorString{s:"unexpected EOF"}

The current heuristic requires 1 MB, and did not trigger here.

@mtrmac mtrmac force-pushed the eof-range-requests branch 2 times, most recently from c3af877 to c9d4ccc Compare January 30, 2023 20:19
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2023
@mtrmac
Copy link
Collaborator Author

mtrmac commented Feb 7, 2023

Analyzing test runs from Jan 30 (and making a record here so that we have some sort of statistics):

Quite a few tests are broken by the extra logging; I need to tune that down.


BODYREADER DEBUG: Reading blob body, decision inputs: lastRetryOffset 0, offset 2771195, time diff 0 ms, error &errors.errorString{s:\"unexpected EOF\"}

so “immediately” after successfully receiving data (possibly buffered locally? OTOH clearly not a stalled connection), we get unexpected EOF.

On the positive side, reconnecting seems to have worked (the first? recorded instance).


BODYREADER DEBUG: Reading blob body, decision inputs: lastRetryOffset 0, offset 7403776, time diff 0 ms, error &errors.errorString{s:\"unexpected EOF\"}"

also seems to have worked.

@mtrmac mtrmac force-pushed the eof-range-requests branch from c9d4ccc to e73a866 Compare February 7, 2023 00:59
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2023
@mtrmac mtrmac force-pushed the eof-range-requests branch from e73a866 to f70523d Compare February 7, 2023 01:22
@mtrmac
Copy link
Collaborator Author

mtrmac commented Feb 7, 2023

vendor/github.com/openshift/imagebuilder/dockerfile/parser/parser.go:122:12: undefined: system.LCOWSupported

openshift/imagebuilder#238

Note to self: a backport to the 5.24 branch might be in order…

@mtrmac mtrmac changed the title DO NOT MERGE: Vendor https://github.com/containers/image/pull/1816 DO NOT MERGE: Vendor https://github.com/containers/image/pull/1816 / https://github.com/containers/image/pull/1832 Feb 7, 2023
@mtrmac mtrmac force-pushed the eof-range-requests branch from f70523d to 0cf89b5 Compare February 7, 2023 02:15
@mtrmac
Copy link
Collaborator Author

mtrmac commented Feb 7, 2023

Backported, let’s see…

(This PR is currently primarily helpful as a source of frequent enough failures to gather more data. Actually backporting depends on following a process…)

@mtrmac mtrmac mentioned this pull request Feb 7, 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

I am also in favor of a backport. If we backport, today is a good time to get it in before the 4.4.1 release.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Feb 8, 2023

Merging #17342 makes this PR, going back to a c/image 5.24 stable branch, plausible but less unattractive. Something like the closed #17405, rolling c/image even further forward, would make Podman smaller (and on the main branch it’s closer to the long-term direction).

@mtrmac mtrmac force-pushed the eof-range-requests branch from 0cf89b5 to 8ba67ba Compare February 8, 2023 19:39
@mtrmac mtrmac changed the title DO NOT MERGE: Vendor https://github.com/containers/image/pull/1816 / https://github.com/containers/image/pull/1832 Vendor c/image after https://github.com/containers/image/pull/1816 Feb 8, 2023
@mtrmac mtrmac force-pushed the eof-range-requests branch 2 times, most recently from 974b4bc to 1d54f86 Compare February 8, 2023 20:09
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Feb 8, 2023
@mtrmac mtrmac force-pushed the eof-range-requests branch 2 times, most recently from f493928 to 2f81a2f Compare February 8, 2023 20:49
@mtrmac
Copy link
Collaborator Author

mtrmac commented Feb 8, 2023

Due to #17342 being merged, just using a c/image 5.24 branch would be a revert. So, this PR now just rolls c/image forward to include the main-branch merged PR. That brings several other dependency updates.

So, this also:

Ready for review and possible merging.

@mtrmac mtrmac marked this pull request as ready for review February 8, 2023 20:51
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 8, 2023
Also includes unreleased openshift/imagebuilder#246 to work
with the updated docker/docker dependency.

And updates some references to newly deprecated docker/docker symbols.

[NO NEW TESTS NEEDED]

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac mtrmac force-pushed the eof-range-requests branch from 2f81a2f to e308ba0 Compare February 8, 2023 21:37
@edsantiago
Copy link
Member

podman grew by 91528 bytes; max allowed is 51200

Ouch.

Restarted system-test flake, it's the dial tcp: lookup cdn03.quay.io: no such host one.

@vrothberg vrothberg added the bloat_approved Approve a PR in which binary file size grows by over 50k label Feb 9, 2023
go.mod Show resolved Hide resolved
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
@edsantiago PTAL

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, 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

@edsantiago
Copy link
Member

I'm sorry... this is not something I can realistically review, given my ignorance of the underlying modules.

@vrothberg
Copy link
Member

@rhatdan PTAL

@vrothberg
Copy link
Member

@containers/podman-maintainers PTAL

@rhatdan
Copy link
Member

rhatdan commented Feb 9, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2023
@openshift-merge-robot openshift-merge-robot merged commit d5e1e27 into containers:main Feb 9, 2023
@mtrmac mtrmac deleted the eof-range-requests branch February 9, 2023 19:09
@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 Sep 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2023
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. bloat_approved Approve a PR in which binary file size grows by over 50k 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.

5 participants