Bug 1351127

Summary: "--record=false" does not work for oc sub-commands such as oc edit
Product: OKD Reporter: Zhang Cheng <chezhang>
Component: ocAssignee: Mike Dame <mdame>
Status: CLOSED CURRENTRELEASE QA Contact: Xingxing Xia <xxia>
Severity: medium Docs Contact:
Priority: unspecified    
Version: 3.xCC: aos-bugs, ffranz, mdame, mmccomas, pweil
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-09-19 13:50:42 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

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.