UPSTREAM: 58991: restore original object on apply err

[juanvallejo]

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1539529

When oc apply is used with the --force flag, it will attempt to patch an existing object A up to 5 times before it deletes the existing object and re-creates it with a new object B originally used to patch A.

If object B has invalid syntax, or a change preventing it from passing validation, the new object will fail to be created and oc apply will simply delete the original object without informing the user that their object no longer exists, and a new one was not created.

This patch restores the original object in the event that an error occurs while attempting to outright replace it with a new one.

Hoping to use this thread to at least discuss the nature of oc apply --force, and better define its behavior when it encounters a non-conflict error.

cc @ironcladlou

[ironcladlou]

I want to create an upstream issue to discuss. We shouldn't delete the resource only to recreate it if we can establish in advance the resource is invalid and has no hope of being persisted. This also brings in to question the semantics of --force in the absence of any merge conflicts (but in the presence of validation errors).

[ironcladlou]

kubectl actually seems to handle this correctly, so we need to investigate why there's a behavioral disparity in oc.

[ironcladlou]

I was able to break kubectl with a server-side-validated replicationcontroller in the same way reported for deploymentconfigs, so we do still have an upstream issue to discuss.

[ironcladlou]

The apply command's --validate flag seems to be false by default in oc but true by default in kubectl, which accounts for the disparity when trying to apply a resource with --force which can be validated client side...

[ironcladlou]

kubernetes/kubernetes#58984

[juanvallejo]

Explanation of what is happening in the bugzilla (two different edge-cases are being hit): https://bugzilla.redhat.com/show_bug.cgi?id=1539529#c3

[juanvallejo]

Upstream PR: kubernetes/kubernetes#58991

[deads2k]

/hold

@juanvallejo just to help sort out what's being picked and not yet lgtm'd/merged upstream, can you apply holds on the picks?

[juanvallejo]

@deads2k @ironcladlou Upstream PR merged kubernetes/kubernetes#58991

This one is now up to date

[juanvallejo]

/retest

[mfojtik]

/retest

[deads2k]

/hold cancel
already merged upstream
/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, juanvallejo

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

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

• vendor/k8s.io/kubernetes/pkg/OWNERS [deads2k]

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

[juanvallejo]

/retest

[juanvallejo]

/test cmd

[juanvallejo]

/test extended_conformance_install

[openshift-merge-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[openshift-merge-robot]

Automatic merge from submit-queue.