Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

rkt list|status: app state info (i.e. exit codes) in --format=json #3638

Merged
merged 3 commits into from
Apr 21, 2017
Merged

rkt list|status: app state info (i.e. exit codes) in --format=json #3638

merged 3 commits into from
Apr 21, 2017

Conversation

fabiokung
Copy link
Contributor

Make rkt list|status --format=json compatible with the default k=v format and include state info (exit codes in particular).

Also deprecate App.AppNames in the json output, but keep it there for backwards compatibility.

Before the change:

$ rkt status --format=json f4ef2301-511f-4ab0-9ce9-fdd02a76798e | jq "."
{
  "name": "f4ef2301-511f-4ab0-9ce9-fdd02a76798e",
  "state": "exited",
  "app_names": [
    "busybox"
  ],
  "started_at": 1491855899
}

After the change:

$ rkt status --format=json f4ef2301-511f-4ab0-9ce9-fdd02a76798e | jq "."
{
  "name": "f4ef2301-511f-4ab0-9ce9-fdd02a76798e",
  "state": "exited",
  "app_names": [
    "busybox"
  ],
  "apps": [
    {
      "name": "busybox",
      "state": "exited",
      "created_at": 1491855899323730200,
      "exit_code": 12,
      "image_id": "sha512-4014cef722fd0105ad0b90a95d403a6a236f6c3fa1e45fc8b0deb319d493d524"
    }
  ],
  "started_at": 1491855899
}

Signed-off-by: Fabio Kung fabio.kung@gmail.com

@ghost
Copy link

ghost commented Apr 10, 2017

Can one of the admins verify this patch?

@lucab
Copy link
Member

lucab commented Apr 10, 2017

ok to test

@lucab
Copy link
Member

lucab commented Apr 11, 2017

This looks fine in general, however I'm wondering if we should tackle #3551 first so that this JSON is properly typed+versioned and we can then touch it with a bit more freedom in future.

@lucab
Copy link
Member

lucab commented Apr 11, 2017

@fabiokung also related, in case you want to tackle them at once: #3550.

@lucab lucab requested review from lucab and s-urbaniak April 11, 2017 07:56
@fabiokung
Copy link
Contributor Author

#3551 is a noble cause, but I don't think it should block this. I think this is a necessary baby step just to get some parity with the default tabbed/plain format.

#3550 is new information, and in that case it may make sense to start versioning these JSONs before.

s-urbaniak
s-urbaniak suggested changes Apr 12, 2017
View reviewed changes
Copy link
Contributor

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

A nit and a question, thanks a lot for the contribution!

Generally I agree with @lucab that we need to address versioning sooner than later.

lib/pod.go Outdated
@@ -63,3 +75,63 @@ func NewPodFromInternalPod(p *pkgPod.Pod) (*Pod, error) {

return pod, nil
}

// appStateInImmutablePod infers most App state from the Pod itself, since all apps are created and destroyed with the Pod
func appStateInImmutablePod(app *App, pod *pkgPod.Pod) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we put appStateInImmutablePod in lib/app.go next to appStateInMutablePod since they are related and of the same type.

lib/pod.go Outdated
finishedAt := finish.UnixNano()
app.FinishedAt = &finishedAt
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

the above state check seems a bit unnecessary (and the fallthrough indicates this fact) since we are collecting startedAt/finishedAt timepoints only, hence I suggest to skip it at all (if that is possible).

@fabiokung
rkt list|status: app state info (i.e. exit codes) in --format=json
f21e0be 
Make `rkt list|status --format=json` compatible with the default k=v
format and include state info (exit codes in particular).

Also deprecate App.AppNames in the json output, but keep it there for
backwards compatibility.

Signed-off-by: Fabio Kung <fabio.kung@gmail.com>
@fabiokung fabiokung force-pushed the json-exit-codes branch 2 times, most recently from 59e6cb6 to c81074d Compare April 12, 2017 18:46
@fabiokung
Copy link
Contributor Author

@s-urbaniak I made (and amended) the suggested changes, thanks!

@lucab I moved existing json models to a versioned package, mirroring what is being done for the gRPC API. Is that the right design? Is api/v1 the right home for them?

@fabiokung
stage0 {list,status}: start versioning json models
ab40f16 
Mark the current JSON output format for rkt status|list as v1, so we can
track changes and compatibility moving forward.

Fixes rkt#3551

Signed-off-by: Fabio Kung <fabio.kung@gmail.com>
@fabiokung
cleanup: delete unused code (function)
e6561a8 
Signed-off-by: Fabio Kung <fabio.kung@gmail.com>
s-urbaniak
s-urbaniak approved these changes Apr 19, 2017
View reviewed changes
Copy link
Contributor

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

this LGTM, but leaving the last two eyes to @lucab, thanks!

@lucab
Copy link
Member

lucab commented Apr 20, 2017

@fabiokung I'm fine with your answers and cleanup, I'm just worried that people should not rely on this JSON output until we stick some versioning on top of it.

To be explicit, I'd like to see:

$ rkt status --format=json f4ef2301-511f-4ab0-9ce9-fdd02a76798e | jq "."
{
  "kind": "rkt-status-v0",
  "value": {
    "name": "f4ef2301-511f-4ab0-9ce9-fdd02a76798e",
    "state": "exited",
    "app_names": [
      "busybox"
    ],
    "apps": [
      {
        "name": "busybox",
        "state": "exited",
        "created_at": 1491855899323730200,
        "exit_code": 12,
        "image_id": "sha512-4014cef722fd0105ad0b90a95d403a6a236f6c3fa1e45fc8b0deb319d493d524"
      }
    ],
    "started_at": 1491855899
  }
}

before touching JSON output and pointing people to it. If your usecase to expose those info is to parse them externally, I fear we are already at that point.

@euank
Copy link
Member

euank commented Apr 20, 2017 •
edited
Loading

I don't think we need to couple getting versioning right with including this change.

There's no semantic changes to the current fields, and adding new fields to a json document can generally be considered backwards compatible.

This change LGTM.

@lucab
Copy link
Member

lucab commented Apr 21, 2017

This PR has already enough LGTM and I have been persuaded by the above thread that my stance of coupling this with output-versioning is not the best course of action, so I'm merging this while keeping #3551 open. @fabiokung sorry for the back and forth and thanks for your contribution!

@lucab lucab merged commit c767336 into rkt:master Apr 21, 2017
@fabiokung fabiokung deleted the json-exit-codes branch April 23, 2017 20:01
@lucab lucab added this to the v1.26.0 milestone May 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@s-urbaniak s-urbaniak s-urbaniak approved these changes

@lucab lucab Awaiting requested review from lucab

Assignees
No one assigned
Labels
area/usability component/stage0
Projects
None yet
Milestone
v1.26.0
Development

Successfully merging this pull request may close these issues.
4 participants
@fabiokung @lucab @euank @s-urbaniak