Bug 1311786 - oc patch reports success with incorrect patch information
Summary: oc patch reports success with incorrect patch information
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: OpenShift Container Platform
Classification: Red Hat
Component: oc
Version: unspecified
Hardware: Unspecified
OS: Unspecified
medium
low
Target Milestone: ---
: ---
Assignee: Jan Chaloupka
QA Contact: zhou ying
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-02-25 00:45 UTC by Chris Ryan
Modified: 2022-08-29 10:13 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2022-08-25 20:19:15 UTC
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Bugzilla 1267454 0 medium CLOSED It's better to show additional prompt when 'oc patch' non-existing fields of resource 2021-02-22 00:41:40 UTC

Internal Links: 1267454

Description Chris Ryan 2016-02-25 00:45:18 UTC
Description of problem:
'oc patch' will report a successful patch and exit code 0, even if the patch is not performed due to incorrect semantical information. 

Version-Release number of selected component (if applicable):
oc v1.1.3-232-gf39a5a8
kubernetes v1.2.0-alpha.7-703-gbc4550d

How reproducible:
Always

Steps to Reproduce:
1. Create a deploymentconfig in a project:
oc create -f https://raw.githubusercontent.com/openshift-qe/v3-testfiles/master/deployment/deployment1.json

2. Patch the deploy pod with a false value (i.e. replicas in a pod)
oc patch pod hooks-1-deploy -p \{\"spec\":\{\"replicas\":\ 7\}\} 

3. Check the result

Actual results:
oc patch reports a success ""hooks-1-deploy" patched", and returns a 0 exit code

Expected results:
oc patch should fail, or at least give a warning with a non-zero exit code

Additional info:

Comment 1 Juan Vallejo 2016-10-12 20:12:41 UTC
I don't think the server response that is received after an object is patched includes any errors from the server's attempt at applying the patch; if any errors were returned by the server, they would be returned here: https://github.com/openshift/origin/blob/master/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/patch.go#L174, therefore it would not be possible at the moment to have the client print a warning for this, at least without making a server-side change.

Comment 2 Juan Vallejo 2016-12-02 19:50:44 UTC
Related PR: https://github.com/openshift/origin/pull/12120

Comment 3 Juan Vallejo 2016-12-15 21:09:19 UTC
Upstream PR[1] has merged.
Origin PR will merge after the rebase with kube 1.6

[1] https://github.com/kubernetes/kubernetes/pull/38538

Comment 4 XiaochuanWang 2017-01-18 07:28:55 UTC
Still reproduced by original steps when pod "hooks-1-deploy" exist.

1. # oc create -f https://raw.githubusercontent.com/openshift-qe/v3-testfiles/master/deployment/deployment1.json -n xiaocwan-t
deploymentconfig "hooks" created
2. # oc patch pod hooks-1-deploy -p \{\"spec\":\{\"replicas\":\ 7\}\} -n xiaocwan-t
"hooks-1-deploy" patched

Please noticed that this is same with bug #1267454 and both reproduced on latest origin: oc v1.5.0-alpha.1+eaa36ed-311

Since the upstream PR (kubernetes/kubernetes/pull/38538) is merged, but openshift PR (openshift/origin/pull/12120) is still open, will wait for merge and test it again.

Comment 5 Xingxing Xia 2017-07-05 10:12:35 UTC
Same as bug 1466671
Now output information is improved
See https://bugzilla.redhat.com/show_bug.cgi?id=1466671#c5 verified in v3.6.133.
(QE is checking many Modified bugs that if they're verifiable. Because fixed, moving to Verified. If have extra concern, pls correct me, thx)

Comment 6 Chris Ryan 2017-07-05 13:54:43 UTC
Even though the human-readable text is now appropriate, the exit code still appears to be 0; for an error like this, we should have a non-zero exit code:

$ oc patch pod hooks-1-deploy -p \{\"spec\":\{\"replicas\":\ 7\}\} 
pod "hooks-1-deploy" not patched

$ echo $?
0


This is seen on:
$ oc version
oc v3.6.0-alpha.2+3c221d5
kubernetes v1.6.1+5115d708d7
features: Basic-Auth GSSAPI Kerberos SPNEGO

Fedora 25 x64
4.10.17-200.fc25.x86_64

Comment 8 Juan Vallejo 2017-09-13 21:59:09 UTC
Upstream PR: https://github.com/kubernetes/kubernetes/pull/52450

Comment 9 Juan Vallejo 2017-09-25 14:02:26 UTC
Upstream PR has merged [1]. Patch will be available in next rebase.

1. https://github.com/kubernetes/kubernetes/pull/52450

Comment 10 Xingxing Xia 2018-07-27 09:56:05 UTC
FYI, a customer's concern https://bugzilla.redhat.com/show_bug.cgi?id=1609179 for the fix.

Comment 11 Xingxing Xia 2018-07-30 06:40:58 UTC
This bug's PR goes to oc for long time. Checked 3.9.30, it is fixed per comment 7.


Note You need to log in before you can comment on or make changes to this bug.