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

fix: make jx prompt fast again #2414

Closed
wants to merge 8 commits into from
Closed

Conversation

jglick
Copy link
Contributor

@jglick jglick commented Dec 5, 2018

Just like in #2225 I noticed that jx prompt was taking much too long (a second or more, when it should generally take <100ms), apparently contacting the API server. git bisect (starting from the last known fix—#2234) pinpointed #2283 as the first culprit, but #2104 seems to have compounded the issue. CC @pmuir who happened to write both of these.

I am not exactly sure what a solution would look like—#2283 could perhaps be amended like in #2234, but #2104 looks like a more fundamental design problem—but this PR at least shows how to reproduce the issue: build and run jx prompt with any of the three suppressed code blocks restoredthe changes to, say, upgrade_apps.go reverted and you will see the stack trace, like

github.com/jenkins-x/jx/pkg/jx/cmd.createKubeConfig(0x27aef6d)
	…/pkg/jx/cmd/factory.go:446 +0x39
github.com/jenkins-x/jx/pkg/jx/cmd.(*factory).CreateKubeConfig(0xc0000f4c30, 0xc00004d8a0, 0x8, 0x12)
	…/pkg/jx/cmd/factory.go:469 +0x20c
github.com/jenkins-x/jx/pkg/jx/cmd.(*factory).CreateJXClient(0xc0000f4c30, 0x41f6c8, 0xc0006d01e0, 0x7fe45fbc57a3, 0x40, 0x1e, 0xc000557810)
	…/pkg/jx/cmd/factory.go:347 +0x2f
github.com/jenkins-x/jx/pkg/jx/cmd.(*CommonOptions).JXClient(0xc0007a1500, 0x10, 0x23bcc20, 0x27afacb, 0xa, 0x10, 0x10)
	…/pkg/jx/cmd/common.go:199 +0x8c
github.com/jenkins-x/jx/pkg/jx/cmd.(*CommonOptions).JXClientAndDevNamespace(0xc0007a1500, 0x59cb5b, 0x0, 0x27bc313, 0xc000557850, 0xc000557840, 0x40c278)
	…/pkg/jx/cmd/common.go:245 +0x170
github.com/jenkins-x/jx/pkg/jx/cmd.(*CommonOptions).GetDevEnv(0xc0007a1500, 0xc0006d01e0, 0xc000720000)
	…/pkg/jx/cmd/common_app.go:19 +0x2f
github.com/jenkins-x/jx/pkg/jx/cmd.NewCmdUpgradeApps(0x2b9d020, 0xc0000f4c30, 0x2b59540, 0xc00000c010, 0x2b59580, 0xc00000c018, 0x2b4ba60, 0xc00000c020, 0xc000720000)
	…/pkg/jx/cmd/upgrade_apps.go:88 +0x20e
github.com/jenkins-x/jx/pkg/jx/cmd.NewCmdUpgrade(0x2b9d020, 0xc0000f4c30, 0x2b59540, 0xc00000c010, 0x2b59580, 0xc00000c018, 0x2b4ba60, 0xc00000c020, 0xc000760a00)
	…/pkg/jx/cmd/upgrade.go:65 +0x748
github.com/jenkins-x/jx/pkg/jx/cmd.NewJXCommand(0x2b9d020, 0xc0000f4c30, 0x2b59540, 0xc00000c010, 0x2b59580, 0xc00000c018, 0x2b4ba60, 0xc00000c020, 0xc00029da10)
	…/pkg/jx/cmd/cmd.go:75 +0x4d0
github.com/jenkins-x/jx/cmd/jx/app.Run(0xc000785f88, 0xc00005a0b8)
	…/cmd/jx/app/jx.go:16 +0xce
main.main()
	…/cmd/jx/jx.go:10 +0x22

(It took some experimentation but AFAICT createKubeConfig is the lowest-level function whose presence indicates some code about to make a network call.) Ideally there would be a *_test.go somewhere which actually tries to run jx prompt and sets some global flag blocking createKubeConfig from working. However I did not yet manage to replicate the Cobra code to run a command without using os.Args; maybe using SetArgs.

Testing: other than existing automated tests and the new test case, I verified that some common commands and jx help and jx help subcommand subsubcommand etc. work as expected. (jx help does contact the API server, but that is OK.) I did not attempt to test functionality of either apps or plugins. From what I can tell, there is some existing test coverage for locally installed plugin commands, but not managed plugins, and none for the affected app subcommands. Maybe BDD tests fill in the gaps?

@jenkins-x-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jstrachan

If they are not already assigned, you can assign the PR to them by writing /assign @jstrachan in a comment when ready.

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

@pmuir
Copy link
Contributor

pmuir commented Dec 19, 2018

Sorry, I missed this until now. Will look at it tomorrow, but I know what the problem is.

@jglick
Copy link
Contributor Author

jglick commented Dec 19, 2018

@pmuir ack. I got as far as diagnosing the particular features which cause the problem, and writing a test which passes iff those are all commented out. Maybe you want to make a subsuming PR fixing the problems, since that is probably beyond me.

@jglick jglick changed the title [DO NOT MERGE] Demonstrating what needs to be suppressed to make jx prompt fast again fix: making jx prompt fast again Dec 27, 2018
@jglick jglick changed the title fix: making jx prompt fast again fix: make jx prompt fast again Dec 27, 2018
if cmd == nil {
panic("nil root command")
}
templater := &templater{
RootCmd: cmd,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tip: Diff settings » Hide whitespace changes

@jglick
Copy link
Contributor Author

jglick commented Dec 27, 2018

Trying to diagnose this test failure.

…DoNotMatch & TestUninstallOptions_Run_ContextSpecifiedViaCli_PassWhenContextNamesMatch were not expecting a namespace name to be printed in green text.
@pmuir
Copy link
Contributor

pmuir commented Jan 4, 2019

/hold

I think this is the right fix, but I'll build on top of it to apply the validation of CLI arguments during Run().

@pmuir pmuir self-assigned this Jan 4, 2019
@pmuir pmuir mentioned this pull request Jan 4, 2019
2 tasks
@jglick jglick deleted the slow-startup branch January 9, 2019 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants