Fix backward compatibility issue caused by promoting initcontainers f… #33055

dchen1107 · 2016-09-19T23:39:35Z

#31026 moves init-container feature from alpha to beta, but only took care the backward compatibility for pod specification, not deal with status. For status, it simply moved from pods.beta.kubernetes.io/init-container-statuses to

pods.beta.kubernetes.io/init-container-statuses instead of introducing one more pods.beta.kubernetes.io/init-container-statuses. This breaks when the cluster is running with 1.4, but the user is still running with kubectl 1.3.x.

Fixed #32711

This change is

…rom alpha to beta.

dchen1107 · 2016-09-19T23:41:02Z

cc/ @erictune @lavalamp

dchen1107 · 2016-09-19T23:48:52Z

Without this patch, if the user runs the init containers tests in 1.3 branch (including kubectl 1.3...): The dumped pod object in .json looks like

...
      Annotations:map      [  
         string
      ]      string      {  
         "pod.beta.kubernetes.io/init-container-statuses":"[{\"name\":\"init1\",\"state\":{\"terminated\":{\"exitCode\":0,\"reason\":\"Completed\",\"startedAt\":\"2016-09-19T20:55:15Z\",\"finishedAt\":\"2016-09-19T20:55:15Z\",\"containerID\":\"docker://559cdeb9f5cdce79f5c90c30d589940d4cb28cbcba0ee32b1dd1ef01219c4d06\"}},\"lastState\":{},\"ready\":true,\"restartCount\":0,\"image\":\"gcr.io/google_containers/busybox:1.24\",\"imageID\":\"docker://sha256:0cb40641836c461bc97c793971d84d758371ed682042457523e4ae701efe7ec9\",\"containerID\":\"docker://559cdeb9f5cdce79f5c90c30d589940d4cb28cbcba0ee32b1dd1ef01219c4d06\"},{\"name\":\"init2\",\"state\":{\"terminated\":{\"exitCode\":0,\"reason\":\"Completed\",\"startedAt\":\"2016-09-19T20:55:16Z\",\"finishedAt\":\"2016-09-19T20:55:16Z\",\"containerID\":\"docker://8fef7c4ddff181624353a7a4b43d7a552aa8e9a9c945bd19c28eb2080c60fd3e\"}},\"lastState\":{},\"ready\":true,\"restartCount\":0,\"image\":\"gcr.io/google_containers/busybox:1.24\",\"imageID\":\"docker://sha256:0cb40641836c461bc97c793971d84d758371ed682042457523e4ae701efe7ec9\",\"containerID\":\"docker://8fef7c4ddff181624353a7a4b43d7a552aa8e9a9c945bd19c28eb2080c60fd3e\"}]",
         "pod.beta.kubernetes.io/init-containers":"[{\"name\":\"init1\",\"image\":\"gcr.io/google_containers/busybox:1.24\",\"command\":[\"/bin/true\"],\"resources\":{},\"volumeMounts\":[{\"name\":\"default-token-x49pm\",\"readOnly\":true,\"mountPath\":\"/var/run/secrets/kubernetes.io/serviceaccount\"}],\"terminationMessagePath\":\"/dev/termination-log\",\"imagePullPolicy\":\"IfNotPresent\"},{\"name\":\"init2\",\"image\":\"gcr.io/google_containers/busybox:1.24\",\"command\":[\"/bin/true\"],\"resources\":{},\"volumeMounts\":[{\"name\":\"default-token-x49pm\",\"readOnly\":true,\"mountPath\":\"/var/run/secrets/kubernetes.io/serviceaccount\"}],\"terminationMessagePath\":\"/dev/termination-log\",\"imagePullPolicy\":\"IfNotPresent\"}]"
      },
...
   Status:api.PodStatus   {  
      Phase:"Succeeded",
...
      InitContainerStatuses:[  
                 <==========================================EMPTY container status here!!!!
      ]      
      ContainerStatuses:[  
...

Running the same tests with this patch, the dumped pod object in .json looks like

...
      Annotations:map      [  
         string
      ]      string      {  
         "pod.beta.kubernetes.io/init-container-statuses":"[{\"name\":\"init1\",\"state\":{\"terminated\":{\"exitCode\":0,\"reason\":\"Completed\",\"startedAt\":\"2016-09-19T23:25:18Z\",\"finishedAt\":\"2016-09-19T23:25:18Z\",\"containerID\":\"docker://7eea9349d1116209dbb731eed4e6be79137b44667aabd2047b4c191af87e4f14\"}},\"lastState\":{},\"ready\":true,\"restartCount\":0,\"image\":\"gcr.io/google_containers/busybox:1.24\",\"imageID\":\"docker://sha256:0cb40641836c461bc97c793971d84d758371ed682042457523e4ae701efe7ec9\",\"containerID\":\"docker://7eea9349d1116209dbb731eed4e6be79137b44667aabd2047b4c191af87e4f14\"},{\"name\":\"init2\",\"state\":{\"terminated\":{\"exitCode\":0,\"reason\":\"Completed\",\"startedAt\":\"2016-09-19T23:25:20Z\",\"finishedAt\":\"2016-09-19T23:25:20Z\",\"containerID\":\"docker://5b6023e6d676d259d502228e856ead065c75a5526f2cd74cde3f60f3d1503b42\"}},\"lastState\":{},\"ready\":true,\"restartCount\":0,\"image\":\"gcr.io/google_containers/busybox:1.24\",\"imageID\":\"docker://sha256:0cb40641836c461bc97c793971d84d758371ed682042457523e4ae701efe7ec9\",\"containerID\":\"docker://5b6023e6d676d259d502228e856ead065c75a5526f2cd74cde3f60f3d1503b42\"}]",
         "pod.beta.kubernetes.io/init-containers":"[{\"name\":\"init1\",\"image\":\"gcr.io/google_containers/busybox:1.24\",\"command\":[\"/bin/true\"],\"resources\":{},\"volumeMounts\":[{\"name\":\"default-token-pfq0w\",\"readOnly\":true,\"mountPath\":\"/var/run/secrets/kubernetes.io/serviceaccount\"}],\"terminationMessagePath\":\"/dev/termination-log\",\"imagePullPolicy\":\"IfNotPresent\"},{\"name\":\"init2\",\"image\":\"gcr.io/google_containers/busybox:1.24\",\"command\":[\"/bin/true\"],\"resources\":{},\"volumeMounts\":[{\"name\":\"default-token-pfq0w\",\"readOnly\":true,\"mountPath\":\"/var/run/secrets/kubernetes.io/serviceaccount\"}],\"terminationMessagePath\":\"/dev/termination-log\",\"imagePullPolicy\":\"IfNotPresent\"}]"
      },
...
      InitContainerStatuses:[  

      ]      api.ContainerStatus      {  
         api.ContainerStatus         {  
            Name:"init1",                                                 <============================= Init Container Status here!!!!
            State:api.ContainerState            {  
               Waiting:(*api.ContainerStateWaiting)(nil),
               Running:(*api.ContainerStateRunning)(nil),
               Terminated:(*api.ContainerStateTerminated)(0xc82032ba40)
            },
            LastTerminationState:api.ContainerState            {  
               Waiting:(*api.ContainerStateWaiting)(nil),
               Running:(*api.ContainerStateRunning)(nil),
               Terminated:(*api.ContainerStateTerminated)(nil)
            },
            Ready:true,
            RestartCount:0,
            Image:"gcr.io/google_containers/busybox:1.24",
            ImageID:"docker://sha256:0cb40641836c461bc97c793971d84d758371ed682042457523e4ae701efe7ec9",
            ContainerID:"docker://7eea9349d1116209dbb731eed4e6be79137b44667aabd2047b4c191af87e4f14"
         },
         api.ContainerStatus         {  
            Name:"init2",
            State:api.ContainerState            {  
               Waiting:(*api.ContainerStateWaiting)(nil),
               Running:(*api.ContainerStateRunning)(nil),
               Terminated:(*api.ContainerStateTerminated)(0xc82032bb90)
            },
            LastTerminationState:api.ContainerState            {  
               Waiting:(*api.ContainerStateWaiting)(nil),
               Running:(*api.ContainerStateRunning)(nil),
               Terminated:(*api.ContainerStateTerminated)(nil)
            },
            Ready:true,
            RestartCount:0,
            Image:"gcr.io/google_containers/busybox:1.24",
            ImageID:"docker://sha256:0cb40641836c461bc97c793971d84d758371ed682042457523e4ae701efe7ec9",
            ContainerID:"docker://5b6023e6d676d259d502228e856ead065c75a5526f2cd74cde3f60f3d1503b42"
         }
      },
...

lavalamp · 2016-09-19T23:51:50Z

LGTM-- unit test would be good so we don't break it in the future but that can be a followup.

dchen1107 · 2016-09-19T23:53:01Z

cc/ @pwittrock

pwittrock · 2016-09-19T23:55:36Z

@eparis I am cherry picking this because it is causing an upgrade failure. Both the problem and solution are well understood.

k8s-bot · 2016-09-20T00:21:33Z

GCE e2e build/test passed for commit 4c4623e.

pwittrock · 2016-09-20T05:57:14Z

@k8s-bot unit test this issue: #IGNORE

pwittrock · 2016-09-20T06:24:52Z

@k8s-bot unit test this issue: #IGNORE

omg this is annoying

…055-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of #33055 Cherry pick of #33055 on release-1.4.

pwittrock · 2016-09-20T14:51:25Z

@k8s-bot unit test this issue: #32770

dchen1107 · 2016-09-20T21:58:40Z

@k8s-bot unit test this issue: #32770

k8s-github-robot · 2016-09-20T23:30:07Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-09-21T00:09:20Z

Automatic merge from submit-queue

k8s-cherrypick-bot · 2016-09-21T00:11:28Z

Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

smarterclayton · 2016-09-21T21:20:58Z

I'll put this in the evidence queue for the amount of work annotations cause not actually being worth the work. Also, the whole point of alpha -> beta is to drop old fields, doing this takes us in the wrong direction (supporting twice as many fields). It would have been better to do nothing than have to jump through these hoops.

dchen1107 · 2016-09-21T21:40:11Z

@smarterclayton I agreed with you absolutely on this case. We need to redefine / redesign how to support alpha / beta feature and related promotion workflow.

smarterclayton · 2016-09-21T22:37:12Z

I have a todo to write up a simple proposal on the issue open for this so we can try this for 1.5 on a few fields.

erictune · 2016-09-22T00:43:18Z

@dchen1107 thanks for fixing this.

@smarterclayton I was having similar thoughts as I was writing the PR that this is fixing-up.

mikedanese · 2016-09-22T22:55:06Z

pkg/api/v1/conversion.go

@@ -627,6 +638,11 @@ func Convert_v1_Pod_To_api_Pod(in *Pod, out *api.Pod, s conversion.Scope) error
 		// back to the caller.
 		in.Spec.InitContainers = values
 	}
+	// If there is a beta annotation, copy to alpha key.
+	// See commit log for PR #31026 for why we do this.
+	if valueBeta, okBeta := in.Annotations[PodInitContainerStatusesBetaAnnotationKey]; okBeta {


We do this a couple lins above (line 623) are we doing this twice intentionally?

mikedanese · 2016-09-22T22:57:44Z

If alpha annotation is synced to beta and beta annotations always trumps alpha annotation, then a v1.3 kubelet who only ever uses the alpha annotation will only be able to update the container-statuses once and never again. The apiserver will sync alpha to beta on the initial write then kubelet's subsequent writes of the alpha annotation will be overrwirtten by the beta annotations which contains the kubelet's initial write.

mikedanese · 2016-09-22T22:59:59Z

And if we did this the other way around (alpha trumping beta), v1.4 kubelets couldn't update init container status for pods that had been updated by v1.3 kubelets. The correct way to fix this is to decide the priority of the annotations by looking at the version of the kubelet which I don't think is possible inside conversions.

mikedanese · 2016-09-22T23:00:27Z

@maisem figured this out.

pwittrock · 2016-09-22T23:09:42Z

Thanks Mike. Does that explain why this only impacts GKE and not GCE upgrade tests?

mikedanese · 2016-09-22T23:16:27Z

@pwittrock this also effects GCE.

erictune · 2016-09-22T23:24:09Z

That is probably why I didn't do this the first time.

Can we just change the test to not expect a particular status, please.

On Thu, Sep 22, 2016 at 4:17 PM, Mike Danese notifications@github.com
wrote:

@pwittrock /~https://github.com/pwittrock this also effects GCE.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#33055 (comment),
or mute the thread
/~https://github.com/notifications/unsubscribe-auth/AHuudl0wROqT7h2GqqkhM3f-y0amif0xks5qswx8gaJpZM4KBFFc
.

dchen1107 · 2016-09-23T00:02:55Z

Wait a minute... before we jumped a conclusion:

without this patch, if master and node running with 1.4, but kubectl is running with 1.3, the test failed
with this patch, if master is on 1.4, but node and kubectl is still on 1.3, the test failed.

Is this right?

mikedanese · 2016-09-23T00:55:00Z

I don't understand the comment. v1.3 kubelet only knows about the alpha annotations thus only the first write is commited because there is not beta annotations. All subsequent writes do not commit because the beta annotation (contaning the initial data) always overwrites the alpha annotation according to this conversion logic. Does my explanation make sense?

erictune · 2016-09-23T15:23:04Z

Yes that makes sense.

erictune · 2016-09-23T18:15:28Z

Status has times in it. What if we let the one with the newest time win?

If the annotation with the newest timestamp is synced to the older one, then a v1.3 kubelet who only ever uses the alpha annotation but has the newest timestamp, it will be able to update the container-statuses every time a timestamp advances. The apiserver will sync newest to oldest on the initial write then kubelet's subsequent writes of the alpha annotation will overrwrite by the beta annotations which contains the kubelet's initial write with older time.

erictune · 2016-09-23T20:41:28Z

And if we did this the other way around (alpha trumping beta), v1.4 kubelets couldn't update init container status for pods that had been updated by v1.3 kubelets. The correct way to fix this is to decide the priority of the annotations by looking at the version of the kubelet which I don't think is possible inside conversions.

I don't think this is true. The Kubelet puts the status into the internal struct. Then, the internal representation gets converted to the v1 struct with 1.3 conversion code in kubelet. That code copies the internal status into just the alpha field. So, when it goes over the wire, it may have conflicting alpha and beta annotations. But, when it reaches the 1.4 apiserver, the json gets round-tripped before going to storage. So, the alpha key trumps the beta key, and the right status is stored.

I am going to test this.

mikedanese · 2016-09-23T21:05:00Z

If the annotation with the newest timestamp is synced to the older one, then a v1.3 kubelet who only ever uses the alpha annotation but has the newest timestamp, it will be able to update the container-statuses every time a timestamp advances. The apiserver will sync newest to oldest on the initial write then kubelet's subsequent writes of the alpha annotation will overrwrite by the beta annotations which contains the kubelet's initial write with older time.

This sounds like it would fix the issue. It's annoying to put this type of logic in conversions though.

smarterclayton · 2016-09-23T21:49:20Z

Also seems fragile.

We're seeing this because we're trying to be backwards compatible on a
field we said we would not be backwards compatible on - it seems more
appropriate to be defensive in the kubelet and require kubelets be updated
first if you don't want disruption of an alpha feature.

Note that we'll have this same problem going from beta to final, which
seems like an indicator that the beta should be a field, not an annotation.

On Sep 23, 2016, at 5:05 PM, Mike Danese notifications@github.com wrote:

If the annotation with the newest timestamp is synced to the older one,
then a v1.3 kubelet who only ever uses the alpha annotation but has the
newest timestamp, it will be able to update the container-statuses every
time a timestamp advances. The apiserver will sync newest to oldest on the
initial write then kubelet's subsequent writes of the alpha annotation will
overrwrite by the beta annotations which contains the kubelet's initial
write with older time.

This sounds like it would fix the issue. It's annoying to put this type of
logic in conversions though.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#33055 (comment),
or mute the thread
/~https://github.com/notifications/unsubscribe-auth/ABG_p8ymKDFVsSAnLEub-xssyHtwzXhXks5qtD8LgaJpZM4KBFFc
.

erictune · 2016-09-24T00:59:36Z

We decided we are not going to fix for 1.4.0. I will update the relnotes to day that you can't have init container status when your kubelets are 1.3.x and your apiservers are 1.4.0. Alpha features may break.

If there is moaning, I will try one of the two fixes I mentioned above for a patch release.

…ick-of-#33055-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of kubernetes#33055 Cherry pick of kubernetes#33055 on release-1.4.

Fix backward compatibility issue caused by promoting initcontainers f…

4c4623e

…rom alpha to beta.

googlebot added the cla: yes label Sep 19, 2016

dchen1107 added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 19, 2016

dchen1107 added this to the v1.4 milestone Sep 19, 2016

dchen1107 added the cherrypick-candidate label Sep 19, 2016

k8s-github-robot assigned erictune Sep 19, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Sep 19, 2016

lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2016

lavalamp assigned lavalamp and unassigned erictune Sep 19, 2016

pwittrock added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Sep 19, 2016

pwittrock added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Sep 19, 2016

pwittrock mentioned this pull request Sep 20, 2016

Automated cherry pick of #33055 #33068

Merged

k8s-github-robot pushed a commit that referenced this pull request Sep 20, 2016

Merge pull request #33068 from pwittrock/automated-cherry-pick-of-#33…

dbd5ab2

…055-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of #33055 Cherry pick of #33055 on release-1.4.

pwittrock mentioned this pull request Sep 20, 2016

TestWatch {etcd} #32770

Closed

k8s-github-robot merged commit aa5372c into kubernetes:master Sep 21, 2016

k8s-cherrypick-bot removed the cherrypick-candidate label Sep 21, 2016

mikedanese reviewed Sep 22, 2016

View reviewed changes

erictune mentioned this pull request Sep 23, 2016

fix init container test which is broken in the version skew suites #33340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix backward compatibility issue caused by promoting initcontainers f… #33055

Fix backward compatibility issue caused by promoting initcontainers f… #33055

dchen1107 commented Sep 19, 2016 •

edited by k8s-oncall

Loading

dchen1107 commented Sep 19, 2016

dchen1107 commented Sep 19, 2016

lavalamp commented Sep 19, 2016

dchen1107 commented Sep 19, 2016

pwittrock commented Sep 19, 2016

k8s-bot commented Sep 20, 2016

pwittrock commented Sep 20, 2016

pwittrock commented Sep 20, 2016

pwittrock commented Sep 20, 2016

dchen1107 commented Sep 20, 2016

k8s-github-robot commented Sep 20, 2016

k8s-github-robot commented Sep 21, 2016

k8s-cherrypick-bot commented Sep 21, 2016

smarterclayton commented Sep 21, 2016

dchen1107 commented Sep 21, 2016

smarterclayton commented Sep 21, 2016

erictune commented Sep 22, 2016

mikedanese Sep 22, 2016

mikedanese commented Sep 22, 2016 •

edited

Loading

mikedanese commented Sep 22, 2016

mikedanese commented Sep 22, 2016

pwittrock commented Sep 22, 2016

mikedanese commented Sep 22, 2016

erictune commented Sep 22, 2016

dchen1107 commented Sep 23, 2016

mikedanese commented Sep 23, 2016 •

edited

Loading

erictune commented Sep 23, 2016

erictune commented Sep 23, 2016

erictune commented Sep 23, 2016

mikedanese commented Sep 23, 2016

smarterclayton commented Sep 23, 2016

erictune commented Sep 24, 2016

Fix backward compatibility issue caused by promoting initcontainers f… #33055

Fix backward compatibility issue caused by promoting initcontainers f… #33055

Conversation

dchen1107 commented Sep 19, 2016 • edited by k8s-oncall Loading

dchen1107 commented Sep 19, 2016

dchen1107 commented Sep 19, 2016

lavalamp commented Sep 19, 2016

dchen1107 commented Sep 19, 2016

pwittrock commented Sep 19, 2016

k8s-bot commented Sep 20, 2016

pwittrock commented Sep 20, 2016

pwittrock commented Sep 20, 2016

pwittrock commented Sep 20, 2016

dchen1107 commented Sep 20, 2016

k8s-github-robot commented Sep 20, 2016

k8s-github-robot commented Sep 21, 2016

k8s-cherrypick-bot commented Sep 21, 2016

smarterclayton commented Sep 21, 2016

dchen1107 commented Sep 21, 2016

smarterclayton commented Sep 21, 2016

erictune commented Sep 22, 2016

mikedanese Sep 22, 2016

Choose a reason for hiding this comment

mikedanese commented Sep 22, 2016 • edited Loading

mikedanese commented Sep 22, 2016

mikedanese commented Sep 22, 2016

pwittrock commented Sep 22, 2016

mikedanese commented Sep 22, 2016

erictune commented Sep 22, 2016

dchen1107 commented Sep 23, 2016

mikedanese commented Sep 23, 2016 • edited Loading

erictune commented Sep 23, 2016

erictune commented Sep 23, 2016

erictune commented Sep 23, 2016

mikedanese commented Sep 23, 2016

smarterclayton commented Sep 23, 2016

erictune commented Sep 24, 2016

dchen1107 commented Sep 19, 2016 •

edited by k8s-oncall

Loading

mikedanese commented Sep 22, 2016 •

edited

Loading

mikedanese commented Sep 23, 2016 •

edited

Loading