Skip to content
This repository has been archived by the owner on Dec 1, 2018. It is now read-only.

Added Master Metrics API types #1151

Merged
merged 1 commit into from
May 12, 2016
Merged

Added Master Metrics API types #1151

merged 1 commit into from
May 12, 2016

Conversation

piosz
Copy link
Contributor

@piosz piosz commented May 9, 2016 •
edited
Loading

@piosz
Copy link
Contributor Author

piosz commented May 9, 2016

cc @davidopp @mwielgus

@lavalamp as we briefly discussed f2f in MTV few weeks ago metrics api types will be defined in Heapster. Is this still valid agreement?

@k8s-bot
Copy link

k8s-bot commented May 9, 2016

Jenkins GCE e2e

Build/test failed for commit c49a066.

@piosz
Copy link
Contributor Author

piosz commented May 9, 2016

@k8s-bot test this

@k8s-bot
Copy link

k8s-bot commented May 9, 2016

Jenkins GCE e2e

Build/test failed for commit c49a066.

@erictune
Copy link

erictune commented May 9, 2016

This is the first I have heard that Master Metrics types would be in Heapster. Can I see a link to the discussion where the change in plans was made?

@k8s-bot
Copy link

k8s-bot commented May 9, 2016

Jenkins GCE e2e

Build/test failed for commit 77d7660.

@piosz
Copy link
Contributor Author

piosz commented May 9, 2016 •
edited
Loading

@erictune to make it clear (at least what I understood from sync with CSI team):

  1. CSI team is working on 'discovery merger' project which is about to allow other components than apiserver to serve Kubernetes API. There will be a kind of proxy introduced in front of apiserver which will redirect user's queries to appropriate component, so that the user won't notice any difference while talking with the api provider. If you need more details please ask @lavalamp
  2. Heapster will act as 3rd party api server which will handle metrics requests. The types will be defined there to make client generation possible (@caesarxuchao did I understand correct this part?)
  3. For 1.3 we decided with @davidopp and @fgrzadkowski that we don't want to implement those mechanisms and only define first version of the api and make it available in Heapster, that's why I haven't written the implementation proposal.

@erictune erictune assigned davidopp and unassigned erictune May 9, 2016
lavalamp
lavalamp reviewed May 9, 2016
View reviewed changes
metrics/apis/metrics/v1alpha1/types.go
Timestamp unversioned.Time `json:"timestamp"`
Window unversioned.Duration `json:"window"`

// The memory usage is the memory working set.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using a key containing "working set" so this is clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one of possibility although we agree in proposal (kubernetes/kubernetes#25279) to use just ResourceMemory constant. We may revisit this decision later since this is just alpha version of the API.

@lavalamp
Copy link
Contributor

lavalamp commented May 9, 2016

Location looks fine to me. I can't comment as to whether the contents are right or not :)

This was referenced May 10, 2016
Closed
Merged
@piosz
Copy link
Contributor Author

piosz commented May 11, 2016

friendly ping @davidopp

davidopp
davidopp reviewed May 12, 2016
View reviewed changes
metrics/apis/metrics/v1alpha1/types.go
v1.ObjectMeta `json:"metadata,omitempty"`

// The following fields define time interval from which metrics were
// collected in the following format [Timestamp-Window, Timestamp].
Copy link

@davidopp davidopp May 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

instead of "in the following format" I would say "from the interval"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@piosz
Copy link
Contributor Author

piosz commented May 12, 2016

comments applied

@davidopp
Copy link

LGTM

@piosz
Copy link
Contributor Author

piosz commented May 12, 2016

Thanks!

@davidopp davidopp added the lgtm Indicates that a PR is ready to be merged. label May 12, 2016
@davidopp
Copy link

cc/ @vishh

@piosz piosz merged commit e76c60a into kubernetes-retired:master May 12, 2016
@piosz piosz deleted the metrics-api-types branch May 12, 2016 09:01
@k8s-bot
Copy link

k8s-bot commented May 12, 2016

Jenkins GCE e2e

Build/test failed for commit d58a4c6.

@k8s-bot
Copy link

k8s-bot commented May 12, 2016

Jenkins GCE e2e

Build/test failed for commit ad14bd2.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@piosz @k8s-bot @erictune @lavalamp @davidopp @googlebot