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

Dev/rsearch #1

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Dev/rsearch #1

wants to merge 24 commits into from

Conversation

flashvoid
Copy link

Reverse search

This is a utility for reverse search in kubernetes (find all objects who select given label).

Usage

Provided binary can act as either client or server depending on command line options.

Server instance connects to Kubernetes API and monitors resource changes

./rsearch -c go/src/github.com/romana/contrib/rsearch/config.ini -s

Client then can query the server

./rsearch -c go/src/github.com/romana/contrib/rsearch/config.ini -h http://localhost -r 'tier/backend#'
  • -r label to query, # sign is not a part of a label but terminator that need to be added to request (TODO at this point i'm not really sure it's needed)
  • -h url address where server instance is running

@jbrendel
Copy link

It seems that most of the code here is actually someone else's code that's been included as is. Or has it been changed here and there? I can't be sure.

Is this the best way that 3rd party packages can be handled in Go? If so, this is deeply troubling.

@jbrendel
Copy link

The Romana files here don't have any file or package comments, or even license headers and most function and type docstrings are missing as well. Can I assume that all of this will be added soon?

"net/http"
)

// SearchResource connects to instance of a server

Choose a reason for hiding this comment

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

Instance of what server?

@@ -0,0 +1 @@
vendor/

Choose a reason for hiding this comment

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

They seem to use godeps instead of vendoring, so this probably isn't needed.

Copy link
Author

Choose a reason for hiding this comment

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

I maintain godep directory, but with VENDOREXPERIMENT vendor/ folder gets created automatically. This line is to prevent accidental commits including this folder.

@@ -0,0 +1,18 @@
; comment
[api]
url=http://192.168.0.10:8080

Choose a reason for hiding this comment

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

Couldn't this just be watchURL as a single item? Does it actually vary between deployments?

Copy link
Author

Choose a reason for hiding this comment

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

Kube api url does change, namespace endpoint might not change but we want to be able to define api url separately as it's going to be used in few places.

}

// Assembling response.
for NPid, _ := range search[string(req.Tag)] {

Choose a reason for hiding this comment

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

All the map accesses are going to be racey.

Copy link
Author

Choose a reason for hiding this comment

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

Process goroutine owns all the maps and it can perform only one task at a time.

we either writing or reading, not both at same time

                        case e := <-in:
                                // On incoming event update caches
                                updateStorage(e, storage, search, config)
                        case request := <-req:
                                // On incoming search request return a list
                                // of resources with matching Selectors
                                processSearchRequest(storage, search, request, config)


package rsearch

func manageResources(ns Event, terminators map[string]chan Done, config Config, out chan Event) {

Choose a reason for hiding this comment

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

docstring

@jbrendel
Copy link

The code here is quite involved, with lots of moving parts. I would really like to see some explanatory docstring somewhere, possibly even a chapter in the README, which explains the architecture and all the various bits and parts. As it is right now, the code is unpleasant, since without a big picture you'll have a hard time making sense of it.

So, please add some explanatory docstring about the architecture somewhere.

"encoding/json"
)

func TestResoureProcessor(t *testing.T) {
Copy link

Choose a reason for hiding this comment

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

typo.

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.

4 participants