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

k8s: Age field to pod proto #1170

Closed
wants to merge 7 commits into from
Closed

k8s: Age field to pod proto #1170

wants to merge 7 commits into from

Conversation

smonero
Copy link
Contributor

@smonero smonero commented Mar 24, 2021

Add an age field representing a human friendly string that is the time elapsed since CreationTimestamp.

Description

This might be a bad idea, if it is, I'll close this PR but wanted to put this out as an attempt at fixing our problem.

In the k8s dash we want to display the age. Normally we would construct this from the timestamp. However, it might be easier to just do it like this, until protobufjs/protobuf.js#1495 is merged or something similar is done.

If it turns out this is a good idea, then I'll make a follow-up PR for doing the same thing in deployment, which can also use an Age field (similar to how lyftkube does it).

Testing Performed

local

GitHub Issue

Fixes #

TODOs

@smonero smonero requested a review from a team as a code owner March 24, 2021 16:11

// human friendly, based off
// https://github.com/lyft/lyftkube/blob/1394f8f8e17d9f3472a0387c901bdb506dde083a/lib/printers/time_format.go#L11
string age = 11;
Copy link
Contributor

Choose a reason for hiding this comment

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

So is the plan to have start time and age as separate fields in Pod? Feels redundant to have both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove start time for now.

CreationTimestamp comment in types.go:
// CreationTimestamp is a timestamp representing the server time when this object was
// created. It is not guaranteed to be set in happens-before order across separate operations.
// Clients may not set this value. It is represented in RFC3339 form and is in UTC.
//
// Populated by the system.
// Read-only.
// Null for lists.
// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata

StartTime comment from types.go:
// RFC 3339 date and time at which the object was acknowledged by the Kubelet.
// This is before the Kubelet pulled the container image(s) for the pod.

@rithujohn191
Copy link
Contributor

In the local testing performed did you have a chance to run through the delete pod flow to ensure the serialization is done correctly?

@danielhochman
Copy link
Collaborator

the twitter API used to do this, they would send the timestamp across in three formats. from memory it was something like:

  • millis since epoch (created_at_ms), e.g. 12323829382
  • ISO8601 (created_at) e.g. 2001-02-01T13:43:32Z
  • human readable (created_at_human or created_at_relative) e.g. 3h ago

they moved away from it and only send one timestamp now because the human readable version goes out-of-date. on the twitter app they want it to say 10s ago, then 30s ago, then 5 min ago if you look at the tweet several times, rather than whatever the age was when the API call was made.

for our use case it's probably fine to add the human readable representation if we can't reasonably fix the serialization bug, knowing that we may end up having to pay the deprecation cost later.

if we do decide to keep this i would keep the existing field (since we should still use it in the future and fix the bug) and call the new field <existing_field_name>_human i.e. start_time_human rather than age.

@smonero smonero marked this pull request as draft March 25, 2021 16:59
@github-actions
Copy link

github-actions bot commented Apr 2, 2021

This PR has been marked as stale after 7 or more days of inactivity. Please have a maintainer add the on hold label if this PR should remain open. If there is no further activity or the on hold label is not added, this PR will be closed in 3 days.

@github-actions github-actions bot added the stale Issue hasn't had activity in awhile label Apr 2, 2021
@smonero smonero added the on hold label Apr 2, 2021
@github-actions github-actions bot removed the stale Issue hasn't had activity in awhile label Apr 2, 2021
@smonero
Copy link
Contributor Author

smonero commented May 7, 2021

closing this for now, going to open one shortly with a different approach

@smonero smonero closed this May 7, 2021
@mikecutalo mikecutalo deleted the ageField branch July 19, 2021 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants