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

Incorrect initContainers order when using server-side apply #104641

Closed
cbandy opened this issue Aug 28, 2021 · 20 comments · Fixed by #105983 or #107565
Closed

Incorrect initContainers order when using server-side apply #104641

cbandy opened this issue Aug 28, 2021 · 20 comments · Fixed by #105983 or #107565
Assignees
Labels
kind/bug priority/important-soon sig/api-machinery triage/accepted wg/api-expression
Milestone

Comments

@cbandy
Copy link

@cbandy cbandy commented Aug 28, 2021

What happened:

The order of initContainers is significant, but when using kubectl apply --server-side to change this field, the intended order is not preserved.

What you expected to happen:

I expected the order of initContainers to match my submitted intent.

How to reproduce it (as minimally and precisely as possible):

Files:

==> 00.yaml <==                                                            
apiVersion: v1
kind: Service
metadata:                                                                  
  name: init-container-order
spec:                
  clusterIP: None                                                          
---                                                                        
apiVersion: apps/v1                                                        
kind: StatefulSet
metadata:                                                                  
  name: init-container-order
spec:
  serviceName: init-container-order
  selector:
    matchLabels: { name: init-container-order }
  template:
    metadata:
      labels: { name: init-container-order }
    spec:
      initContainers:
      - { image: busybox, name: original }
      containers:
      - { image: busybox, name: main }

==> 01.yaml <==
apiVersion: v1
kind: Service
metadata:
  name: init-container-order
spec:
  clusterIP: None
---
apiVersion: apps/v1
kind: StatefulSet
metadata:
  name: init-container-order
spec:
  serviceName: init-container-order
  selector:
    matchLabels: { name: init-container-order }
  template:
    metadata:
      labels: { name: init-container-order }
    spec:
      initContainers:
      - { image: busybox, name: first }
      - { image: busybox, name: original }
      - { image: busybox, name: last }
      containers:
      - { image: busybox, name: main }
● diff -u 00.yaml 01.yaml 
--- 00.yaml     2021-08-27 20:39:10.000000000 -0500
+++ 01.yaml     2021-08-27 20:39:08.000000000 -0500
@@ -18,6 +18,8 @@
       labels: { name: init-container-order }
     spec:
       initContainers:
+      - { image: busybox, name: first }
       - { image: busybox, name: original }
+      - { image: busybox, name: last }
       containers:
       - { image: busybox, name: main }

Create the StatefulSet and see it has one init container:

● kubectl apply --server-side -f 00.yaml 
service/init-container-order serverside-applied
statefulset.apps/init-container-order serverside-applied

● kubectl get sts/init-container-order -o jsonpath='{.spec.template.spec.initContainers[*].name}{"\n"}'
original

Add two more init containers, one before and one after the original. See that they are both added to the end:

● kubectl apply --server-side -f 01.yaml 
service/init-container-order serverside-applied
statefulset.apps/init-container-order serverside-applied

● kubectl get sts/init-container-order -o jsonpath='{.spec.template.spec.initContainers[*].name}{"\n"}'
original first last

Anything else we need to know?:

kubectl replace puts them in the intended order:

● kubectl replace -f 01.yaml 
service/init-container-order replaced
statefulset.apps/init-container-order replaced

● kubectl get sts/init-container-order -o jsonpath='{.spec.template.spec.initContainers[*].name}{"\n"}'
first original last

Environment:

  • Kubernetes version (use kubectl version):

    Client Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.0", GitCommit:"c2b5237ccd9c0f1d600d3072634ca66cefdf272f", GitTreeState:"clean", BuildDate:"2021-08-04T17:56:19Z", GoVersion:"go1.16.6", Compiler:"gc", Platform:"darwin/amd64"}
    Server Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.4+k3s1", GitCommit:"3e250fdbab72d88f7e6aae57446023a0567ffc97", GitTreeState:"clean", BuildDate:"2021-08-19T19:09:53Z", GoVersion:"go1.16.6", Compiler:"gc", Platform:"linux/amd64"}
    
  • Install tools: k3d, brew install kubernetes-cli

/sig api-machinery

@cbandy cbandy added the kind/bug label Aug 28, 2021
@k8s-ci-robot k8s-ci-robot added sig/api-machinery needs-triage labels Aug 28, 2021
@liggitt liggitt added priority/critical-urgent wg/api-expression labels Aug 29, 2021
@liggitt
Copy link
Member

@liggitt liggitt commented Aug 29, 2021

@liggitt liggitt added priority/important-soon and removed priority/critical-urgent labels Aug 29, 2021
@lavalamp
Copy link
Member

@lavalamp lavalamp commented Aug 30, 2021

Hm, IIRC we thought about this and the design had an answer for this, so there is probably just a bug somewhere.

@apelisse
Copy link
Member

@apelisse apelisse commented Aug 30, 2021

Here's the code, if I'm reading it correctly.

We first add and merge all items in the order provided by LHS (the current, live object):
https://github.com/kubernetes-sigs/structured-merge-diff/blob/master/typed/merge.go#L202-L231

And then we collect/append all remaining items from RHS (the applied object):
https://github.com/kubernetes-sigs/structured-merge-diff/blob/master/typed/merge.go#L233-L245

So the code works "as intended", the intent seems very wrong though?

@fedebongio
Copy link
Contributor

@fedebongio fedebongio commented Aug 31, 2021

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted and removed needs-triage labels Aug 31, 2021
@fedebongio
Copy link
Contributor

@fedebongio fedebongio commented Aug 31, 2021

/assign @apelisse

@jiahuif
Copy link
Member

@jiahuif jiahuif commented Oct 6, 2021

/assign

@liggitt
Copy link
Member

@liggitt liggitt commented Nov 23, 2021

/reopen

The modified behavior in #105983 still does not match my expectations.

Hoisting from #106191 (comment):

This is what happens now:

  • apply a,b,c as fieldManager x
  • apply c,d,e,f as fieldManager y
  • final order is c,d,e,f,a,b

That discards the order expressed by fieldManager x, which is unexpected. If there's a conflict in order between x andy, honoring the most recently applied order makes sense, but discarding intended order when we don't need to doesn't seem correct to me.

Also, when applying non-overlapping items, this PR changed the behavior, which makes it a hard case for backporting:

  • apply a,b,c as fieldManager x
  • apply d,e,f as fieldManager y
  • final order (with this PR) is d,e,f,a,b,c (in 1.22 and earlier, order was a,b,c,d,e,f... appending the new items seems more natural to me)

@liggitt liggitt added this to the v1.23 milestone Nov 23, 2021
@liggitt liggitt added priority/critical-urgent and removed priority/important-soon labels Nov 23, 2021
@k8s-ci-robot k8s-ci-robot reopened this Nov 23, 2021
@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Nov 23, 2021

@liggitt: Reopened this issue.

In response to this:

/reopen

The modified behavior in #105983 still does not match my expectations.

Hoisting from #106191 (comment):

This is what happens now:

  • apply a,b,c as fieldManager x
  • apply c,d,e,f as fieldManager y
  • final order is c,d,e,f,a,b

That discards the order expressed by fieldManager x, which is unexpected

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@liggitt
Copy link
Member

@liggitt liggitt commented Nov 23, 2021

milestoning and upgrading priority so we ensure the behavior we ship in 1.23 is intentional

@jiahuif
Copy link
Member

@jiahuif jiahuif commented Nov 23, 2021

Thanks for the counter example. Working on that.

@jiahuif
Copy link
Member

@jiahuif jiahuif commented Nov 23, 2021

How about this. We add each item in LHS until hitting an item in RHS (not inclusive), then we do our current algorithm with the rest

a,b,c MEREG c,d,e,f

  1. each in LHS not in RHS: a b
  2. prefer RHS order a b | c d e f

Another example

a b c g f MERGE c d e f,
1 add a b first: a b
2 then prefer RHS, add c d e f
3 then add remaining g

result = a b c d e f g

@liggitt
Copy link
Member

@liggitt liggitt commented Nov 23, 2021

I think reading from HEAD of LHS until we hit a shared item is a good start, but I think this should be iterative so that we preserve both LHS and RHS in non-conflicting cases, and there needs to be a pre-merge step where we identify items in LHS we can't honor the LHS order of because of RHS.

I was thinking something along these lines: https://gist.github.com/liggitt/1e5f40f92235d4440724af24f803f17e

@liggitt
Copy link
Member

@liggitt liggitt commented Nov 24, 2021

if the updated merge is not available ~today, we should revert #105983 to make sure we don't ship 1.23 with unfinalized changes

@dims
Copy link
Member

@dims dims commented Nov 24, 2021

@liggitt
Copy link
Member

@liggitt liggitt commented Nov 24, 2021

cc @endocrimes @SergeyKanzhelev @ehashman

fyi, this is apimachinery, not node (it's an apply issue, not an initContainers issue)

@liggitt
Copy link
Member

@liggitt liggitt commented Nov 24, 2021

if the updated merge is not available ~today, we should revert #105983 to make sure we don't ship 1.23 with unfinalized changes

Actually, since we're in rc territory already, I opened the revert in #106660 and picked to release-1.23 in #106661. Once the updated merge is ready, we can consider backports.

@jiahuif
Copy link
Member

@jiahuif jiahuif commented Nov 24, 2021

Since we already have two new candidates for the new algorithm, both of which covers all known cases, this issue should settle soon-ish once we choose one of them.

@liggitt
Copy link
Member

@liggitt liggitt commented Nov 24, 2021

that's fine, but at this point, this seems like a candidate for v1.23.x via normal backport

@liggitt liggitt added priority/important-soon and removed priority/critical-urgent labels Nov 24, 2021
@liggitt liggitt removed this from the v1.23 milestone Nov 24, 2021
@liggitt liggitt added this to the v1.24 milestone Nov 24, 2021
@liggitt
Copy link
Member

@liggitt liggitt commented Nov 24, 2021

revert merged to master, will get picked to 1.23. reset priority and milestoned as 1.24, but a fix here could be eligible for backport to release branches, including 1.23.x

@jiahuif
Copy link
Member

@jiahuif jiahuif commented Nov 24, 2021

Got it. Will have the new implementation ready soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug priority/important-soon sig/api-machinery triage/accepted wg/api-expression
Projects
None yet
8 participants