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

Change kubectl diff to exclude managedFields by default #111319

Merged
merged 1 commit into from Jul 28, 2022

Conversation

brianpursley
Copy link
Member

@brianpursley brianpursley commented Jul 21, 2022

What type of PR is this?

/kind feature

What this PR does / why we need it:

Changes kubectl diff to exclude managedFields by default.
Adds a new --show-managed-fields flag that allows you to include managed fields in the diff.

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#1242

Special notes for your reviewer:

New Output:

diff -u -N /tmp/LIVE-4192522669/apps.v1.Deployment.nginx-test.foo /tmp/MERGED-3896013886/apps.v1.Deployment.nginx-test.foo
--- /tmp/LIVE-4192522669/apps.v1.Deployment.nginx-test.foo	2022-07-21 11:11:39.107811757 -0400
+++ /tmp/MERGED-3896013886/apps.v1.Deployment.nginx-test.foo	2022-07-21 11:11:39.111811814 -0400
@@ -6,7 +6,7 @@
     kubectl.kubernetes.io/last-applied-configuration: |
       {"apiVersion":"apps/v1","kind":"Deployment","metadata":{"annotations":{},"name":"foo","namespace":"nginx-test"},"spec":{"replicas":1,"selector":{"matchLabels":{"app":"foo"}},"template":{"metadata":{"labels":{"app":"foo"}},"spec":{"containers":[{"command":["sh","-c","echo Container bar is running! \u0026\u0026 sleep 9999999"],"image":"busybox:1.30","name":"bar"}]}}}}
   creationTimestamp: "2022-07-21T13:31:29Z"
-  generation: 1
+  generation: 2
   name: foo
   namespace: nginx-test
   resourceVersion: "38911353"
@@ -14,7 +14,7 @@
 spec:
   progressDeadlineSeconds: 600
   replicas: 1
-  revisionHistoryLimit: 10
+  revisionHistoryLimit: 11
   selector:
     matchLabels:
       app: foo
exit status 1

Old output:

diff -u -N /tmp/LIVE-2971090238/apps.v1.Deployment.nginx-test.foo /tmp/MERGED-4094261447/apps.v1.Deployment.nginx-test.foo
--- /tmp/LIVE-2971090238/apps.v1.Deployment.nginx-test.foo	2022-07-21 11:12:22.416433597 -0400
+++ /tmp/MERGED-4094261447/apps.v1.Deployment.nginx-test.foo	2022-07-21 11:12:22.420433655 -0400
@@ -6,13 +6,47 @@
     kubectl.kubernetes.io/last-applied-configuration: |
       {"apiVersion":"apps/v1","kind":"Deployment","metadata":{"annotations":{},"name":"foo","namespace":"nginx-test"},"spec":{"replicas":1,"selector":{"matchLabels":{"app":"foo"}},"template":{"metadata":{"labels":{"app":"foo"}},"spec":{"containers":[{"command":["sh","-c","echo Container bar is running! \u0026\u0026 sleep 9999999"],"image":"busybox:1.30","name":"bar"}]}}}}
   creationTimestamp: "2022-07-21T13:31:29Z"
-  generation: 1
+  generation: 2
   managedFields:
   - apiVersion: apps/v1
     fieldsType: FieldsV1
     fieldsV1:
       f:metadata:
         f:annotations:
+          f:deployment.kubernetes.io/revision: {}
+      f:status:
+        f:availableReplicas: {}
+        f:conditions:
+          .: {}
+          k:{"type":"Available"}:
+            .: {}
+            f:lastTransitionTime: {}
+            f:lastUpdateTime: {}
+            f:message: {}
+            f:reason: {}
+            f:status: {}
+            f:type: {}
+          k:{"type":"Progressing"}:
+            .: {}
+            f:lastTransitionTime: {}
+            f:lastUpdateTime: {}
+            f:message: {}
+            f:reason: {}
+            f:status: {}
+            f:type: {}
+        f:observedGeneration: {}
+        f:readyReplicas: {}
+        f:replicas: {}
+        f:updatedReplicas: {}
+    manager: kube-controller-manager
+    operation: Update
+    subresource: status
+    time: "2022-07-21T13:31:30Z"
+  - apiVersion: apps/v1
+    fieldsType: FieldsV1
+    fieldsV1:
+      f:metadata:
+        f:annotations:
           .: {}
           f:kubectl.kubernetes.io/last-applied-configuration: {}
       f:spec:
@@ -49,41 +83,7 @@
             f:terminationGracePeriodSeconds: {}
     manager: kubectl-client-side-apply
     operation: Update
-    time: "2022-07-21T13:31:29Z"
-  - apiVersion: apps/v1
-    fieldsType: FieldsV1
-    fieldsV1:
-      f:metadata:
-        f:annotations:
-          f:deployment.kubernetes.io/revision: {}
-      f:status:
-        f:availableReplicas: {}
-        f:conditions:
-          .: {}
-          k:{"type":"Available"}:
-            .: {}
-            f:lastTransitionTime: {}
-            f:lastUpdateTime: {}
-            f:message: {}
-            f:reason: {}
-            f:status: {}
-            f:type: {}
-          k:{"type":"Progressing"}:
-            .: {}
-            f:lastTransitionTime: {}
-            f:lastUpdateTime: {}
-            f:message: {}
-            f:reason: {}
-            f:status: {}
-            f:type: {}
-        f:observedGeneration: {}
-        f:readyReplicas: {}
-        f:replicas: {}
-        f:updatedReplicas: {}
-    manager: kube-controller-manager
-    operation: Update
-    subresource: status
-    time: "2022-07-21T13:31:30Z"
+    time: "2022-07-21T15:12:22Z"
   name: foo
   namespace: nginx-test
   resourceVersion: "38911353"
@@ -91,7 +91,7 @@
 spec:
   progressDeadlineSeconds: 600
   replicas: 1
-  revisionHistoryLimit: 10
+  revisionHistoryLimit: 11
   selector:
     matchLabels:
       app: foo

Does this PR introduce a user-facing change?

kubectl diff changed to ignore managed fields by default, and a new --show-managed-fields flag has been added to allow you to include managed fields in the diff

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 21, 2022
@brianpursley
Copy link
Member Author

/hold for review
/cc @ardaguclu what do you think of this change?

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 21, 2022
@brianpursley
Copy link
Member Author

brianpursley commented Jul 21, 2022

I named the flag --include-managed-fields instead of --show-managed-fields because with diff, it is not guaranteed they will be shown, only that they would be included in the diff decision. I wavered on whether to do this though because the flag is called --show-managed-fields in kubectl get

Flag renamed to --show-managed-fields for consistency with existing flag on other commands

@ardaguclu
Copy link
Member

Thanks for the PR @brianpursley!

In my opinion, we may not need new flag to fix that problem;

  • If this issue kubectl diff show managed-fields kubectl#1242 is a regression caused by referenced PR(which I think it is), there should not be any changes needed because diff already shows expected results in older releases.
  • If issue is not a regression and an inevitable change, there are different use cases we need to consider;
  1. kubectl apply -f foo.yaml -> kubectl diff -f foo.yaml
    Both of them are client side apply and there is no managedfields already. In some cases(like operators take ownership and apply SSA as in our case), there will be managedFields and I think it is a good thing to show these fields to users to warn that ownership changed, etc.
  2. kubectl apply -f --server-side foo.yaml -> kubectl diff -f foo.yaml
  3. kubectl apply -f foo.yaml -> kubectl diff --server-side -f foo.yaml
    AFAIK, we don't support 2. and 3. or maybe I'm wrong(in any case, these are edge cases)
  4. kubectl apply --server-side -f foo.yaml -> kubectl diff --server-side -f foo.yaml
    I think, we should always show managed fields if there is any change in there. Because that will be a good indicator to the user about new fields, possible ownership changes, etc. and if we hide managed fields that will reduce the usability of diff command for SSA.

In my opinion this new include-managed-fields flag should already be always to true in SSA and flag does not matter in client side apply(only usable for the case I mentioned: ownership changes). In addition to that, in the future, someday, SSA will be default and very likely this new flag will be deprecated?.

To sum up, if there is a regression that means apply also is affected and we need to fix that. If not, instead exposing a new flag, we can always show managed fields in SSA and always disable in client side apply maybe(or always show managed fields)?

@brianpursley
Copy link
Member Author

Thanks @ardaguclu. I'll need to look at that other commit you referenced and understand it better.

Something I noticed when I looked at the live and merged files being compared is that the managed fields came back in a different order (reversed) in the live vs the merged, and that was the cause of the big diff in the example output I posted.

So it could also be an ordering problem and maybe it can be fixed if managed fields are always returned in a consistent order so that the diff doesn't pick up the reordering as a change.

@brianpursley brianpursley changed the title Change kubectl diff to exclude managedFields by default WIP: Change kubectl diff to exclude managedFields by default Jul 22, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2022
@ardaguclu
Copy link
Member

ardaguclu commented Jul 22, 2022

@brianpursley you are right, always updating time fields is causing different ordering each time(and diff results). Because fields are sorted according to time field

func sortEncodedManagedFields(encodedManagedFields []metav1.ManagedFieldsEntry) (sortedManagedFields []metav1.ManagedFieldsEntry, err error) {

When I disable sorting manually, diff works as expected. I wonder can we handle that case somehow in diff command.

@ardaguclu
Copy link
Member

Maybe the best option is like you said hiding altogether and only show them when user passes include-managed-fields. Because time field always will be changed and always will be in diff. WDYT?

@brianpursley
Copy link
Member Author

@ardaguclu thanks for looking into it further. Your explanation makes sense to me.

I think it is preferable for diff to exclude managed fields by default. Including them doesn't seem like helpful information to me.

I actually would be OK with always excluding the managed fields and not even having the --include-managed-fields flag, but was hesitant to go that far in case someone wanted to see them. I could easily be convinced the flag isn't necessary though.

I guess we start without the flag and if there is demand for that capability, then add the flag to allow managed fields to be included.

@brianpursley brianpursley changed the title WIP: Change kubectl diff to exclude managedFields by default Change kubectl diff to exclude managedFields by default Jul 22, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2022
@ardaguclu
Copy link
Member

@ardaguclu thanks for looking into it further. Your explanation makes sense to me.

I think it is preferable for diff to exclude managed fields by default. Including them doesn't seem like helpful information to me.

I actually would be OK with always excluding the managed fields and not even having the --include-managed-fields flag, but was hesitant to go that far in case someone wanted to see them. I could easily be convinced the flag isn't necessary though.

I guess we start without the flag and if there is demand for that capability, then add the flag to allow managed fields to be included.

Thanks, maybe like you said we can directly disable managedfields and will expose this flag if there is any demand? but I'm fine both options.

Your PR also looks good to me but I wonder why none of integration tests did not fail and maybe we need one for managedField case?

@KnVerey
Copy link
Contributor

KnVerey commented Jul 26, 2022

I guess we start without the flag and if there is demand for that capability, then add the flag to allow managed fields to be included.

If the output as-is is always useless, then this makes sense to me. I don't see the sense in maintaining a flag for something that isn't helpful. If we do have a flag, I'd expect we'd need to find a way to fix the problem of the diffing essentially not working for managed fields before exposing it.

That said, since this is a behaviour change either way, I suggest we discuss this briefly at tomorrow's meeting.

@apelisse
Copy link
Member

I personally believe that diffing the managed fields is generally useful. The other mentioned error is definitely not a bug. On the other hand, I agree that the re-ordering of managers in this case is not super helpful. I'm curious if there's something we can do about that.

Changes kubectl diff to exclude managedFields by default.
Adds a new --show-managed-fields flag that allows you to
include managed fields in the diff.
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

@soltysh
Copy link
Contributor

soltysh commented Jul 27, 2022

/priority important-longterm
/triage accepted

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 27, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@soltysh
Copy link
Contributor

soltysh commented Jul 27, 2022

/hold cancel
this was discussed during today's sig-cli and the comments above from me are part of that.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2022
@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

1 similar comment
@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubectl diff show managed-fields
7 participants