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

Client-side apply has a pathological case with deeply nested identical objects #698

Closed
DirectXMan12 opened this issue Aug 13, 2019 · 5 comments
Assignees
Labels
area/kubectl kind/bug Categorizes issue or PR as related to a bug. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/P2 sig/cli Categorizes an issue or PR as relevant to SIG CLI.

Comments

@DirectXMan12
Copy link

DirectXMan12 commented Aug 13, 2019

Test case

  1. Strip the description fields from https://raw.githubusercontent.com/kubernetes-sigs/kubebuilder/master/docs/book/src/multiversion-tutorial/testdata/project/config/crd/bases/batch.tutorial.kubebuilder.io_cronjobs.yaml (to make it fit in an annotation)
  2. kubectl apply -f the above
  3. kubectl apply -f the above again

Underlying issue

Deeply nested (e.g. validation for podspec) identical issues hit a pathological case in the json-patch library's merge patch generator, which recursively traverses every sub-tree to check if two json trees are identical

Evidence

You can attach an execution trace and use go tool trace to see this for yourself

y41oDeo5pLi

(that's me stopping the attempt after 30s, not it taking 30s total)

runtime trace call patch
diff --git a/cmd/kubectl/kubectl.go b/cmd/kubectl/kubectl.go
index 53934ac2b0..fab992ad95 100644
--- a/cmd/kubectl/kubectl.go
+++ b/cmd/kubectl/kubectl.go
@@ -21,6 +21,9 @@ import (
 	"math/rand"
 	"os"
 	"time"
+	"runtime/trace"
+	"os/signal"
+	"syscall"
 
 	"github.com/spf13/pflag"
 
@@ -33,6 +36,16 @@ import (
 )
 
 func main() {
+	traceOut, err := os.Create("/tmp/kubectl.trace")
+	if err != nil {
+		panic(err)
+	}
+	defer traceOut.Close()
+	if err := trace.Start(traceOut); err != nil {
+		panic(err)
+	}
+	defer trace.Stop()
+		
 	rand.Seed(time.Now().UnixNano())
 
 	command := cmd.NewDefaultKubectlCommand()
@@ -46,7 +59,17 @@ func main() {
 	logs.InitLogs()
 	defer logs.FlushLogs()
 
+	sigCh := make(chan os.Signal)
+	go func() {
+		<-sigCh
+		trace.Stop()
+		traceOut.Close()
+		panic("at the disco!")
+	}()
+	signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM)
+
 	if err := command.Execute(); err != nil {
-		os.Exit(1)
+		//os.Exit(1)
+		return
 	}
 }

Potential fixes

EDIT: See below -- moving around some conditions in the jsonpatch library seems to fix things.

Also, this may be kind-of moot, since we'll be getting server-side apply, which doesn't suffer from this particular issue.

@DirectXMan12
Copy link
Author

/assign @seans3

@seans3
Copy link
Contributor

seans3 commented Aug 13, 2019

/sig cli
/area kubectl
/kind bug
/priority P2

This performance bug should be fixed by server-side apply.

@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. area/kubectl kind/bug Categorizes issue or PR as related to a bug. priority/P2 labels Aug 13, 2019
@DirectXMan12
Copy link
Author

DirectXMan12 commented Aug 15, 2019

double-edit: bug in some of my test code (thanks to @logicalhan for pointing out something was really off here), so ignore what was previously in this comment :-/ That explains one of the bizarre results. Now on to the other...

@DirectXMan12
Copy link
Author

evanphx/json-patch#87 <-- should fix the issue. The dual traversal of maps was causing an exponential time complexity, which really only manifested seriously with lots of nested maps.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubectl kind/bug Categorizes issue or PR as related to a bug. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. priority/P2 sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Projects
None yet
Development

No branches or pull requests

4 participants