cisco-open / k8s-objectmatcher

A Kubernetes object matcher library to avoid unnecessary K8s object updates
Apache License 2.0
157 stars 29 forks source link

objectmatcher tries to update status on PodDisruptionBudget #19

Closed whiskeyjimbo closed 4 years ago

whiskeyjimbo commented 4 years ago

Describe the bug using objectmatcher on a v1beta.PodDisruptionBudget causes unnecessary patches because it is trying to update the status field of PDR which is dynamic

"patch": "{"status":{"currentHealthy":0,"disruptionsAllowed":0,"expectedPods":0}}",

"current": "{"apiVersion":"policy/v1beta1","metadata":{"annotations":{"banzaicloud.com/last-applied":"{"spec":{"selector":{"matchLabels":{"app":"webapptestwgroup1"}},"maxUnavailable":1},"status":{"currentHealthy":0,"desiredHealthy":0,"expectedPods":0,"disruptionsAllowed":0},"metadata":{"name":"webapptestwgroup1","namespace":"teamtest-appgrouptest","ownerReferences":[{"name":"webapptestwgroup1","uid":"f778f2ff-4911-11ea-9c67-08002786ed12","controller":true,"apiVersion":"app.grnds.com/v1","kind":"WebApp"}]}}"},"namespace":"teamtest-appgrouptest","selfLink":"/apis/policy/v1beta1/namespaces/teamtest-appgrouptest/poddisruptionbudgets/webapptestwgroup1","resourceVersion":"15164","name":"webapptestwgroup1","generation":1,"creationTimestamp":"2020-02-06T19:26:21Z","uid":"8e265972-4916-11ea-9c67-08002786ed12","ownerReferences":[{"name":"webapptestwgroup1","uid":"f778f2ff-4911-11ea-9c67-08002786ed12","controller":true,"apiVersion":"app.grnds.com/v1","kind":"WebApp"}]},"spec":{"selector":{"matchLabels":{"app":"webapptestwgroup1"}},"maxUnavailable":1},"status":{"desiredHealthy":0,"expectedPods":1,"observedGeneration":1,"disruptionsAllowed":1,"currentHealthy":1},"kind":"PodDisruptionBudget"}",

"modified": "{"metadata":{"ownerReferences":[{"controller":true,"apiVersion":"app.grnds.com/v1","kind":"WebApp","name":"webapptestwgroup1","uid":"f778f2ff-4911-11ea-9c67-08002786ed12"}],"name":"webapptestwgroup1","namespace":"teamtest-appgrouptest"},"spec":{"selector":{"matchLabels":{"app":"webapptestwgroup1"}},"maxUnavailable":1},"status":{"disruptionsAllowed":0,"currentHealthy":0,"desiredHealthy":0,"expectedPods":0}}",

"original": "{"spec":{"selector":{"matchLabels":{"app":"webapptestwgroup1"}},"maxUnavailable":1},"status":{"currentHealthy":0,"desiredHealthy":0,"expectedPods":0,"disruptionsAllowed":0},"metadata":{"name":"webapptestwgroup1","namespace":"teamtest-appgrouptest","ownerReferences":[{"name":"webapptestwgroup1","uid":"f778f2ff-4911-11ea-9c67-08002786ed12","controller":true,"apiVersion":"app.grnds.com/v1","kind":"WebApp"}]}}"}

Steps to reproduce the issue: create a PDB and use objectmatch to update it.

Expected behavior

PDB status should not be updated as it is dynamic based off of pods

Screenshots If applicable, add screenshots to help explain your problem.

Additional context Add any other context about the problem like release numberm version, branch, etc.

pepov commented 4 years ago

I beleive it shouldn't update the status field as long as you don't set anything on it locally. I will add a test case and check this, but until then you can simply update the status field with the values from the existing object to make sure it won't diff.

whiskeyjimbo commented 4 years ago

I haven't tested this with versions > 1.14.9 (amazon EKS...), I know in versions < 1.15 PDB is immutable, not sure if that is contributing to this but a thought.

pepov commented 4 years ago

I've checked this and it works as expected. The status fields are changed on the API server and since those fields are not declared as omitempty it will be checked against the desired state which which will always diff if their values are different. (If they would be omitempty, then you won't see a diff as long as you don't set them in your desired state explicitly)

    // Number of pod disruptions that are currently allowed.
    PodDisruptionsAllowed int32 `json:"disruptionsAllowed" protobuf:"varint,3,opt,name=disruptionsAllowed"`

    // current number of healthy pods
    CurrentHealthy int32 `json:"currentHealthy" protobuf:"varint,4,opt,name=currentHealthy"`

    // minimum desired number of healthy pods
    DesiredHealthy int32 `json:"desiredHealthy" protobuf:"varint,5,opt,name=desiredHealthy"`

    // total number of pods counted by this disruption budget
    ExpectedPods int32 `json:"expectedPods" protobuf:"varint,6,opt,name=expectedPods"`

PDB can be immutable but the issue here is only with the status fields, which are indeed dynamic, so that thing is not contributing.

I still recommend to set the above fields in your desired object's status to the values in the existing object, so that you can avoid the diff.

whiskeyjimbo commented 4 years ago

thanks for looking into this @pepov

pepov commented 4 years ago

@whiskeyjimbo this should help: https://github.com/banzaicloud/k8s-objectmatcher/pull/21