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

Add tail and since to service logs #31500

Merged
merged 1 commit into from Mar 14, 2017

Conversation

Projects
None yet
6 participants
@dperny
Contributor

dperny commented Mar 2, 2017

- What I did
This change adds the ability to do --tail and --since on docker service logs.

Should also fix #31307.

Part of a series of changes in #31399.

- How I did it
It wires up the API endpoints to each other and fixes some older bugs.

- How to verify it
Make a service, note how passing --tail and --since

- Description for the changelog

docker service logs now supports since and tail options.

- A picture of a cute animal (not mandatory but encouraged)

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Mar 2, 2017

Contributor

Can you please add tests for these features?

Contributor

aaronlehmann commented Mar 2, 2017

Can you please add tests for these features?

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Mar 2, 2017

Contributor

I'm finally remembering to run make test before I push but now the hurdle is remembering to actually write new tests.

I'll get right on it @aaronlehmann .

Contributor

dperny commented Mar 2, 2017

I'm finally remembering to run make test before I push but now the hurdle is remembering to actually write new tests.

I'll get right on it @aaronlehmann .

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Mar 3, 2017

Contributor

pushed up new changes. tests are gonna fail, this requires a revendor from swarmkit master, which someone mentioned is currently in a different PR

Contributor

dperny commented Mar 3, 2017

pushed up new changes. tests are gonna fail, this requires a revendor from swarmkit master, which someone mentioned is currently in a different PR

c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
// make sure task has been deployed.
waitAndAssert(c, defaultReconciliationTimeout, d.CheckActiveContainerCount, checker.Equals, 1)

This comment has been minimized.

@aaronlehmann

aaronlehmann Mar 4, 2017

Contributor

I think this could be racy. If service logs runs before the container finishes its output, the test might fail. Should we wait for the task to finish running instead of waiting for it to start running?

@aaronlehmann

aaronlehmann Mar 4, 2017

Contributor

I think this could be racy. If service logs runs before the container finishes its output, the test might fail. Should we wait for the task to finish running instead of waiting for it to start running?

This comment has been minimized.

@dperny

dperny Mar 4, 2017

Contributor

you're almost certainly correct, and the possibility of a race also exists in the since test as well, as I've just realized (sleeping does not guarantee that anything else has happened on the system in the meantime.)

@dperny

dperny Mar 4, 2017

Contributor

you're almost certainly correct, and the possibility of a race also exists in the since test as well, as I've just realized (sleeping does not guarantee that anything else has happened on the system in the meantime.)

c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "")
// make sure task has been deployed.
waitAndAssert(c, defaultReconciliationTimeout, d.CheckActiveContainerCount, checker.Equals, 1)

This comment has been minimized.

@aaronlehmann

aaronlehmann Mar 4, 2017

Contributor
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 5, 2017

Member

This is not introducing API changes (i.e. the API already supported this, and is documented?)

Member

thaJeztah commented Mar 5, 2017

This is not introducing API changes (i.e. the API already supported this, and is documented?)

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Mar 6, 2017

Contributor

@thaJeztah I'm not sure if this is documented or not, but the API endpoints and CLI flags all existed, they just weren't wired up.

Contributor

dperny commented Mar 6, 2017

@thaJeztah I'm not sure if this is documented or not, but the API endpoints and CLI flags all existed, they just weren't wired up.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 6, 2017

Member

It's probably here; https://github.com/docker/docker/blob/f9bd8ec8b268581f93095c5a80679f0a8ff498bf/api/swagger.yaml#L7646-L7726, but check if it's all documented.

Given that it's been an experimental feature, if we expect this feature to leave experimental in docker 17.04, it should be mentioned in the API changes for API 1.27 https://github.com/docker/docker/blob/3a88a24d23e6eb1ca521cd9ab6e306d4ba1c1464/docs/api/version-history.md#v127-api-changes (or 1.28 if it's gonna leave experimental in 17.05

Member

thaJeztah commented Mar 6, 2017

It's probably here; https://github.com/docker/docker/blob/f9bd8ec8b268581f93095c5a80679f0a8ff498bf/api/swagger.yaml#L7646-L7726, but check if it's all documented.

Given that it's been an experimental feature, if we expect this feature to leave experimental in docker 17.04, it should be mentioned in the API changes for API 1.27 https://github.com/docker/docker/blob/3a88a24d23e6eb1ca521cd9ab6e306d4ba1c1464/docs/api/version-history.md#v127-api-changes (or 1.28 if it's gonna leave experimental in 17.05

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Mar 7, 2017

Contributor

Right now there is a flaky test in my local branch that I'm trying to sort out before I finish this PR.

Contributor

dperny commented Mar 7, 2017

Right now there is a flaky test in my local branch that I'm trying to sort out before I finish this PR.

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Mar 7, 2017

Contributor

Fixed the flaky integration test.

Contributor

dperny commented Mar 7, 2017

Fixed the flaky integration test.

@dperny dperny referenced this pull request Mar 7, 2017

Closed

Make Docker Service Logs GA #31399

7 of 12 tasks complete
@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Mar 8, 2017

Contributor

There probably needs to be a revendoring for the integration test to pass

Contributor

dperny commented Mar 8, 2017

There probably needs to be a revendoring for the integration test to pass

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Mar 8, 2017

Contributor

This definitely need revendoring for the integration tests to pass. There is a separate open PR for revendoring swarmkit.

Contributor

dperny commented Mar 8, 2017

This definitely need revendoring for the integration tests to pass. There is a separate open PR for revendoring swarmkit.

@dperny dperny referenced this pull request Mar 8, 2017

Merged

Vendor swarmkit d60ccf3 #31535

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Mar 8, 2017

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Mar 8, 2017

Contributor

Rebasing, running tests, and pushing now.

Contributor

dperny commented Mar 8, 2017

Rebasing, running tests, and pushing now.

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Mar 9, 2017

Contributor

test failure is almost certainly not my fault, can it be rerun?

Contributor

dperny commented Mar 9, 2017

test failure is almost certainly not my fault, can it be rerun?

@aluzzardi aluzzardi added this to the 17.04.0 milestone Mar 10, 2017

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Mar 10, 2017

Member

Tentatively targeting 17.04.0

Member

aluzzardi commented Mar 10, 2017

Tentatively targeting 17.04.0

@aluzzardi

This comment has been minimized.

Show comment
Hide comment
@aluzzardi

aluzzardi Mar 10, 2017

Member

@dperny I see a few of @aaronlehmann's comments not being addressed - could you double check?

Member

aluzzardi commented Mar 10, 2017

@dperny I see a few of @aaronlehmann's comments not being addressed - could you double check?

@dperny

This comment has been minimized.

Show comment
Hide comment
@dperny

dperny Mar 10, 2017

Contributor

His comments don't appear addressed because the diff for those particular lines is the same; the comments have been addressed after those lines.

Contributor

dperny commented Mar 10, 2017

His comments don't appear addressed because the diff for those particular lines is the same; the comments have been addressed after those lines.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Mar 10, 2017

Contributor

LGTM

Contributor

aaronlehmann commented Mar 10, 2017

LGTM

Add tail and since to service logs
This change adds the ability to do --tail and --since on docker service
logs. It wires up the API endpoints to each other and fixes some older
bugs. It adds integration tests for these new features.

Signed-off-by: Drew Erny <drew.erny@docker.com>
@thaJeztah

LGTM

@vdemeester

LGTM 🐸

@vdemeester vdemeester merged commit 1d46080 into moby:master Mar 14, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 31706 has succeeded
Details
janky Jenkins build Docker-PRs 40189 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 357 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 11268 has succeeded
Details
z Jenkins build Docker-PRs-s390x 235 has succeeded
Details

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment