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

PodInfo should be a list, not a map #3622

Closed
abonas opened this issue Jan 20, 2015 · 25 comments · Fixed by #5915
Closed

PodInfo should be a list, not a map #3622

abonas opened this issue Jan 20, 2015 · 25 comments · Fixed by #5915
Assignees
Labels
area/api Indicates an issue on api area. area/usability priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@abonas
Copy link
Contributor

abonas commented Jan 20, 2015

Comparing a single container details in "spec" and the "status" sections,
there are some differences in what data is exposed per container in those sections:

  1. container is identified with index array in "spec", and its name is a property inside the index.
    while in "status" its main element is its name.
  2. in the "status" which is the runtime online representation, there's no info about ports, memory, cpu, etc.
    even when container is running.

podports

@erictune
Copy link
Member

@bgrant0607 PTAL

@abonas
Copy link
Contributor Author

abonas commented Jan 20, 2015

@erictune what is PTAL?

@erictune
Copy link
Member

PTAL = "please take a look" or "please take another look".

@thockin
Copy link
Member

thockin commented Jan 20, 2015

As per docs/api-conventions.md "the convention is to use a list of
subobjects containing name fields"

#2004

It sounds like status is buggy

On Tue, Jan 20, 2015 at 8:43 AM, Eric Tune notifications@github.com wrote:

PTAL = "please take a look" or "please take another look".

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

@erictune erictune added the area/api Indicates an issue on api area. label Jan 20, 2015
@abonas
Copy link
Contributor Author

abonas commented Jan 20, 2015

but what about the other issue - missing info on ports/resources?

@bgrant0607
Copy link
Member

/cc @dchen1107

@bgrant0607
Copy link
Member

Yes, status should not be a map.

@bgrant0607
Copy link
Member

@abonas What info would you like to see about ports?

We plan to introduce a separate API for resource information. See the section about usage here:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/docs/resources.md

/cc @rjnagal

@bgrant0607 bgrant0607 added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Jan 20, 2015
@abonas
Copy link
Contributor Author

abonas commented Jan 21, 2015

@bgrant0607 when I query for pod information and its containers, it's important to see what's going on with them in runtime. so if spec was ports XYZ, cpu A and memory B, I would like to see if indeed it matches the spec or is different. were the ports allocated? same ports? different ports?
otherwise, having only the spec information, there's nothing user can do to monitor/manage the runtime.

@abonas
Copy link
Contributor Author

abonas commented Jan 21, 2015

also, having an additional api means it's requiring more requests to server, more merging work and resolving relationships on client side. not very convenient for clients.

@abonas
Copy link
Contributor Author

abonas commented Jan 21, 2015

I will open a separate issue on the ports missing info, and keep this one for the buggy status structure

@bgrant0607 bgrant0607 changed the title REST api v3 running containers (in status section) contain very little information PodInfo should be a list, not a map Jan 21, 2015
@bgrant0607 bgrant0607 added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Feb 5, 2015
@bgrant0607 bgrant0607 modified the milestone: v1.0 Feb 6, 2015
@piosz
Copy link
Member

piosz commented Feb 20, 2015

@dchen1107 are you working on it?

@dchen1107
Copy link
Member

not at this moment, feel free grabbing it.

@piosz piosz assigned piosz and unassigned dchen1107 Feb 20, 2015
@piosz
Copy link
Member

piosz commented Feb 26, 2015

@bgrant0607
I'm trying to change PodInfo to be a list.

Should it only be changed in api v3 or also in v1 and v2? In general do we still want to modify api v1 and v2 or are those versions final?

How about internal representation of PodInfo? Do we want to keep it as it is or change to be compatible with external type? There is some internal code (pkg/kubelet/dockertools/docker.go) which relies on that PodInfo is a map. Do we want to try use externally and internally the same types when possible?

@thockin
Copy link
Member

thockin commented Feb 27, 2015

For consistency in beta3, I still endorse this. Brian is contemplating the
longer term of maps vs lists, and this may yet reverse. I don't see that
happening in v1.

I would make this change only in b3 and internal. If keeping it as a map
is important internally, we CAN do that, but it requires a conversion func,
which I know Brian is trying to avoid.

On Thu, Feb 26, 2015 at 4:57 AM, Piotr Szczesniak notifications@github.com
wrote:

@bgrant0607 https://github.com/bgrant0607
I'm trying to change PodInfo to be a list.

Should it only be changed in api v3 or also in v1 and v2? In general do we
still want to modify api v1 and v2 or are those versions final?

How about internal representation of PodInfo? Do we want to keep it as it
is or change to be compatible with external type? There is some internal
code (pkg/kubelet/dockertools/docker.go) which relies on that PodInfo is a
map. Do we want to try use externally and internally the same types when
possible?

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

@bgrant0607
Copy link
Member

I recommend waiting on this until at least Monday.

@piosz
Copy link
Member

piosz commented Feb 27, 2015

Thanks for the info.

@bgrant0607 ping me when you're ready.

@bgrant0607
Copy link
Member

Sorry for the delay. Hold off on this a bit longer. Debate is ongoing. I recommend choosing something else to work on in the short term.

@piosz
Copy link
Member

piosz commented Mar 3, 2015

No problem. Sure, I'm working also on other issues. Please let me know the decision will be made.

@bgrant0607
Copy link
Member

Ok, decision is lists: #4889.

Sorry for the delay.

@piosz
Copy link
Member

piosz commented Mar 10, 2015

Thanks for the info.

@thockin
Copy link
Member

thockin commented Mar 11, 2015

I did not get the sense that a decision had been made and I've been
watching that thread...

On Tue, Mar 10, 2015 at 11:01 AM, Brian Grant notifications@github.com
wrote:

Ok, decision is lists: #4889
#4889.

Sorry for the delay.


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

@brendandburns
Copy link
Contributor

I don't think that this makes the v1.0 cut.

@brendandburns brendandburns modified the milestones: v1.0-bubble, v1.0 Mar 23, 2015
@bgrant0607 bgrant0607 added priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. and removed priority/backlog Higher priority than priority/awaiting-more-evidence. labels Mar 23, 2015
@bgrant0607
Copy link
Member

We were trying to get this into v1beta3, since it currently violates our stated API convention.

That said, I'm fine with demoting the priority at this point, esp. since it's in status rather than spec.

@piosz
Copy link
Member

piosz commented Mar 24, 2015

I'm about to finish this change (still need to fix one test)

piosz added a commit to piosz/kubernetes that referenced this issue Mar 26, 2015
This change is to make API consistent with our convention.

Fixes kubernetes#3622
akram referenced this issue in akram/kubernetes Apr 7, 2015
This change is to make API consistent with our convention.

Fixes #3622
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue on api area. area/usability priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants