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

Fix #6618 #9109

Merged
merged 1 commit into from
Jan 27, 2021
Merged

Fix #6618 #9109

merged 1 commit into from
Jan 27, 2021

Conversation

mheon
Copy link
Member

@mheon mheon commented Jan 26, 2021

Obsoletes #9092

Ensure that the networks list is populated, even when only the default network is in use.

@mheon mheon mentioned this pull request Jan 26, 2021
Closed
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2021
vrothberg
vrothberg reviewed Jan 26, 2021
View reviewed changes
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

@mheon
Copy link
Member Author

mheon commented Jan 26, 2021

E2E tests are timing out. It looks like the tests we added aren't the issue, too - they're running fine and not marked as slow.

It looks like the timeout is 30 minutes here, not 90? @cevich PTAL

@baude baude mentioned this pull request Jan 26, 2021
Closed
@TomSweeneyRedHat
Copy link
Member

LGTM
assuming happy tests

@cevich
Copy link
Member

cevich commented Jan 26, 2021

It looks like the timeout is 30 minutes here, not 90

FWIW: There are multiple timeouts in-play:

  • Ginkgo has one internally, for communication between multiple nodes. I don't know how to see or set this.
  • There's an overall ginkgo timeout set by the GINKGOTIMEOUT Makefile variable.
  • Cirrus-CI has a hard-coded/unchangeable 2-hour timeout per task
  • We set several Cirrus-CI timeouts for tasks via the timeout_in option in cirrus.yml.

For the last item, I don't see any of them set lower than 60min.

@mheon
Copy link
Member Author

mheon commented Jan 26, 2021

Failures are, unfortunately, consistent. Damn.

@baude @mheon
Add default net info in container inspect
02ec529 
when inspecting a container that is only connected to the default
network, we should populate the default network in the container inspect
information.

Fixes: containers#6618

Signed-off-by: baude <[email protected]>

MH: Small fixes, added another test

Signed-off-by: Matthew Heon <[email protected]>
@mheon
Copy link
Member Author

mheon commented Jan 26, 2021

Ah, it was a legitimate issue with the PR, that was for some reason hanging the tests instead of causing them to fail?

@cevich
Copy link
Member

cevich commented Jan 26, 2021

Sorry, my eyeballs aren't nearly as well-trained as yours when it comes to the tests and podman internals. I only remember hitting this node-hang before in different contexts, a long while ago. In every case I remember, it was the tests actually hanging because of a legitimate (i.e. bug) reason. Though arguably, a test-bug could be caused by some CI-environmental goof, so I wouldn't completely rule that out, it just seems less-likely since master is passing (I assume). It's also possible the bug was always there, but timing-dependent, and the PR's changes simply tickle it in the wrong way.

@mheon
Copy link
Member Author

mheon commented Jan 27, 2021

OK, green now. @containers/podman-maintainers PTAL

saschagrunert
saschagrunert approved these changes Jan 27, 2021
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert 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-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, saschagrunert

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 [mheon,saschagrunert]

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

vrothberg
vrothberg reviewed Jan 27, 2021
View reviewed changes
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-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2021
@openshift-merge-robot openshift-merge-robot merged commit 2102e26 into containers:master Jan 27, 2021
@TerraTech TerraTech mentioned this pull request Jan 31, 2021
Closed
@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg left review comments

@saschagrunert saschagrunert saschagrunert approved these changes

Assignees

@vrothberg vrothberg

Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mheon @TomSweeneyRedHat @cevich @openshift-ci-robot @saschagrunert @vrothberg @openshift-merge-robot @baude