Skip to content

Adding logging to keymanager and cluster store object create.#2422

Merged
dperny merged 1 commit intomoby:masterfrom
anshulpundir:cleanups
Nov 10, 2017
Merged

Adding logging to keymanager and cluster store object create.#2422
dperny merged 1 commit intomoby:masterfrom
anshulpundir:cleanups

Conversation

@anshulpundir
Copy link
Copy Markdown
Contributor

@anshulpundir anshulpundir commented Oct 31, 2017

We recently saw an issue where the cluster object was not found in the store during keymanager init. Adding some minor debug info. Probably not super helpful, but suggestions welcome.

cc @nishanttotla

Signed-off-by: Anshul Pundir anshul.pundir@docker.com

@GordonTheTurtle
Copy link
Copy Markdown

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "cleanups" git@github.com:anshulpundir/swarmkit.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

rootCA))

if err != nil {
log.G(ctx).WithError(err).Errorf("Error creating cluster object")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's make sure that this error contains the cluster id. Is there any other info we can log here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Which cluster id do you want to log ? The one that already exists ? A cluster object may not always exist.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 31, 2017

Codecov Report

Merging #2422 into master will decrease coverage by 3.33%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2422      +/-   ##
==========================================
- Coverage   63.89%   60.56%   -3.34%     
==========================================
  Files          64      128      +64     
  Lines       11788    26409   +14621     
==========================================
+ Hits         7532    15994    +8462     
- Misses       3649     9009    +5360     
- Partials      607     1406     +799

unlockKeys,
rootCA))

if err != nil {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Make this if err != nil && err != store.ErrExist.

On most leadership changes the object will already exist.

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
Copy link
Copy Markdown
Collaborator

@dperny dperny left a comment

Choose a reason for hiding this comment

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

LGTM

@dperny dperny merged commit 6032116 into moby:master Nov 10, 2017
@anshulpundir anshulpundir deleted the cleanups branch November 10, 2017 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants