Mysql cli #353

smacker · 2019-03-19T14:57:12Z

Most probably it fixes some other issue but they need to be tested with this patch.

It currently uses mysql image and separate container to run client. When src-d/gitbase#733 is fixed & released we can switch to using that image. Or we can use some other client such as mysqlsh.

It will be also possible to refactor it without starting separate container but attaching to gitbase one.
Most probably it should be very little change.

smacker · 2019-03-19T15:01:19Z

go mod check has failed 🤦‍♀️ Please don't mind it now, I'll figure how to fix it and push it later.

smacker · 2019-03-19T15:22:30Z

Okay. I was able to fix it only by updating go to 1.12.1 locally, removing local cache, removing go.sum, removing vendor. Missing any of this was producing a different result 🤦‍♀️

carlosms

I've tested it a bit and works great for the normal usage.
Pushing the limits a bit I found these problems:

I cannot use the up arrow to use the latest command, or ctrl+R to search the history
If I exit the client with exit the mysql container is stopped, but not if I exit with ctrl+C

Then, there is the missing parts to put this new component at the same level as the other components:

It does not appear in the srcd components list output
It is not included in other commands, like srcd prune --with-images. Which I'm not sure if it should... if engine installed the mysql image then yes, it should be pruned. But you may have that image before using engine 🤔

I guess you didn't include the new component in components list and other commands because we will eventually use the mysql client from the gitbase image.
But we may have to do an engine release before gitbase does a new release. Ideally I think we should have our master ready to release at any moment.

What do you think? should we make this new component a "first class" component before merging? or keep this PR open until gitbase includes the mysql client?

carlosms · 2019-03-20T10:45:29Z

cmd/srcd/cmd/sql.go

-	// Might have to pull some images
-	ctx, cancel := context.WithTimeout(context.Background(), 1440*time.Minute)
-	defer cancel()
+func installMysqlCli(client api.EngineClient) error {


Nitpick, but since the function also starts gitbase, I would either change the function name, or do the gitbase start outside of the function.

do you think installMysqlCliWithDeps is a good name for it?
I do everything in 1 function because this way I'm able to use defer and print log message only once.

Makes sense to bundle it in one function. I guess what seemed a bit strange to me is here:

if err := installMysqlCli(client); err != nil { fatal(err, "could not install mysql client") } connReady := logAfterTimeoutWithSpinner("waiting for gitbase to be ready", 5*time.Second, 0) err = ensureConnReady(client) connReady() if err != nil { fatal(err, "could not connect to gitbase") }

It reads like you install the mysql client, and then you check the connectivity to gitbase. Maybe this function should be called startGitbaseWithClient?

Honestly it's a small nickpick, leave as it is if you think it's ok.

startGitbaseWithClient is a good name. Applied in: src-d/sourced-ce@9542724

also, I changed the signature so the function doesn't return an error anymore. I did it to make error handling for gitbase & mysql-cli consistent.

se7entyse7en · 2019-03-20T18:59:34Z

cmd/srcd/cmd/sql.go

+		}
+		defer resp.Close()
+		defer func() {
+			err := docker.RemoveContainer(components.MysqlCli.Name)


Why you remove the container? Isn't it better to remove it only on srcd stop?

that is a very good question. I did it for a reason for sure. But I don't remember this reason anymore... 🤔

ahhh. because the container wasn't presented in the components list and stop wasn't stopping it. After the recent change, I think I don't need to remove it anymore.

Ooops. No. We still need it. To show help & mysql > with current implementation.
It's possible to run new mysql command in the same container each time but it would require more changes. Do you want me to do it in this PR?

No I'd consider it as an improvement, thanks for the clarification!

Now I remember the original reason. I looked at how web client works. It removes container on exit. So I did the same for cli client.

(moved this comment from a wrong thread)

smacker · 2019-03-20T18:59:43Z

I cannot use the up arrow to use the latest command, or ctrl+R to search the history

that is interesting. Thanks! It works in the client itself. Need to investigate more why it doesn't work using current attach. Though this PR already solves many problems. Maybe we can fix it in sequential pr. (I'm not sure how hard it will be to fix lacking features)

If I exit the client with exit the mysql container is stopped, but not if I exit with ctrl+C

I'll fix it. Should be simple.

It does not appear in the srcd components list output
It is not included in other commands, like srcd prune --with-images. Which I'm not sure if it should... if engine installed the mysql image then yes, it should be pruned. But you may have that image before using engine 🤔

I'll fix it. Should be simple.

se7entyse7en · 2019-03-20T19:04:48Z

BTW well done! 👏 and also to @carlosms for pushing the limits!

smacker · 2019-03-20T19:59:16Z

Update:

add client into components list -> done
consistent behavior for Ctrl-C & exit
fix for Ctrl-D (discovered myself)

Pending:

now, after adding it to components list it's possible to keep container running, though it would require adding Exec command to docker. Shouldn't be very difficult but most probably quite many changes
fix special keys like "Ctrl-R" and arrow-up. Right now I have no idea why they don't work, need to investigate

Do you guys want me to implement pending things in this PR or in sequential ones? (imo this pr is big enough already and pending things are more features than bugs)

se7entyse7en · 2019-03-20T20:50:11Z

imo this pr is big enough already

yup, I agree. I'd close this and open issues for the pending things.

se7entyse7en · 2019-03-20T21:16:03Z

Just one thing. Could you make more clear whether we want to give priority to the input from piping or from args? What I mean is that we could move the reading of args just before checking if the stdin is a pipe. So something like this:

		var query string
		if len(args) == 1 {
			query = strings.TrimSpace(args[0])
		} else {
			fi, _ := os.Stdin.Stat()
			if (fi.Mode() & os.ModeCharDevice) == 0 {
				fmt.Println("b")
				b, err := ioutil.ReadAll(os.Stdin)
				if err != nil {
					fatal(err, "could not read input")
				}

				query = string(b)
			}
		}

or like this:

		fi, _ := os.Stdin.Stat()
		if (fi.Mode() & os.ModeCharDevice) == 0 {
			fmt.Println("b")
			b, err := ioutil.ReadAll(os.Stdin)
			if err != nil {
				fatal(err, "could not read input")
			}

			query = string(b)
		} else if len(args) == 1 {
			query = strings.TrimSpace(args[0])
		}

depending on to which we want to give priority. We could also move it in a separate function to make the flow of the code easier to read.

se7entyse7en · 2019-03-20T21:53:07Z

cmd/srcd/cmd/sql.go

+		defer stopMysqlClient()
+
+		// in case of Ctrl-C or kill defer wouldn't work
+		ch := make(chan os.Signal, 1)


Maybe we could unify this with the logic in attachStdio? I was thinking about sending nil (or an error identifying the signal) to ouputDone or inputDone when one of the two signals arrive.

we can't put this logic into attachStdio because we need to handle signal correctly for the case when we don't attach stdin too. Example: you run select * from files which takes a lot of time, Ctrl-C should stop the query and remove container correctly. Instead of writing additional code for this case we can handle it using this function in both cases with or without stdin.

ok! gonna approved then! thank!

smacker · 2019-03-21T12:10:51Z

Imo priority for arg or stdin doesn't matter. Mixing them would work weird anyway. For example how mysql-cli itself works with it:

root@2a07b9ea86ca:/# echo "select 2" | mysql -h srcd-cli-gitbase -e "select 1"
1
1

It prefers arguments over stdin but changes output format to -B one.

I changed to if/else to make it more clear but I disagree about moving it to function. I think we should improve it later by really piping stdin to mysql-cli instead of overriding the query. (but as I mentioned in the comment it would change current output)

carlosms · 2019-03-21T12:52:23Z

docker/docker.go

@@ -519,3 +519,52 @@ func GetLogs(ctx context.Context, containerID string) (io.ReadCloser, error) {

 	return reader, err
 }
+
+func Attach(ctx context.Context, config *container.Config, host *container.HostConfig, name string) (*types.HijackedResponse, chan int64, error) {


By Attach it sounds like this command should be used like docker attach, over a running container. So you call Start and then Attach.

But this method already starts the container.
I guess you need to it this way for the attach config settings. Is there a way to attach to a running container? I think it would be more clear.
It not, I would consider changing the method name. StartAttach? And documenting what it does.

this command actually works as docker run -it. The difference from normal docker attach is that it attaches BEFORE starting the container. So you have all the output of the container.
You can try with docker by running docker run mysql mysql -e "select * from files" and then docker attach. You will get only part of the output since the moment you attached.

I don't see any use case when engine would need attach to a running container. So I named it just Attach not AttachRun or StartAttach. But I don't mind to rename it if you prefer so.

Later we can implement exec that actually attaches to a running container and runs a command. It should be very similar code. (most probably reuse parts of this function). Though for exec we need image that runs something as pid 1 and doesn't exit. Like new gitbase. For the current image run is a better fit.

Thanks for the explanation. Attaching before starting the container is a very good reason to keep it like it's now 👍.

Maybe we could add comment explaining that it's similar to run -it

Signed-off-by: Maxim Sukharev <max@smacker.ru>

smacker · 2019-03-21T13:40:36Z

rebased on master due to conflicts

smacker requested a review from a team March 19, 2019 14:57

carlosms reviewed Mar 20, 2019

View reviewed changes

se7entyse7en reviewed Mar 20, 2019

View reviewed changes

se7entyse7en mentioned this pull request Mar 21, 2019

Test for interactive sql REPL #334

Closed

se7entyse7en approved these changes Mar 21, 2019

View reviewed changes

carlosms reviewed Mar 21, 2019

View reviewed changes

smacker added 14 commits March 21, 2019 14:24

initial implementation of mysql cli as srcd sql

8ad59d5

Signed-off-by: Maxim Sukharev <max@smacker.ru>

go mod tidy

5c9c1e2

Signed-off-by: Maxim Sukharev <max@smacker.ru>

support piping

4b2192d

Signed-off-by: Maxim Sukharev <max@smacker.ru>

use new fatal function from master

e5dd21e

Signed-off-by: Maxim Sukharev <max@smacker.ru>

update integration tests

5b6b6b1

Signed-off-by: Maxim Sukharev <max@smacker.ru>

return exit code from docker.Attach and use it in srcd sql

3b153e4

Signed-off-by: Maxim Sukharev <max@smacker.ru>

update tests

cb7c1c7

Signed-off-by: Maxim Sukharev <max@smacker.ru>

rename installMysqlCli to startGitbaseWithClient

4bd96ca

Signed-off-by: Maxim Sukharev <max@smacker.ru>

add mysql-cli into components list

e27b6ff

Signed-off-by: Maxim Sukharev <max@smacker.ru>

stop mysql-cli container on Ctrl-C

fa61291

Signed-off-by: Maxim Sukharev <max@smacker.ru>

fix Ctrl-D handling

87b2cac

Signed-off-by: Maxim Sukharev <max@smacker.ru>

update components test

62aa0b3

Signed-off-by: Maxim Sukharev <max@smacker.ru>

make priority of arg vs stdin more clear

f682ecd

Signed-off-by: Maxim Sukharev <max@smacker.ru>

add documentation to docker.Attach function

722ee19

Signed-off-by: Maxim Sukharev <max@smacker.ru>

carlosms approved these changes Mar 21, 2019

View reviewed changes

smacker merged commit c7cdeee into src-d:master Mar 21, 2019

carlosms mentioned this pull request Mar 28, 2019

"exit" on srcd sql doesn't exit command prompt #215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mysql cli #353

Mysql cli #353

smacker commented Mar 19, 2019

smacker commented Mar 19, 2019

smacker commented Mar 19, 2019

carlosms left a comment

carlosms Mar 20, 2019

smacker Mar 20, 2019

carlosms Mar 20, 2019

smacker Mar 20, 2019 •

edited

Loading

se7entyse7en Mar 20, 2019

smacker Mar 20, 2019

smacker Mar 20, 2019

smacker Mar 20, 2019

se7entyse7en Mar 20, 2019

smacker Mar 21, 2019

smacker commented Mar 20, 2019

se7entyse7en commented Mar 20, 2019

smacker commented Mar 20, 2019

se7entyse7en commented Mar 20, 2019

se7entyse7en commented Mar 20, 2019

se7entyse7en Mar 20, 2019

smacker Mar 21, 2019

se7entyse7en Mar 21, 2019

smacker commented Mar 21, 2019

carlosms Mar 21, 2019

smacker Mar 21, 2019

carlosms Mar 21, 2019

smacker Mar 21, 2019

smacker commented Mar 21, 2019

Mysql cli #353

Mysql cli #353

Conversation

smacker commented Mar 19, 2019

smacker commented Mar 19, 2019

smacker commented Mar 19, 2019

carlosms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smacker Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smacker commented Mar 20, 2019

se7entyse7en commented Mar 20, 2019

smacker commented Mar 20, 2019

se7entyse7en commented Mar 20, 2019

se7entyse7en commented Mar 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smacker commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smacker commented Mar 21, 2019

smacker Mar 20, 2019 •

edited

Loading