kubectl apply for DaemonSet and Deployment have different behavior with RollingUpdate

[onorua]

Kubernetes version (use kubectl version):

kubectl version
Client Version: version.Info{Major:"1", Minor:"6+", GitVersion:"v1.6.0-beta.3+coreos.0", GitCommit:"7013c674183a8b4ee3b3dd846f108e3a5575f4af", GitTreeState:"clean", BuildDate:"2017-03-13T19:57:10Z", GoVersion:"go1.7.5", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"6+", GitVersion:"v1.6.0-beta.3+coreos.0", GitCommit:"7013c674183a8b4ee3b3dd846f108e3a5575f4af", GitTreeState:"clean", BuildDate:"2017-03-13T19:57:10Z", GoVersion:"go1.7.5", Compiler:"gc", Platform:"linux/amd64"}

Environment:

• Cloud provider or hardware configuration: DigitalOcean
• OS (e.g. from /etc/os-release):

cat /etc/os-release
NAME="Ubuntu"
VERSION="16.04.2 LTS (Xenial Xerus)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 16.04.2 LTS"
VERSION_ID="16.04"
HOME_URL="http://www.ubuntu.com/"
SUPPORT_URL="http://help.ubuntu.com/"
BUG_REPORT_URL="http://bugs.launchpad.net/ubuntu/"
VERSION_CODENAME=xenial
UBUNTU_CODENAME=xenial

• Kernel (e.g. uname -a):

uname -a
Linux k8s-master-fra10-01 4.4.0-64-generic #85-Ubuntu SMP Mon Feb 20 11:50:30 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

• Install tools:

cargo

• Others:

What happened:
when I do:

kubectl apply -f deployment.yaml

it will run rolling update ONLY if there is a change. In case I do:

kubectl apply -f daemonset.yaml

Each time I run it - it will execute rolling update, which is misleading.

What you expected to happen:
I would like to have consistent behavior for Deployment and DaemonSet

How to reproduce it (as minimally and precisely as possible):
Create any DaemonSet with following:

piVersion: extensions/v1beta1
kind: DaemonSet
metadata:
  name: bind
spec:
  updateStrategy:
    type: RollingUpdate
    rollingUpdate:
      maxUnavailable: 10%
  template:
    metadata:
      labels:
        service: bind
        commit: e311f982eb769835ca75d942bd2fd0d9890776fb
    spec:
      nodeSelector:
          nodetype: "hss"
      affinity:
        podAntiAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
            - labelSelector:
                matchExpressions:
                - key: "service"
                  operator: "In"
                  values: ["bind"]
              topologyKey: "kubernetes.io/hostname"
              namespaces: []
      containers:
      - env:
        - name: SERVICE_53_IGNORE
          value: '"true"'
        - name: TZ
          value: US/Pacific
        image: bind
        name: named
        resources:
          limits:
            memory: "150Mi"
          requests:
            memory: "40Mi"
      restartPolicy: Always

[0xmichalis]

@kubernetes/sig-apps-bugs

[lukaszo]

Hmm, it shouldn't happen. TemplateGeneration is updated only when something changed. I will debug it.

[janetkuo]

I can reproduce this, and the DaemonSet templateGeneration is updated after every apply, even when nothing is changed.

In func (daemonSetStrategy) PrepareForUpdate, old/new DaemonSet template are seen as different with reflect.DeepEqual. I tried to change this part to use apiequality.Semantic.DeepEqual instead (needed to update code in validation as well), and the DaemonSet templates were then seen as the same, and thus the issue was gone.

We used reflect.DeepEqual everywhere in the controllers. Is apiequality.Semantic.DeepEqual the right way to compare stuffs? @smarterclayton @wojtek-t

[lukaszo]

It's very strange. I tried to use pretty.Diff to print the differences but it returns empty slice.
apiequality.Semantic.DeepEqual uses forked version of reflect.DeepEqual and it's little different from version in 1.7.
@janetkuo will you prepare the patch?

[janetkuo]

The key of the reported DaemonSet config is its affinity.podAntiAffinity.requiredDuringSchedulingIgnoredDuringExecution.namespaces.

      affinity:
        podAntiAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
            - labelSelector:
                matchExpressions:
                - key: "service"
                  operator: "In"
                  values: ["bind"]
              topologyKey: "kubernetes.io/hostname"
              namespaces: []  # <----

Note that in the config file, namespaces: [] (empty list); however, in the live DaemonSet object (created from the config file), namespaces: null (null list). That's why reflect.DeepEqual thinks they're different.

If we use namespaces: null in the config file instead, templateGeneration won't change.

From the namespaces field description, empty list and null list are different.

	// namespaces specifies which namespaces the labelSelector applies to (matches against);
	// nil list means "this pod's namespace," empty list means "all namespaces"
	// The json tag here is not "omitempty" since we need to distinguish nil and empty.
	// See https://golang.org/pkg/encoding/json/#Marshal for more details.
	Namespaces []string `json:"namespaces" protobuf:"bytes,2,rep,name=namespaces"`

Feel it's api machinery related @kubernetes/sig-api-machinery-misc

[liggitt]

protobuf storage isn't able to store [], so the object has that field set to null when read back from storage. that is causing problems for that field in particular (where null and [] have different meanings)

see #43203 (comment)

[liggitt]

cc @kubernetes/sig-scheduling-bugs for behavior of namespaces field

[janetkuo]

@lukaszo so it's not a DaemonSet bug

[liggitt]

Well, still not sure whether Semantic.DeepEqual should be used or not. I'm still concerned with the behavior of the controller for other fields that can be populated with non-null empty lists by users, but serialize back out to null (or set omitempty)

[lukaszo]

deployments are using apiequality.Semantic.DeepEqual https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/deployment/util/deployment_util.go#L731 Maybe we should also switch?

[janetkuo]

If empty lists should be treated differently from null list, we should replace apiequality.Semantic.DeepEqual with reflect.DeepEqual in controller code.

[liggitt]

that distinction is going away in all the fields I know of... I'm not super happy with Semantic.DeepEqual masking the difference between those two things, but that's probably the right thing for you to be using

[liggitt]

Special meaning of [] was removed in #43271

I think a switch to Semantic.DeepEqual is still needed here

[janetkuo]

I think a switch to Semantic.DeepEqual is still needed here

Filed #43337