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

List all gear units including dead ones #262

Merged
merged 10 commits into from
Jul 11, 2014
Merged

List all gear units including dead ones #262

merged 10 commits into from
Jul 11, 2014

Conversation

wallrj
Copy link
Contributor

@wallrj wallrj commented Jul 10, 2014

I've added the ?all=1 to the gear list url and added a test which demonstrates the effect.

But I'm not entirely sure which extra states are included by making that change.

For example failed units (eg units created with a non-existent docker image name) are listed anyway.

I'm also not sure how units get into the dead state. The example in my test (and the example that @itamarst used in #208 (comment)) recreate the dead state by creating a gear unit using the busybox image which exits immediately with a 0 status code. I'd have thought that systemd would attempt to restart such units a few times before marking them failed, but that doesn't seem to happen.

I also need some guidance around how to wait for the unit-under-test to reach the dead state. I can test the current state using systemctl, but that won't make any sense when running these tests against the FakeGearClient.
Perhaps this particular test should be move to the implementation specific flocker.node.functional.test_gear.GearClientTests case.

Fixes: #208

# XXX: Real units take a while to start up and die. I need to a way
# to check for the death of a unit when running these tests against
# the real GearClient (perhaps by calling `systemctl status`) but
# not when running unit tests against FakeGearClient.
Copy link
Contributor

Choose a reason for hiding this comment

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

Poll docker until the process goes away, maybe?

@itamarst
Copy link
Contributor

Address and merge, or put up for review if you're still not sure about the testing mechanism.

@wallrj
Copy link
Contributor Author

wallrj commented Jul 11, 2014

Thanks @itamarst

  • I moved the test to the functional tests
  • Added a new Unit.sub_state attribute and populate that from GearClient.list
  • I decided not to set a fixed sub_state value in FakeGearClient because I'm not sure what value to choose and the existing tests all pass, so I guess we can cross that bridge in the future.

Not sure whether I need tests for other previously unlisted states given the comment in openshift/geard#228 (comment)

Putting up for another review because I've changed quite a lot.

@wallrj
Copy link
Contributor Author

wallrj commented Jul 11, 2014

I'm also not sure exactly which "stopped" units were already being listed, prior to this change. In http://lists.openshift.redhat.com/openshift-archives/users/2014-July/msg00006.html the implication is that units that "stop cleanly" wouldn't normally be listed, but actually we've already got a test for units that have been explicitly stopped via the Gear API. I guess in that case (that test) the systemd unit exits with a non-zero code:

@wallrj
Copy link
Contributor Author

wallrj commented Jul 11, 2014

Ok. I'm going to merge after discussion with @itamarst

Richard Wall: When I moved the test to the functional test case i noticed you'd already got a test for the stopped case.

Richard Wall: And whether adding an attribute for the substate is necessary in this branch....

Richard Wall: Maybe i should wait until you';ve had a chance to review those changes.

Itamar Turner-Trauring: adding substate probably doesn't hurt

Itamar Turner-Trauring: I skimmed the branch, it seemed fine

Itamar Turner-Trauring: I would say that testing all states is useful, but not strictly necessary for now since we're not dealing with all edge cases

Richard Wall: Ok, great. Well I'll merge it then.

wallrj added a commit that referenced this pull request Jul 11, 2014
List all gear units including dead ones

Author: @wallrj
Reviewer: @itamarst

Fixes #208
@wallrj wallrj merged commit fa0368d into master Jul 11, 2014
@wallrj wallrj deleted the list-all-units-208 branch July 11, 2014 14:05
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.

GearClient.list only returns running units
2 participants