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

Report StatusConflict on Pod opt partial failures #9188

Merged
merged 1 commit into from
Feb 3, 2021

Conversation

jwhonce
Copy link
Member

@jwhonce jwhonce commented Feb 1, 2021

  • When one or more containers in the Pod reports an error on an operation
    report StatusConflict and report the error(s)

  • jsoniter.RegisterTypeEncoderFunc() used to marshal error as string using error.Error(). This picks up any other places in API where error has been used.

  • Update test framework to allow setting any flag when creating pods

Fixes #8865

Signed-off-by: Jhon Honce jhonce@redhat.com

@jwhonce jwhonce added kind/bug Categorizes issue or PR as related to a bug. Backport to v3.0 labels Feb 1, 2021
@jwhonce jwhonce requested review from mheon and baude February 1, 2021 20:22
@jwhonce jwhonce self-assigned this Feb 1, 2021
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwhonce

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2021
@baude
Copy link
Member

baude commented Feb 1, 2021

code and idea LGTM, legit test failures

@jwhonce jwhonce force-pushed the issues/8865 branch 6 times, most recently from bbcf771 to 03216d0 Compare February 2, 2021 19:23
- When one or more containers in the Pod reports an error on an operation
report StatusConflict and report the error(s)

- jsoniter type encoding used to marshal error as string using error.Error()

- Update test framework to allow setting any flag when creating pods

- Fix test_resize() result check

Fixes containers#8865

Signed-off-by: Jhon Honce <jhonce@redhat.com>
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.

Code LGTM but I don't have enough knowledge judging whether 409 Conflict is valid in all cases.

MDN states: "Conflicts are most likely to occur in response to a PUT request. For example, you may get a 409 response when uploading a file which is older than the one already on the server resulting in a version control conflict."

Does that include transient errors of pod start?

@jwhonce
Copy link
Member Author

jwhonce commented Feb 3, 2021

@vrothberg The handler already used 409, I simply documented that fact and expanded it's usage to all instances of this condition. I believe 409 is more useful than 500 (that was returned in some cases) since a retry may resolve the issue, which is not implied by 500.

@mheon
Copy link
Member

mheon commented Feb 3, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2021
@openshift-merge-robot openshift-merge-robot merged commit 4e1bcf3 into containers:master Feb 3, 2021
@jwhonce jwhonce deleted the issues/8865 branch June 30, 2021 16:10
@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.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. 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
Development

Successfully merging this pull request may close these issues.

Errors are not reported when starting a pod from the REST API
6 participants