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

Migrate: handle NotFound via resource matching and during conflicts #18280

Conversation

enj
Copy link
Contributor

@enj enj commented Jan 25, 2018

This change fixes two issues with the migrate command:

  1. It updates isNotFoundForInfo to compare the error's kind to the info's resource as well as its kind. The server error's use the resource value in places where the kind should be used. We simply accept both now.
  2. It updates DefaultRetriable to not fail on conflict errors if they were caused by the deletion of the given resource.

The following shell based tests were used to stress the edge cases that this change fixes. The migrate command will readily fail in these tests without the fixes from this change.

In one shell we continuously create service accounts with random names as fast as possible. The resource type is not relevant.

N=8
while true; do
    ((i=i%N)); ((i++==0)) && wait
    oc create serviceaccount $((1 + RANDOM % 10000000)) &
done

In a second shell we try to continuously delete all service accounts:

while true; do
    oc delete sa --all
done

In a third shell we try to continuously migrate service accounts:

while true; do
    oc adm migrate storage --include='serviceaccount' --confirm
done

Bug 1537751

Signed-off-by: Monis Khan [email protected]

This change fixes two issues with the migrate command:

1. It updates isNotFoundForInfo to compare the error's kind to the
info's resource as well as its kind.  The server error's use the
resource value in places where the kind should be used.  We simply
accept both now.
2. It updates DefaultRetriable to not fail on conflict errors if
they were caused by the deletion of the given resource.

The following shell based tests were used to stress the edge cases
that this change fixes.  The migrate command will readily fail in
these tests without the fixes from this change.

In one shell we continuously create service accounts with random
names as fast as possible.  The resource type is not relevant.

N=8
while true; do
    ((i=i%N)); ((i++==0)) && wait
    oc create serviceaccount $((1 + RANDOM % 10000000)) &
done

In a second shell we try to continuously delete all service
accounts:

while true; do
    oc delete sa --all
done

In a third shell we try to continuously migrate service accounts:

while true; do
    oc adm migrate storage --include='serviceaccount' --confirm
done

Bug 1537751

Signed-off-by: Monis Khan <[email protected]>
@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 25, 2018
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 25, 2018
@enj enj added kind/bug Categorizes issue or PR as related to a bug. kind/delivery-blocker labels Jan 25, 2018
@enj
Copy link
Contributor Author

enj commented Jan 25, 2018

/assign @simo5

This is blocking upgrades from 3.7 to 3.9.

@simo5
Copy link
Contributor

simo5 commented Jan 25, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, simo5

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@enj
Copy link
Contributor Author

enj commented Jan 25, 2018

cc @jupierce @sdodson

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 16754, 18280).

@openshift-merge-robot openshift-merge-robot merged commit 82fe749 into openshift:master Jan 25, 2018
@enj
Copy link
Contributor Author

enj commented Jan 25, 2018

/cherrypick release-3.8

@openshift-cherrypick-robot

@enj: new pull request created: #18286

In response to this:

/cherrypick release-3.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@enj
Copy link
Contributor Author

enj commented Jan 25, 2018

/cherrypick release-3.7

@openshift-cherrypick-robot

@enj: new pull request created: #18287

In response to this:

/cherrypick release-3.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-merge-robot added a commit that referenced this pull request Jan 26, 2018
…-18280-to-release-3.7

Automatic merge from submit-queue.

[release-3.7] Migrate: handle NotFound via resource matching and during conflicts

This is an automated cherry-pick of #18280

/assign enj
openshift-merge-robot added a commit that referenced this pull request Jan 27, 2018
…-18280-to-release-3.8

Automatic merge from submit-queue.

[release-3.8] Migrate: handle NotFound via resource matching and during conflicts

This is an automated cherry-pick of #18280

/assign enj
sjenning pushed a commit to sjenning/origin that referenced this pull request Jul 30, 2018
…18220-upstream-ose-enterprise-3.8

Automated cherry pick of openshift#18220 openshift#18280 on enterprise-3.8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/delivery-blocker lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants