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

dhcp: Base for dhcp server #110

Merged
merged 1 commit into from
Nov 23, 2016
Merged

dhcp: Base for dhcp server #110

merged 1 commit into from
Nov 23, 2016

Conversation

jellonek
Copy link
Contributor

@jellonek jellonek commented Nov 9, 2016

  • base skeleton for serving 2 protocols on 2 different connection types
  • grpc on unix socket server for configuration providing/modifying/removal
  • skeleton for dhcp server
  • passing real data from configuration as options (ip/routes, hardcoded dns entries read from host?)

With this points fulfilled we will have possibility to add support for dhcp into calico branch.


This change is Reviewable

@jellonek
Copy link
Contributor Author

jellonek commented Nov 9, 2016

https://github.com/Mirantis/virtlet/pull/110/files#diff-98e28311e3529c25828c931b46319afaR45 - this was chosen as a simple way to store/restore internal metadata. Also traversing through list is suboptimal, but for now - we don't need any extreme performance.

When this will be too slow - we will be able to change saving/updating/restoring metadata in hashmap, internally in simple map, externally - in e.x. redis or something similar.
This is not added now due to unnecessary complication and no need for greater performance for this moment.

@nhlfr
Copy link
Contributor

nhlfr commented Nov 10, 2016

Review status: 0 of 11 files reviewed at latest revision, 1 unresolved discussion.


pkg/dhcp/README.md, line 5 at r1 (raw file):

```sh
  protoc --go_out=plugins=grpc:. configuration.proto

Why two spaces at the beginning?


Comments from Reviewable

@jellonek
Copy link
Contributor Author

Review status: 0 of 11 files reviewed at latest revision, 1 unresolved discussion.


pkg/dhcp/README.md, line 5 at r1 (raw file):

Previously, nhlfr (Michal Rostecki) wrote…

Why two spaces at the beginning?

Good catch. Just habit. Will fix this.

Comments from Reviewable

@jellonek jellonek force-pushed the jell/dhcp_server branch 2 times, most recently from 9a00e17 to 232c29d Compare November 15, 2016 13:11
@jellonek
Copy link
Contributor Author

For now, there is no passing any dns values - should be rethinked.

@ivan4th
Copy link
Contributor

ivan4th commented Nov 15, 2016

Review status: 0 of 12 files reviewed at latest revision, 3 unresolved discussions.


cmd/dhcpserver/dhcp-server.go, line 45 at r2 (raw file):

      os.Exit(1)
  }
  glog.Infof("Starting server on socket %s", *socketPath)

glog.V(1) maybe?


pkg/dhcp/server/dhcp_server.go, line 35 at r2 (raw file):

)

type dHCPServer struct {

somewhat strange casing, maybe dhcpServer ?


Comments from Reviewable

@jellonek
Copy link
Contributor Author

Review status: 0 of 12 files reviewed at latest revision, 3 unresolved discussions.


cmd/dhcpserver/dhcp-server.go, line 45 at r2 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

glog.V(1) maybe?

Yes, good catch.

pkg/dhcp/server/dhcp_server.go, line 35 at r2 (raw file):

Previously, ivan4th (Ivan Shvedunov) wrote…

somewhat strange casing, maybe dhcpServer ?

Specific shortcuts in golang should be named in upcase, like DNS, DHCP, IP, ID, but i tried to not export this from package and this is why it have so ugly name. I think that your proposal is much better.

Comments from Reviewable

@jellonek jellonek force-pushed the jell/dhcp_server branch 10 times, most recently from 85e9ffd to 526dc8d Compare November 18, 2016 09:45
@ivan4th
Copy link
Contributor

ivan4th commented Nov 23, 2016

LGTM, but we need tests for that, and not just integration tests.


Review status: 0 of 13 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@ivan4th
Copy link
Contributor

ivan4th commented Nov 23, 2016

:lgtm:


Review status: 0 of 13 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@jellonek jellonek mentioned this pull request Nov 23, 2016
@jellonek jellonek changed the title [WIP] dhcp: Base for dhcp server dhcp: Base for dhcp server Nov 23, 2016
@jellonek jellonek merged commit 058d988 into master Nov 23, 2016
@jellonek jellonek deleted the jell/dhcp_server branch November 23, 2016 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants