Skip to content
This repository was archived by the owner on Jul 18, 2025. It is now read-only.

Conversation

@aduermael
Copy link
Contributor

@aduermael aduermael commented Nov 26, 2015

This PR addresses #12.

Lua plugin had to execute goproxy with arguments, and they were transmitted to the goproxy daemon over http. The goproxy daemon could send information to the Lua plugin using http requests as well.

This PR replaces all this with a single TCP connection and a simple JSON protocol. Lua plugin and goproxy daemon can use this to exchange JSON messages.

JSON strings can be sent in both directions, each sequence should end with '\n'.
It means JSON data has to be condensed, it can't contain '\n' characters (outside of string values). This protocol can be improved later, but it works so far.

type TCPMessage struct {
    Cmd  string   `json:"cmd"`
    Args []string `json:"args"`
    // Id is used to associate requests & responses
    Id   int         `json:"id"`
    // To store arbitrary data
    Data interface{} `json:"data"`
}

Tagged as WIP because it has to be heavily tested.

Also, I listed these necessary fixes:

  • Reading from cNetwork TCP client is non-blocking. I guess this should be fixed in Cuberite. I'll take a look at it myself: http://apidocs.cuberite.org/cNetwork.html (blocking at this level doesn't mean it will be blocked at the Lua level). This is why I added a delay in goproxy/main.go, in handleConn function, a few lines after conn.Read(buf[cursor:]). It's not necessary to have this in order to merge the PR. The delay in go code will be sufficient for now.
  • Stats are monitored after a "start" event is received and for each running container when listing them all. But I never checked to see if go routines end correctly when the containers are stopped.
  • Lua script should keep trying to establish TCP connection when it fails, and refresh all containers when it finally gets connected.

Choose a reason for hiding this comment

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

The dtFall constant should be available globally, you should use the constant rather than a magic number, because the constant's integral value may change in a future version of the server.

Ref.: http://apidocs.cuberite.org/Globals.html#DamageType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! This was from a commit by @dave-tucker, I don't know what happened with my merge... But I can fix it.

@aduermael
Copy link
Contributor Author

Stats are monitored after a "start" event is received and for each running container when listing them all. But I never checked to see if go routines end correctly when the containers are stopped.

I'm currently working on this. I tested it, and the answer is no, stats monitoring go routines never end... I'm fixing it.

@aduermael
Copy link
Contributor Author

Stats are monitored after a "start" event is received and for each running container when listing them all. But I never checked to see if go routines end correctly when the containers are stopped.

This one should be fixed now! :)

@aduermael
Copy link
Contributor Author

I'll try to merge that PR finally next week, I would like to work on funnier stuff! :)

Adrien Duermael and others added 18 commits September 27, 2016 12:15
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@docker.com>
Signed-off-by: Adrien Duermael <adrien@duermael.com>
Signed-off-by: Adrien Duermael <adrien@duermael.com>
Signed-off-by: Adrien Duermael <adrien@duermael.com>
Signed-off-by: Adrien Duermael <adrien@duermael.com>
Signed-off-by: Adrien Duermael <adrien@duermael.com>
Signed-off-by: Adrien Duermael <adrien@duermael.com>
Signed-off-by: Adrien Duermael <adrien@duermael.com>
@aduermael
Copy link
Contributor Author

@dave-tucker @gdevillele ok, that was a hell of a merge...
This is not super clean, but we should try to break it and merge it if stable enough.
There are huge differences with previous code base, this definitely has to be merged before we start adding new features. (networks, volumes, images, etc.)

@aduermael aduermael changed the title [WIP] Using TCP connection and simple JSON protocol instead of HTML Using TCP connection and simple JSON protocol instead of HTML Sep 27, 2016
@dave-tucker
Copy link
Contributor

HOLY REBASE BATMAN! 😲
Great work @aduermael 🎉
Couple of nits, but otherwise LGTM! 🌴

  • Could we move the go code back to the root of the project?
    Go is extremely picky about location and interprets folders as part of the package name.
    E.g github.com/docker/dockercraft/go
  • Could we also put the files back in the $GOPATH in the container? e.g
    /go/src/github.com/docker/dockercraft
    It's important for making sure the vendored packages are picked up correctly

@aduermael
Copy link
Contributor Author

aduermael commented Sep 28, 2016

@dave-tucker I can totally do that! Thought would really like to understand why it's necessary, as Dockercraft can't be used as a package for another Go application.

I'm preparing a commit with to move the files!

@dave-tucker
Copy link
Contributor

@aduermael I guess it's not entirely necessary but we did it in #59 so it seems silly to revert it again. It's probably me being an idealist as I would consider it a "best practice" 😉
We might one day add a package that someone else wants to use! Who knows!

Signed-off-by: Adrien Duermael <adrien@duermael.com>
@aduermael
Copy link
Contributor Author

@dave-tucker ok! I understand, thanks!
Last commit moved all the files back to root! :)
I also fixed a few things to please golint. 😛
Merging now!

@aduermael aduermael merged commit cfdaa2d into docker-archive-public:master Sep 28, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants