Skip to content

Comments

container: Handle failed memdb lookups#33882

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
aaronlehmann:memdb-no-container
Jun 30, 2017
Merged

container: Handle failed memdb lookups#33882
cpuguy83 merged 1 commit intomoby:masterfrom
aaronlehmann:memdb-no-container

Conversation

@aaronlehmann
Copy link

@aaronlehmann aaronlehmann commented Jun 29, 2017

If a container doesn't exist in the memdb, First will return nil, not an error. This should be checked for before using the result.

This fixes the panic in #33863, but does not seem to solve the underlying problem causing test failures.

cc @fabiokung

@aaronlehmann aaronlehmann mentioned this pull request Jun 29, 2017
Closed
thaJeztah
thaJeztah approved these changes Jun 29, 2017
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@aaronlehmann aaronlehmann mentioned this pull request Jun 29, 2017
Closed
@thaJeztah
Copy link
Member

thaJeztah commented Jun 29, 2017 •
edited
Loading

don't merge yet; we're discussing #33883 (comment)

container: Handle failed memdb lookups
c26b0cd 
If a container doesn't exist in the memdb, First will return nil, not an
error. This should be checked for before using the result.

Signed-off-by: Aaron Lehmann <[email protected]>
thaJeztah
thaJeztah approved these changes Jun 29, 2017
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@tiborvass
Copy link
Contributor

tiborvass commented Jun 29, 2017

LGTM

1 similar comment
@fabiokung
Copy link
Contributor

fabiokung commented Jun 29, 2017

LGTM

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Jun 30, 2017

CI failure seems related?

02:18:19.026 =================================== FAILURES ===================================
02:18:19.027 _____________ ServiceTest.test_create_service_with_unicode_secret ______________
02:18:19.027 /docker-py/tests/integration/api_service_test.py:454: in test_create_service_with_unicode_secret
02:18:19.027     container = self.get_service_container(name)
02:18:19.027 /docker-py/tests/integration/api_service_test.py:41: in get_service_container
02:18:19.027     all=include_stopped
02:18:19.027 /docker-py/docker/api/container.py:189: in containers
02:18:19.027     res = self._result(self._get(u, params=params), True)
02:18:19.027 /docker-py/docker/api/client.py:220: in _result
02:18:19.027     self._raise_for_status(response)
02:18:19.027 /docker-py/docker/api/client.py:216: in _raise_for_status
02:18:19.027     raise create_api_error_from_http_exception(e)
02:18:19.028 /docker-py/docker/errors.py:31: in create_api_error_from_http_exception
02:18:19.028     raise cls(e, response=response, explanation=explanation)
02:18:19.028 E   NotFound: 404 Client Error: Not Found ("no such container 54b544bd97eac685cf7d532f6dde24ce5ae072fa7fb52bf873485ab0608c8388")
02:18:19.028 ========= 1 failed, 244 passed, 8 skipped, 3 xfailed in 376.10 seconds =========

@AkihiroSuda AkihiroSuda added rebuild/experimental status/failing-ci Indicates that the PR in its current state fails the test suite labels Jun 30, 2017
@aaronlehmann
Copy link
Author

aaronlehmann commented Jun 30, 2017

It's an existing failure. This PR is the first step in fixing it. #33883 is the second step.

@aaronlehmann aaronlehmann removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 30, 2017
@cpuguy83
Copy link
Member

cpuguy83 commented Jun 30, 2017

This LGTM, I agree this failure is because we're returning an error here which is caught elsewhere but the real issue is synchronization between the snapshot db and the real db.

This just prevents a panic.

Merging.

@cpuguy83 cpuguy83 merged commit 18d874a into moby:master Jun 30, 2017
@aaronlehmann aaronlehmann deleted the memdb-no-container branch July 6, 2017 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees

No one assigned

Labels

status/4-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@aaronlehmann @thaJeztah @tiborvass @fabiokung @AkihiroSuda @cpuguy83 @GordonTheTurtle