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

discoveryserver: implement a minimal discovery API on top of v3 APIs #10627

Closed
wants to merge 29 commits into from

Conversation

philips
Copy link
Contributor

@philips philips commented Apr 11, 2019

To make the etcd discovery service maintainable by the CNCF (coreos/discovery.etcd.io#63) we should make it easy to run on the v3 API and storage.

This discoveryserver package implements enough of the v2 API alongside the existing discovery.etcd.io API. I have tested it just now with the hack/insta-discovery bring-up script and it worked enough to do that.

This PR is operating under a deadline. One of the AWS nodes that is running discovery.etcd.io is scheduled for deletion on April 16th. So, ideally I can get a Kubernetes cluster up, an etcd Operator backed etcd cluster, and this code all running by end of week.

@philips
Copy link
Contributor Author

philips commented Apr 11, 2019

Oh, and the reason I would like to put this in the tree here is because trying to vendor the v2store and v2v3 pkgs was a nightmare.

@philips
Copy link
Contributor Author

philips commented Apr 13, 2019

Everything is working and deployed here for demonstration purposes. My testing shows it works just fine: https://discovery.ifup.org/new

@xiang90
Copy link
Contributor

xiang90 commented Apr 14, 2019

@philips I skimmed this PR. Overall, looks reasonable good, just a few nits.

@philips
Copy link
Contributor Author

philips commented Apr 15, 2019 via email

@codecov-io
Copy link

codecov-io commented Apr 18, 2019

Codecov Report

Merging #10627 into master will increase coverage by 0.42%.
The diff coverage is 69.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10627      +/-   ##
==========================================
+ Coverage   71.51%   71.94%   +0.42%     
==========================================
  Files         394      406      +12     
  Lines       36658    36988     +330     
==========================================
+ Hits        26217    26610     +393     
+ Misses       8595     8479     -116     
- Partials     1846     1899      +53
Impacted Files Coverage Δ
discoveryserver/handlers/robots.go 0% <0%> (ø)
discoveryserver/handlers/home.go 0% <0%> (ø)
discoveryserver/timeprefix/time.go 100% <100%> (ø)
discoveryserver/handlers/httperror/httperror.go 100% <100%> (ø)
discoveryserver/handlers/ctx.go 100% <100%> (ø)
discoveryserver/metrics/metrics.go 100% <100%> (ø)
discoveryserver/handlers/garbage.go 56.25% <56.25%> (ø)
discoveryserver/handlers/token.go 58.66% <58.66%> (ø)
discoveryserver/handlers/health.go 65.21% <65.21%> (ø)
discoveryserver/handlers/new.go 73.97% <73.97%> (ø)
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8146e1e...e0ea2c5. Read the comment docs.

@philips philips force-pushed the discovery branch 5 times, most recently from 4b56423 to c891b68 Compare April 24, 2019 23:56
gyuho and others added 7 commits April 27, 2019 03:54
_config is part of the spec, fix this
This handles PUT/DELETE/GET via the v2v3 storage API.
With this commit hack/insta-discovery/discovery now works to spin up a
three node cluster!
Brandon Philips added 18 commits April 27, 2019 03:54
This is necessary for the watch behavior
The dockerfile needs the full tree context for vendor so put it in the
root.
This makes it so watches work sufficient for a discovery client.
The CoreOS docs are no longer maintained. Point the home to the github
docs on how to use the discovery service.
follow Go styling and lowercase all of the error messages
We don't want to continue on an unrecognized method, return from the
function.
This could happen on all verbs, not just GET.
Include the verb to ease debugging. This was really annoying when trying
to get the integration tests running again
Remove v2 specific stuff, and away it goes.
Add periodic GC proteted by a mutex in the server. Also add a simple
integration test.
Replace with Kube instructions for discovery.etcd.io
staticcheck identified deprecated package and unused variables. Fix.
Ensure that the client is syncing with the known etcd machines regularly
Use a custom metrics registry so this program doesn't pickup all of the
random guages registered via init functions in the rest of etcd.
@philips
Copy link
Contributor Author

philips commented May 2, 2019

The semaphore tests are bogus and this service is in production. Can someone help review and merge this?

@xiang90
Copy link
Contributor

xiang90 commented May 2, 2019

@philips I will give it a look over the weekend.

@philips
Copy link
Contributor Author

philips commented Jul 17, 2019

@hexfusion @xiang90 can someone please give this a review so we can get this merged?

@philips
Copy link
Contributor Author

philips commented Sep 6, 2019

Ping. This is both in production and in need of a code review.

@hexfusion
Copy link
Contributor

@philips I will take a look this week.

@gyuho
Copy link
Contributor

gyuho commented Sep 19, 2019

@hexfusion Please help on this. This is implemented in a separate package, so it's pretty low risk.

@stale
Copy link

stale bot commented Apr 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 6, 2020
@philips philips closed this Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants