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

[DO NOT MERGE] fix apply issue #26405

Closed
wants to merge 1 commit into from

Conversation

adohe-zz
Copy link

@adohe-zz adohe-zz commented May 27, 2016

Please note this is not a final PR. Just open this for a discussion.

When we use kubectl apply to create resource, we update the annotation, add the last-applied-configuration. But this last-applied-configuration is not the real configuration we used to create the resource, it's just read from the command input. e.g:

Create a deployment with apply -f:

[tony@localhost kubernetes]$ cluster/kubectl.sh apply -f nginx-dpl.yaml
deployment "nginx-deployment" created
[tony@localhost kubernetes]$ cat nginx-dpl.yaml
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  name: nginx-deployment
spec:
  replicas: 1
  template:
    metadata:
      labels:
        app: nginx
        test: abcd
    spec:
      containers:
      - name: nginx
        image: nginx:1.7.9
        ports:
        - containerPort: 80

The last-applied-configuration would be:

{

    "kind":"Deployment",
    "apiVersion":"extensions/v1beta1",
    "metadata":{
        "name":"nginx-deployment",
        "creationTimestamp":null
    },
    "spec":{
        "replicas":1,
        "template":{
            "metadata":{
                "creationTimestamp":null,
                "labels":{
                    "app":"nginx",
                    "test":"abcd"
                }
            },
            "spec":{
                "containers":[
                    {
                        "name":"nginx",
                        "image":"nginx:1.7.9",
                        "ports":[
                            {
                                "containerPort":80
                            }
                        ],
                        "resources":{
                        }
                    }
                ]
            }
        },
        "strategy":{
        }
    },
    "status":{
    }

}

but the real configuration sent to apiserver would be:

{

    "kind":"Deployment",
    "apiVersion":"extensions/v1beta1",
    "metadata":{
        "name":"nginx-deployment",
        "namespace":"default",
        "creationTimestamp":null,
        "labels":{
            "app":"nginx",
            "test":"abcd"
        },
        "annotations":{
            "kubectl.kubernetes.io/last-applied-configuration":"{\"kind\":\"Deployment\",\"apiVersion\":\"extensions/v1beta1\",\"metadata\":{\"name\":\"nginx-deployment\",\"creationTimestamp\":null},\"spec\":{\"replicas\":1,\"template\":{\"metadata\":{\"creationTimestamp\":null,\"labels\":{\"app\":\"nginx\",\"test\":\"abcd\"}},\"spec\":{\"containers\":[{\"name\":\"nginx\",\"image\":\"nginx:1.7.9\",\"ports\":[{\"containerPort\":80}],\"resources\":{}}]}},\"strategy\":{}},\"status\":{}}"
        }
    },
    "spec":{
        "replicas":1,
        "selector":{
            "matchLabels":{
                "app":"nginx",
                "test":"abcd"
            }
        },
        "template":{
            "metadata":{
                "creationTimestamp":null,
                "labels":{
                    "app":"nginx",
                    "test":"abcd"
                }
            },
            "spec":{
                "containers":[
                    {
                        "name":"nginx",
                        "image":"nginx:1.7.9",
                        "ports":[
                            {
                                "containerPort":80,
                                "protocol":"TCP"
                            }
                        ],
                        "resources":{
                        },
                        "terminationMessagePath":"/dev/termination-log",
                        "imagePullPolicy":"IfNotPresent"
                    }
                ],
                "restartPolicy":"Always",
                "terminationGracePeriodSeconds":30,
                "dnsPolicy":"ClusterFirst",
                "securityContext":{
                }
            }
        },
        "strategy":{
            "type":"RollingUpdate",
            "rollingUpdate":{
                "maxUnavailable":1,
                "maxSurge":1
            }
        }
    },
    "status":{
    }

}

The last-applied-configuration annotation has no fields which we set by default. I think this has leads some bugs for apply usage.

@k8s-bot
Copy link

k8s-bot commented May 27, 2016

GCE e2e build/test failed for commit 36ee8f3.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@bgrant0607 bgrant0607 added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label May 27, 2016
@bgrant0607 bgrant0607 assigned bgrant0607 and unassigned lavalamp May 28, 2016
meta := &deployment.ObjectMeta
annots := meta.Annotations
if annots != nil {
original := annots[annotations.LastAppliedConfigAnnotation]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't touch this annotation in the server at the moment.

@bgrant0607
Copy link
Member

Thanks, @adohe, but I'm closing this for now, since it's not the right approach.

@bgrant0607 bgrant0607 closed this May 28, 2016
@adohe-zz adohe-zz deleted the kubectl_apply_label branch May 29, 2016 13:31
@lavalamp
Copy link
Member

lavalamp commented Jun 2, 2016

@adohe Background info: not including the defaults is actually quite important to be able to compute a diff of what the user is intending to change. If you include defaults, it may appear that the user is trying to clear fields that the user actually has no opinion about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants