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

api: return endpoints pod identifiers #4482

Merged
merged 1 commit into from Mar 10, 2015

Conversation

simon3z
Copy link
Contributor

@simon3z simon3z commented Feb 17, 2015

This patch is work in progress and proposed as proof of concept. The idea is to connect more reliably endpoints to pods (and maybe nodes as well).

Example of the proposed api:

{
  "metadata": {
    "name": "example",
    ...
  },
  "endpoints": [
    {
      "podID": "php1",
      "endpointIP": "172.17.0.61:80"
    },
    {
      "podID": "php2",
      "endpointIP": "172.17.0.62:80"
    }
  ]
},
...
{
  "metadata": {
    "name": "kubernetes",
    ...
  },
  "endpoints": [
    {
      "endpointIP": "192.168.122.4:6443"
    }
  ]
},

Names (especially EndpointIP) and structures are open for debate. If generally accepted I'll polish the patch (comments, descriptions, names, checks, refactoring, etc.).

@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@simon3z
Copy link
Contributor Author

simon3z commented Feb 17, 2015

@bgrant0607 can you check if this proof of concept is acceptable? Thanks.

@simon3z
Copy link
Contributor Author

simon3z commented Feb 18, 2015

@thockin any comment on this? (or can you suggest anyone else that can take a look at it?) thanks.

@thockin
Copy link
Member

thockin commented Feb 18, 2015

Hilariously, I have a very similar change in progress that I started just yesterday. Why do we need pod ID here, though? The Endpoints API is intended to be a low-coupling interface, not deeply tied to Kubernetes.

@simon3z
Copy link
Contributor Author

simon3z commented Feb 18, 2015

@thockin I need it to reliably correlate services and pods. I consider the podId as additional metadata that we can add for better description (rather than a deep coupling). Maybe we can also add the hostId when it's an host endpoint, and so on.

@bgrant0607 bgrant0607 self-assigned this Feb 18, 2015
@bgrant0607
Copy link
Member

@simon3z Sorry, due to the holiday and a few days of meetings, I'm backlogged. The short answer is that podID cannot be a field here. I want Endpoints to be more general than just pods.

That said, I have thoughts about a more general metadata mechanism for Endpoints.

However, could you please explain in more detail why you want this?

@simon3z
Copy link
Contributor Author

simon3z commented Feb 19, 2015

@bgrant0607 I need it to reliably correlate services and pods.

Anyway endpoints can be more general than just pods, for example we could use an optional field "object":

"endpoints": [
  {
    "endpoint": "172.17.0.61:80",
    "object": {
       "kind": "Pod",
       "id": "php"
     }
   }
]

@bgrant0607
Copy link
Member

@simon3z Correlate them for what purpose?

@bgrant0607
Copy link
Member

Re. metadata, see:

#2585 (comment)
#2585 (comment)
#2585 (comment)

@simon3z
Copy link
Contributor Author

simon3z commented Feb 19, 2015

@bgrant0607 I need to correlate them for analysis and visualization. I want to be able to navigate from pods to their services, and the other way around.

@bgrant0607
Copy link
Member

@simon3z The service contains the label selector. Why not just GET the set of pods?

As for the other direction, we should support reverse lookup #1348 in our API. We currently do reverse lookups of replication controllers in kubectl:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/kubectl/describe.go#L338

@simon3z
Copy link
Contributor Author

simon3z commented Feb 19, 2015

@bgrant0607 given that I want to analyze all services I'll have to do a GET request for each selector (and that can't scale).

@simon3z
Copy link
Contributor Author

simon3z commented Feb 20, 2015

@bgrant0607 I updated the format to reference any kind of object (when applicable). Although the patch still needs cleanup.

{ 
  "kind": "EndpointsList",
  "apiVersion": "v1beta3",
  "metadata": {
    "selfLink": "/api/v1beta3/namespaces/endpoints",
    "resourceVersion": "29"
  },
  "items": [
    { 
      "metadata": {
        "name": "example",
        "namespace": "default",
        "selfLink": "/api/v1beta3/namespaces/default/endpoints/example",
        "uid": "391ec1b5-b926-11e4-9d80-525400c903c1",
        "resourceVersion": "25",
        "creationTimestamp": "2015-02-20T12:30:54-05:00"
      },
      "endpoints": [
        { 
          "endpoint": "172.17.0.116:80",
          "metadata": {
            "kind": "Pod",
            "namespace": "default",
            "name": "php",
            "uid": "36859334-b926-11e4-9d80-525400c903c1",
            "resourceVersion": "14"
          }
        }
      ]
    },
    { 
      "metadata": {
        "name": "kubernetes",
        "namespace": "default",
        "selfLink": "/api/v1beta3/namespaces/default/endpoints/kubernetes",
        "resourceVersion": "7",
        "creationTimestamp": null
      },
      "endpoints": [
        { 
          "endpoint": "192.168.122.4:6443",
          "metadata": {}
        }
      ]
    },

@thockin
Copy link
Member

thockin commented Feb 21, 2015

I am still not a fan of this.

If you want to navigate from all services to all pods and are concerned
about perf, could we do something like: get ALL pods, build an index, then
for each service do a LOCAL selector query (we should be able to do that
pretty easily). The result will be stale, but not more so that anything
else in a lockless system.

On Fri, Feb 20, 2015 at 9:40 AM, Federico Simoncelli <
notifications@github.com> wrote:

@bgrant0607 https://github.com/bgrant0607 I updated the format to
reference any kind of object (when applicable). Although the patch still
needs cleanup.

{
"kind": "EndpointsList",
"apiVersion": "v1beta3",
"metadata": {
"selfLink": "/api/v1beta3/namespaces/endpoints",
"resourceVersion": "29"
},
"items": [
{
"metadata": {
"name": "example",
"namespace": "default",
"selfLink": "/api/v1beta3/namespaces/default/endpoints/example",
"uid": "391ec1b5-b926-11e4-9d80-525400c903c1",
"resourceVersion": "25",
"creationTimestamp": "2015-02-20T12:30:54-05:00"
},
"endpoints": [
{
"endpoint": "172.17.0.116:80",
"metadata": {
"kind": "Pod",
"namespace": "default",
"name": "php",
"uid": "36859334-b926-11e4-9d80-525400c903c1",
"resourceVersion": "14"
}
}
]
},
{
"metadata": {
"name": "kubernetes",
"namespace": "default",
"selfLink": "/api/v1beta3/namespaces/default/endpoints/kubernetes",
"resourceVersion": "7",
"creationTimestamp": null
},
"endpoints": [
{
"endpoint": "192.168.122.4:6443",
"metadata": {}
}
]
},

Reply to this email directly or view it on GitHub
#4482 (comment)
.

@simon3z
Copy link
Contributor Author

simon3z commented Feb 21, 2015

@thockin what is a LOCAL selector query? We don't want to replicate the kubernetes selector logic and keep tracking it outside of kubernetes (implementing all the possible logic it will have: in, not-in, etc.).

Kubernetes already uses that logic to build endpoints.

Possible solutions are (some already proposed):

  • mention the services related to a pod in get-all-pods
  • mention the pods related to a service in get-all-services
  • mention the pods in the endpoints

I understand that you don't want to tie pods and services, but on the contrary in endpoints you are already matching them so it's just a matter of mentioning what is the destination object of the endpoint.

@thockin
Copy link
Member

thockin commented Feb 21, 2015

I'm hoping there is a more elegant way to back-index this all than building
pointers between all the objects. As for local queries, I was suggesting
that you could use that until such time as we have a real answer.

But like most API things, I will defer to Brian as final arbiter.

On Sat, Feb 21, 2015 at 1:46 AM, Federico Simoncelli <
notifications@github.com> wrote:

@thockin https://github.com/thockin what is a LOCAL selector query? We
don't want to replicate the kubernetes selector logic and keep tracking it
outside of kubernetes (implementing all the possible logic it will have:
in, not-in, etc.).

Kubernetes already uses that logic to build endpoints.

Possible solutions are (some already proposed):

  • mention the services related to a pod in get-all-pods
  • mention the pods related to a service in get-all-services
  • mention the pods in the endpoints

I understand that you don't want to tie pods and services, but on the
contrary in endpoints you are already matching them so it's just a matter
of mentioning what is the destination object of the endpoint.

Reply to this email directly or view it on GitHub
#4482 (comment)
.

@bgrant0607
Copy link
Member

@simon3z With respect to performance, eventually we need to index both labels and selectors, so that lookups don't require scanning the list of all objects. Listing replication controllers has this problem already. Speaking of which, this approach doesn't solve the problem for replication controllers.

I could also imagine indexing pods by IP address, for fast lookup by IP using a field selector (#1362).

Why wouldn't a forward mapping be sufficient for your needs? We've discussed representing a query that would return the targeted pods in the statuses of services and replication controllers. That would appear sufficient for a UI. Yes, it has the same performance issues at the moment, but we should fix that to make the existing API more usable.

I'll comment on the PR details, also, but note that more critical changes to services and endpoints are underway (see @thockin's PRs).

@@ -744,13 +744,18 @@ type Service struct {
Status ServiceStatus `json:"status,omitempty"`
}

type EndpointEntry struct {
Endpoint string `json:"endpoint"`
ObjectReference `json:"metadata,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that the object reference would be one field of a more general metadata map.

Also, I want to reserve the term metadata for ObjectMeta/ListMeta.

What I proposed in #2585 was:

    Info map[string]string `json:"info,omitempty"`

I understand it would be more convenient for Kubernetes-specific applications for a first-class ObjectReference field. I have been concerned about introducing too many Kubernetes-isms into this API, but perhaps I should just accept that we'll need an adaptor to a more generic API, as @thockin recommended. In that case, I'd make the field name more specific, such as targetRef.

@simon3z
Copy link
Contributor Author

simon3z commented Feb 24, 2015

We've discussed representing a query that would return the targeted pods in the statuses of services and replication controllers.

@bgrant0607 how would it be different from what I proposed here? #3872

@bgrant0607
Copy link
Member

@simon3z and I discussed this today in person (great to meet you!). My interpretation of the use case: watch all objects in the cluster and build relationships between them, without duplicating the label selection implementation and, ideally, without lots more queries, though if more queries are required, the pattern can't have quadratic complexity (e.g., for each pod do a reverse lookup on services, which results in another walk of all pods).

We're aware of other use cases for correlating endpoints back to their corresponding objects (pod or node), such as to cross-reference with resource usage data for load balancing, so I'd be amenable to adding the object reference, independent of a more general metadata publishing mechanism. It would make the API more Kubernetes-specific, but I'm more concerned with addressing the use cases at this point than keeping the API generic.

Since an Endpoint struct has been introduced, I don't think we need another wrapper struct. I'd just add a TargetRef *ObjectReference field to Endpoint.

This wouldn't solve the problem for replication controllers, other controllers, plugins, auto-scalers, etc., but it's reasonable on its own. We'll still want to do the label indexing, reverse label indexing, lookup by IP address, specification of query URLs in service and controller statuses, field selection for more efficient listing of just metadata, etc.

@simon3z
Copy link
Contributor Author

simon3z commented Feb 25, 2015

@thockin for beta1/beta2 I added a string-ObjectReference map to hold the target references, I am not sure if it's the way to go. I did it this way after noticing that you haven't touched the endpoint structures in beta1/beta2.

@simon3z
Copy link
Contributor Author

simon3z commented Feb 26, 2015

@bgrant0607 @thockin I hate the implementation for v1beta1/beta2 but it has to be there today to make it work. Anyway if you're going to drop v1beta1/beta2 in the next few weeks it shouldn't be a problem I suppose.

@simon3z simon3z force-pushed the endpoints-podid branch 4 times, most recently from b4934f8 to 1bba8bf Compare March 2, 2015 12:47
@simon3z
Copy link
Contributor Author

simon3z commented Mar 2, 2015

@thockin it's not the first time that shippable fails the build without any information. Is there a way to know what went wrong? It seems all fine to me. Thanks!

@thockin thockin added cla: yes and removed cla: no labels Mar 2, 2015
@thockin
Copy link
Member

thockin commented Mar 2, 2015

I kicked shippable, but at this point I'll take either Travis or Shippable
:)

On Mon, Mar 2, 2015 at 7:39 AM, Federico Simoncelli <
notifications@github.com> wrote:

@thockin https://github.com/thockin it's not the first time that
shippable fails the build without any information. Is there a way to know
what went wrong? It seems all fine to me. Thanks!

Reply to this email directly or view it on GitHub
#4482 (comment)
.

@simon3z
Copy link
Contributor Author

simon3z commented Mar 3, 2015

@bgrant0607 I think I addressed all the things that were mentioned, can you check if there's anything else to fix? Thanks

@googlebot googlebot added cla: no and removed cla: yes labels Mar 3, 2015
// EndpointObjectReference is a reference to an object exposing the endpoint
type EndpointObjectReference struct {
Endpoint string `json:"endpoint" description:"endpoint exposed by the referenced object"`
ObjectReference
Copy link
Member

Choose a reason for hiding this comment

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

Needs json tag and description.

@bgrant0607
Copy link
Member

Sorry, massively overloaded. Fields need json descriptions. Suggested simplification for v1beta1/2. Otherwise, looks reasonable.

@simon3z
Copy link
Contributor Author

simon3z commented Mar 5, 2015

@bgrant0607 pr is updated (expect for the v1beta1/2 simplification which I commented about).

@simon3z
Copy link
Contributor Author

simon3z commented Mar 10, 2015

@bgrant0607 I rebased but it seems that the coverall check is stuck (travis passed). Do you think we can merge this?

@@ -798,6 +798,9 @@ type Endpoint struct {

// Required: The destination port to access.
Port int `json:"port" description:"destination port of this endpoint"`

// Optional: The kubernetes object related to the entry point.
TargetRef *ObjectReference `json:"targetRef,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, v1beta3 now requires descriptions, also.

@bgrant0607
Copy link
Member

Will merge as soon as a description is added to the v1beta3 field and at least one of the builds passes.

Signed-off-by: Federico Simoncelli <fsimonce@redhat.com>
@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2015
@simon3z
Copy link
Contributor Author

simon3z commented Mar 10, 2015

@bgrant0607 this should be ok I think. Sorry tonight I can't wait for travis to finish in order to either ping you or revise the patch (it's getting late here now). Anyway if later you'll remember to check if it was successful then you can merge. Thanks!

@bgrant0607
Copy link
Member

Shippable passed.

bgrant0607 added a commit that referenced this pull request Mar 10, 2015
api: return endpoints pod identifiers
@bgrant0607 bgrant0607 merged commit d8c6e34 into kubernetes:master Mar 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants