Skip to content

container_internal: don't ignore error from cleanupNetwork()#1002

Closed
giuseppe wants to merge 1 commit into
containers:masterfrom
giuseppe:cleanup-network-error
Closed

container_internal: don't ignore error from cleanupNetwork()#1002
giuseppe wants to merge 1 commit into
containers:masterfrom
giuseppe:cleanup-network-error

Conversation

@giuseppe
Copy link
Copy Markdown
Member

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

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@mheon
Copy link
Copy Markdown
Member

mheon commented Jun 26, 2018

LGTM once tests green

@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Jun 26, 2018

@rh-atomic-bot r+

@rh-atomic-bot
Copy link
Copy Markdown
Collaborator

📌 Commit f22db58 has been approved by rhatdan

@rh-atomic-bot
Copy link
Copy Markdown
Collaborator

⌛ Testing commit f22db58 with merge 3c83570...

// Clean up network namespace, if present
if err := c.cleanupNetwork(); err != nil {
lastError = nil
lastError = err
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This had been non-fatal since #302, but I don't see any discussion there about why it was non-fatal then.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The only think I can think of was there is an issue where the cleanupNetwork can return an error that states the network does not exist, since it was cleaned up previously but there is no current way for us to check for that error because the oci network code does not use constants.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think were were just being overly cautious there, given that we were already only warning on errors from teardownNetwork() in cleanupNetwork().

@giuseppe
Copy link
Copy Markdown
Member Author

I'll probably need to rebase on top of #1001 before tests are happy

@rh-atomic-bot
Copy link
Copy Markdown
Collaborator

💔 Test failed - status-papr

@mheon
Copy link
Copy Markdown
Member

mheon commented Jun 26, 2018

Test failure are #1001 - retrigger the merge once that's in and it should be good

@TomSweeneyRedHat
Copy link
Copy Markdown
Member

LGTM once merge and test happiness is complete.

@mheon
Copy link
Copy Markdown
Member

mheon commented Jun 27, 2018

Alright, #1001 is merged.
@rh-atomic-bot retry

@rh-atomic-bot
Copy link
Copy Markdown
Collaborator

⌛ Testing commit f22db58 with merge 8ee8f84...

@rh-atomic-bot
Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-papr
Approved by: rhatdan
Pushing 8ee8f84 to master...

@giuseppe giuseppe deleted the cleanup-network-error branch February 26, 2019 10:29
@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 27, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants