Bug 1351127 - "--record=false" does not work for oc sub-commands such as oc edit
Summary: "--record=false" does not work for oc sub-commands such as oc edit
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: OKD
Classification: Red Hat
Component: oc
Version: 3.x
Hardware: Unspecified
OS: Unspecified
unspecified
medium
Target Milestone: ---
: ---
Assignee: Mike Dame
QA Contact: Xingxing Xia
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-06-29 10:05 UTC by Zhang Cheng
Modified: 2017-08-23 09:08 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-09-19 13:50:42 UTC
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)

Description Zhang Cheng 2016-06-29 10:05:54 UTC
Description of problem: Record current kubectl command in the resource annotation while '--record=false'


Version-Release number of selected component (if applicable):
[root@ip-172-18-2-17 ~]# openshift version
openshift v3.2.1.3-1-gcbaf600
kubernetes v1.2.0-36-g4a3f9c5
etcd 2.2.5
[root@ip-172-18-2-17 ~]# docker version
Client:
 Version:         1.10.3
 API version:     1.22
 Package version: docker-common-1.10.3-44.el7.x86_64
 Go version:      go1.4.2
 Git commit:      7ffc8ee-unsupported
 Built:           Fri Jun 17 15:27:21 2016
 OS/Arch:         linux/amd64

Server:
 Version:         1.10.3
 API version:     1.22
 Package version: docker-common-1.10.3-44.el7.x86_64
 Go version:      go1.4.2
 Git commit:      7ffc8ee-unsupported
 Built:           Fri Jun 17 15:27:21 2016
 OS/Arch:         linux/amd64


How reproducible:
Always


Steps to Reproduce:
1. Create a pod with: oc create -f ./pod.yaml
# cat pod.yaml
apiVersion: v1
kind: Pod
metadata:
  name: hello-openshift
  labels:
    app: hello-openshift
spec:
  containers:
  - name: hello-openshift
    image: openshift/hello-openshift
    ports:
    - containerPort: 80

2. Edit and rename pod.metadata.labels with '--record=true'
# oc edit pod hello-openshift --record=true
pod "hello-openshift" edited

3. Check annotations info in pod yaml file:
# oc get pod hello-openshift -o yaml | grep change-cause
    kubernetes.io/change-cause: oc edit pod hello-openshift --record=true

4. Edit and rename pod.metadata.labels again with '--record=false'
# oc edit pod hello-openshift --record=false
pod "hello-openshift" edited

5. Check annotations info in pod yaml file again


Actual results: oc edit command with '--record=false' was recorded in annotation
5. Check annotations info in pod yaml file again:
# oc get pod hello-openshift -o yaml | grep change-cause
    kubernetes.io/change-cause: oc edit pod hello-openshift --record=false


Expected results: The command with '--record=false' should not be recorded in annotation in Step 5. Also, I'm not sure if the original info should be removed when '--record=false'


addition info:
Other commands that have '--record' option also have similar problem, such as: oc scale, oc annotate, oc apply, and so on.

Comment 1 Mike Dame 2016-06-29 17:54:05 UTC
I believe this is a bug with kubernetes and how it records these changes. This issue only shows up when "--record=false" is passed to a command after a previous command used "--record=true".

The code I'm referring to is here: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/util/helpers.go#L484-L495

Those functions decide if the change should be recorded, by checking if either the "record" flag is set, OR there is a ChangeCauseAnnotation set on the object. The problem arises if the flag had previously been set to "true", which records a ChangeCauseAnnotation and thus further prevents this logic check from ever failing.

This could be fixed by changing the ShouldRecord function to only check for the "record" flag, but I don't know enough about the ChangeCauseAnnotations to know the importance of having an OR check in that spot.

Comment 2 Mike Dame 2016-06-29 18:34:38 UTC
Other option for a proposed fix: https://github.com/damemi/kubernetes/commit/7b842faaf48e5839206f401972068a3ac8b71501

Comment 3 Fabiano Franz 2016-07-01 14:39:16 UTC
Mike, are you working on a fix? If so, please take assignment of the bug, tks!

Comment 4 Mike Dame 2016-07-01 15:17:12 UTC
Yes, I submitted a PR here: https://github.com/kubernetes/kubernetes/pull/28234

Comment 5 Fabiano Franz 2016-07-01 15:38:55 UTC
Ok. Once the fix is accepted upstream you have to create a separate PR to OpenShift Origin following the UPSTREAM commit convention:

https://github.com/openshift/origin/blob/master/HACKING.md#cherry-picking-an-upstream-commit-into-origin

Comment 6 Paul Weil 2016-08-17 13:18:47 UTC
looks like this PR was accepted upstream, do we have the pick downstream?

Comment 7 Mike Dame 2016-08-17 15:08:36 UTC
PR created: https://github.com/openshift/origin/pull/10486

Comment 8 Zhang Cheng 2016-08-24 03:21:53 UTC
Verified and Passed in openshift origin:
$ oc version
oc v1.3.0-alpha.3+5983e63-dirty
kubernetes v1.3.0+507d3a7
features: Basic-Auth

Server https://ec2-54-152-68-91.compute-1.amazonaws.com:8443
openshift v1.3.0-alpha.3+5983e63-dirty
kubernetes v1.3.0+507d3a7

Test coverage: Test "--record" function with commands "oc scale", "oc edit" and "oc apply".

Such as for "oc scale", TCs focus on:

1. Run `oc scale` with "--record=flase", the changes can not be recorded
2. Run `oc scale` with "--record=true", the changes can be recorded
3. Firstly, run `oc scale` with "--record=true", and then run `oc scale` with "--record=flase", only first changes can be recorded
4. Firstly, run `oc scale` with "--record=true", and then run `oc scale` with "--record=true", all changes can be recorded and the first changes was overrided
5. Run `oc scale` without "--record", the changes can not be recorded
6. Firstly, run `oc scale` with "--record=false", and then run `oc scale` without "--record", all changes cannot be recorded
7. Firstly, run `oc scale` with "--record=true", and then run `oc scale` without "--record", all changes can be recorded and the first changes was overrided.
8. Firstly, run `oc scale` with "--record=true", and then run `oc scale` without "--record", and then run `oc scale` without "--record", all changes can be recorded and the first two changes was overrided.
9. Firstly, run `oc scale` with "--record=true", and then run `oc scale` without "--record", and then run `oc scale` with "--record=flase", only the first and second changes can be recorded, and only left the second changes in currently.


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