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

Expose target machine/state fields in list-units #664

Merged
merged 3 commits into from
Jul 16, 2014

Conversation

bcwaldon
Copy link
Contributor

Add new TSTATE and TMACHINE fields to list-units, dropping some irrelevant states from the default list.

In the event that you have two units that conflict with one another, this was the old output you would see:

$ fleetctl list-units
UNIT        STATE   LOAD    ACTIVE      SUB DESC    MACHINE
bar.service inactive    -   -       -   -   -
foo.service loaded  loaded  inactive    dead    -   5849e100.../172.17.8.101

...and the new output:

$ fleetctl list-units
UNIT        TSTATE      TMACHINE            STATE       MACHINE             ACTIVE
bar.service launched    -               inactive    -               -
foo.service loaded      5849e100.../172.17.8.101    loaded      5849e100.../172.17.8.101    inactive

This new format at least gives you an idea of what bar.service should be doing

@jonboulle
Copy link
Contributor

👍

@bcwaldon
Copy link
Contributor Author

@jonboulle I'm going to switch TSTATE to DSTATE - I think 'desired' makes more sense than 'target'

@jonboulle
Copy link
Contributor

ack

@jonboulle
Copy link
Contributor

Can you update documentation too?

@bcwaldon
Copy link
Contributor Author

bar

@bcwaldon
Copy link
Contributor Author

@jonboulle updated states.md along with the first two commits, but using-the-client.md was way out of date so I updated that along with some unrelated stuff in a third commit.

@@ -39,3 +40,4 @@ The systemd state is composed of three subcomponents, also exposed in `fleetctl
- `ACTIVE` (the high-level unit activation state, i.e. generalization of SUB)
- `SUB` (the low-level unit activation state, values depend on unit type)

By default, on the `ACTIVE` state is exposed from `fleetctl list-units`. Expose the `LOAD` and `SUB` fields using the `--fields` flag to `fleetctl list-units`.
Copy link
Contributor

Choose a reason for hiding this comment

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

S/on the/only the/
S/from/in/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for reading through that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonboulle fixed

@jonboulle
Copy link
Contributor

Any idea if the deis guys/anyone else are still parsing fleetctl output without --fields? Maybe worth reaching out

@bcwaldon
Copy link
Contributor Author

@gabrtv @carmstrong Is this going to bite you guys?

@carmstrong
Copy link

I'm going to switch TSTATE to DSTATE - I think 'desired' makes more sense than 'target'

Agreed - had to look at the output a few times to glean its value.

@gabrtv @carmstrong Is this going to bite you guys?

Looks like it will. We awk the output of fleetctl in several places in our Makefile. Shouldn't be a lot of code, but will require some testing to see what breaks. We're also moving away from our Makefile entirely toward a Go-based provision utility which interacts with the fleet API directly, so long-term we won't care. But right now, we do.

Any idea when this beat will drop, @bcwaldon?

@bcwaldon
Copy link
Contributor Author

@carmstrong As soon as you say we can :) This information is incredibly useful when debugging failures, so I want to get it out in people hands ASAP. Short term, you could just append this to all of your list-units commands:

--fields unit,state,load,active,sub,desc,machine

@carmstrong
Copy link

Short term, you could just append this to all of your list-units commands:

--fields unit,state,load,active,sub,desc,machine

Good enough for us. We're actually still using CoreOS 349.0.0 - we want to grab the latest alpha as soon as Docker is bumped to 1.1+, so if this change will be in that release, I'll make these changes now.

@bcwaldon
Copy link
Contributor Author

@carmstrong This change is unlikely to make it into the next alpha release, but it should make its way out within the next few.

@carmstrong
Copy link

@carmstrong This change is unlikely to make it into the next alpha release, but it should make its way out within the next few.

Sounds reasonable. We'll grab the next alpha and will probably be on that for a little while.

@bcwaldon
Copy link
Contributor Author

@jonboulle ok, sounds reasonably safe once I get your LGTM

@carmstrong
Copy link

LGTM.


Jobs may only transition directly between these states. For example, for a job to transition from `inactive` to `launched` it must first go state `loaded`.

The current job state is exposed in the `state` column of the output from `fleetctl list-units`.
The desired state and the last known states are exposed in the `DSTATE` and `STATE` columns of the output from `fleetctl list-units`.
Copy link
Contributor

Choose a reason for hiding this comment

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

last known state, or desired and last known states

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -11,7 +11,7 @@ import (
)

const (
defaultListUnitsFields = "unit,state,load,active,sub,desc,machine"
defaultListUnitsFields = "unit,dstate,tmachine,state,machine,active"
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed something - what's the real value in providing tmachine in the default output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If TMACHINE != MACHINE, something has gone wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that, just not sure it adds enough utility to be in the default output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. Until we fix #638, I believe it is incredibly important to expose in the default output. Once we feel more confident in how we're publishing state, I'm fine with removing it from the default output..

On a related note, how would you feel about implementing + and - to the --fields option? For example, running fleetctl list-units --fields -tmachine,+hash would hide the tmachine field but add the hash field. Useful?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds a bit overly complex and trouble-prone to me - at that stage why not just explicitly name all the fields? and it ties us to the default output fields more closely.

I'd entertain a PR but wouldn't do it myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree. It would basically be a shorthand for humans, but give people the ability to shoot themselves in the foot if they write a script against it.

@jonboulle
Copy link
Contributor

lgtm

bcwaldon added a commit that referenced this pull request Jul 16, 2014
Expose target machine/state fields in list-units
@bcwaldon bcwaldon merged commit 93da9c3 into coreos:master Jul 16, 2014
@bcwaldon bcwaldon deleted the 663 branch July 16, 2014 18:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants