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.

Sign up for GitHub

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

Jump to bottom

Vendor c/common pasta branch for testing #21563

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Vendor c/common pasta branch for testing #21563

merged 2 commits into from
Feb 29, 2024

Conversation

mheon
Copy link
Member

@mheon mheon commented Feb 8, 2024

Test swapping the default from Slirp4netns to Pasta

Does this PR introduce a user-facing change?

Connectivity for rootless containers is now provided by Pasta by default, instead of slirp4netns

@openshift-ci openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 8, 2024
@packit-as-a-service Packit-as-a-Service
Copy link

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

@packit-as-a-service Packit-as-a-Service
Copy link

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

@mheon
Copy link
Member Author

mheon commented Feb 8, 2024

Looks like I broke remote, but everything else seems solid

@packit-as-a-service Packit-as-a-Service
Copy link

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

@mheon
Copy link
Member Author

mheon commented Feb 8, 2024

I'm actually not sure how this APIv2 test passed in the first place. We don't populate the NetworkSettings.Networks field for slirp4netns either per my testing.

@Luap99
Copy link
Member

Luap99 commented Feb 9, 2024

see 2165170

@Luap99
Copy link
Member

Luap99 commented Feb 9, 2024

Seems like the test failures are no mostly things that expect slirp4netns default so we need to change them, the timeouts in the sys tests are however very concerning because they do not seem to hang at a specific point and just are much slower.

@mheon mheon force-pushed the test_pasta_default branch 2 times, most recently from a31c715 to ae3891b Compare February 9, 2024 14:27
@packit-as-a-service Packit-as-a-Service
Copy link

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

@packit-as-a-service Packit-as-a-Service
Copy link

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

@packit-as-a-service Packit-as-a-Service
Copy link

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

@mheon mheon mentioned this pull request Feb 9, 2024
Merged
@packit-as-a-service Packit-as-a-Service
Copy link

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

@mheon
Copy link
Member Author

mheon commented Feb 9, 2024

APIv2 failure is legitimate, getContainerNetIO needs a rewrite to handle Pasta properly.

@mheon
Copy link
Member Author

mheon commented Feb 9, 2024

System tests are hard-stuck on [+2306s] ok 647 [700] podman kube play healthcheck should wait initialDelaySeconds before updating status (healthy) (or, more accurately, whatever comes after this)

@mheon
Copy link
Member Author

mheon commented Feb 9, 2024

I see nothing in there that Pasta ought to have broken...

@Luap99
Copy link
Member

Luap99 commented Feb 9, 2024

Odd, can you reproduce locally?

@mheon
Copy link
Member Author

mheon commented Feb 9, 2024

The 700-play file passes locally without issue (and don't seem to take unduly long either)

@Luap99
Copy link
Member

Luap99 commented Feb 9, 2024

If nothing helps there is this re-run with terminal access feature in the cirrus web ui. It should allow you to look around while the tests are running.

@packit-as-a-service Packit-as-a-Service
Copy link

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

@mheon
Copy link
Member Author

mheon commented Feb 29, 2024

Yep that 100% broke us.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 29, 2024
@packit-as-a-service Packit-as-a-Service
Copy link

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

@mheon
Fix events by fully adding the new PullError event
03f6589 
Signed-off-by: Matt Heon <mheon@redhat.com>
@mheon
Copy link
Member Author

mheon commented Feb 29, 2024

I think I fixed it.

@rhatdan
Copy link
Member

rhatdan commented Feb 29, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 29, 2024
@mheon
Copy link
Member Author

mheon commented Feb 29, 2024 

# Error: initializing source docker://nixery.dev/shell:latest: pinging container registry nixery.dev: Get "https://nixery.dev/v2/": net/http: TLS handshake timeout

@mheon
Copy link
Member Author

mheon commented Feb 29, 2024

Registry issues, as usual

@packit-as-a-service Packit-as-a-Service
Copy link

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

@mheon
Copy link
Member Author

mheon commented Feb 29, 2024

Two flakes restarted.
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 29, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit b736c46 into containers:main Feb 29, 2024
90 of 94 checks passed
This was referenced Mar 1, 2024
Merged
Closed
Closed
@martinpitt
Copy link
Contributor

This caused a major regression, just filed as issue #21896

Luap99
Luap99 reviewed Mar 1, 2024
View reviewed changes
libpod/events/config.go
@@ -41,6 +41,8 @@ type Event struct {
Type Type
// Health status of the current container
HealthStatus string `json:"health_status,omitempty"`
// Error code for certain events involving errors.
Error error `json:"error,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

This cannot work with the file logger as you cannot unmarshal into an error interface so it must be changed to a string. And in general I am really not a fan of merging such changes (in an unrelated PR) without any tests nor chance for actual review.
I know we just wanted pasta to get in but that could have just gone it normal way without causing all the extra troubles by simply not merging unverified/untested changes in c/common without thinking how they effect podman.

Also the whole thing is not plumbed in at all, you never set this option from the libimage type so the error is lost and also not plugged into podman the cli nor API which means it will not show any error messages, users only see pull-error $IMAGE right now.

I am aware that your goal was to just get test passing but I write this because this "feature" if far from done and needs tests.

@Luap99 Luap99 mentioned this pull request Mar 1, 2024
Closed
@rhatdan
Copy link
Member

rhatdan commented Mar 1, 2024

@Luap99 or @mheon can you work this, I don't currently have time to make this work.

@Luap99
Copy link
Member

Luap99 commented Mar 1, 2024

Not today, I could do it next week. Nothing is really broken with that it is just the the error message is not being passed through to the consumer. I don't think it is a big deal if it missed rc4 but should definitely be there for the final release.

@rhatdan
Copy link
Member

rhatdan commented Mar 1, 2024

Works for me.

martinpitt added a commit to martinpitt/cockpit-podman that referenced this pull request Mar 1, 2024
@martinpitt
test: HACK: Add fake default route for podman-next scenario
26f6d69 
The introduction of pasta [1] regressed user containers if there is no
default route [2]. While that is being sorted out, add a fake interface
with a default route for our offline tests, to unbreak upstream podman
PRs testing.

[1] containers/podman#21563
[2] containers/podman#21896
@martinpitt martinpitt mentioned this pull request Mar 1, 2024
Merged
martinpitt added a commit to martinpitt/cockpit-podman that referenced this pull request Mar 1, 2024
@martinpitt
test: HACK: Add fake default route for podman-next scenario
80050e8 
The introduction of pasta [1] regressed user containers if there is no
default route [2]. While that is being sorted out, add a fake interface
with a default route for our offline tests, to unbreak upstream podman
PRs testing.

Fixes cockpit-project#1595

[1] containers/podman#21563
[2] containers/podman#21896
martinpitt added a commit to cockpit-project/cockpit-podman that referenced this pull request Mar 1, 2024
@martinpitt
test: HACK: Add fake default route for podman-next scenario
cecb2cc 
The introduction of pasta [1] regressed user containers if there is no
default route [2]. While that is being sorted out, add a fake interface
with a default route for our offline tests, to unbreak upstream podman
PRs testing.

Fixes #1595

[1] containers/podman#21563
[2] containers/podman#21896
@stale-locking-app 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 May 31, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@edsantiago edsantiago edsantiago left review comments

@Luap99 Luap99 Luap99 approved these changes

@sbrivio-rh sbrivio-rh sbrivio-rh left review comments

Assignees

@rhatdan rhatdan

@Luap99 Luap99

Labels
5.0 approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. pasta pasta(1) bugs or features release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@mheon @Luap99 @martinpitt @sbrivio-rh @edsantiago @rhatdan @dgibson @openshift-merge-robot