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

etcdmain: correctly check return values from SdNotify() #6006

Merged

Conversation

dongsupark
Copy link

Since coreos/go-systemd@0d7fccc, SdNotify() in go-systemd started to return 2 values, sent and err. So startEtcdOrProxyV2() in etcd needs to check the 2 return values correctly. As the 2 values are independent of each other, error checking logic needs to be slightly updated too. SdNotifyNoSocket, which was previously provided by go-systemd, does not exist any more. In that case (false, nil) will be returned instead.

This was a direct reason of a travis-ci failure in fleet like this: https://travis-ci.org/coreos/fleet/builds/146093283. Although fleet has nothing to do with this API change, travis always fetches the most recent master branch for each git repo.

@dongsupark dongsupark changed the title Dongsu/fix build error go systemd etcdmain: correctly check return values from SdNotify() Jul 20, 2016
@heyitsanthony
Copy link
Contributor

@dongsupark vendor updates need to be done through godep so the vendored packages match an upstream revision. Try running scripts/updatedep.sh with the recent go-systemd in the gopath?

@dongsupark dongsupark force-pushed the dongsu/fix-build-error-go-systemd branch 4 times, most recently from 5316a45 to 5b203e7 Compare July 20, 2016 19:23
@dongsupark dongsupark changed the title etcdmain: correctly check return values from SdNotify() [WIP] etcdmain: correctly check return values from SdNotify() Jul 20, 2016
@dongsupark
Copy link
Author

@heyitsanthony Thanks. I tried to update both Godeps.json and vendor tree together, but several tests started to fail. I'm not sure why. I'll look into that tomorrow.

@heyitsanthony
Copy link
Contributor

heyitsanthony commented Jul 20, 2016

@dongsupark it looks like this is upgrading some extra deps which may be causing the failures. I think it'll be OK to keep the go-systemd updates to Godeps.json and vendor/ but keep the rest the same.

@dongsupark dongsupark force-pushed the dongsu/fix-build-error-go-systemd branch from 5b203e7 to 8607ac3 Compare July 21, 2016 05:27
@dongsupark dongsupark changed the title [WIP] etcdmain: correctly check return values from SdNotify() etcdmain: correctly check return values from SdNotify() Jul 21, 2016
@dongsupark
Copy link
Author

@heyitsanthony Thanks. That did the trick! :-)
Removing go-semver and clockwork from vendor, both unit tests and functional tests work without failures. Interesting.
Please have a look.

@heyitsanthony
Copy link
Contributor

@dongsupark please squash the commits cmd/Godeps: update Godeps.json and cmd/vendor: update vendor trees according to go-systemd update into vendor: update go-systemd and it'll lgtm. Thanks!
/cc @xiang90

Godeps.json and vendor need to be updated according to the newest
go-systemd, as SdNotify() in go-systemd has changed its API.
@dongsupark dongsupark force-pushed the dongsu/fix-build-error-go-systemd branch from 8607ac3 to b438f0a Compare July 21, 2016 06:21
@dongsupark
Copy link
Author

@heyitsanthony Done. Thanks!

@dongsupark dongsupark force-pushed the dongsu/fix-build-error-go-systemd branch from b438f0a to 624187d Compare July 21, 2016 07:18
SdNotify() now returns 2 values, sent and err. So startEtcdOrProxyV2()
needs to check the 2 return values correctly. As the 2 values are
independent of each other, error checking needs to be slightly updated
too.

SdNotifyNoSocket, which was previously provided by go-systemd, does not
exist any more. In that case (false, nil) will be returned instead.
@xiang90
Copy link
Contributor

xiang90 commented Jul 21, 2016

lgtm

@heyitsanthony heyitsanthony merged commit 32553c5 into etcd-io:master Jul 21, 2016
@dongsupark
Copy link
Author

Thanks. Now travis error from fleet is gone. :-)
BTW as for the test failure in TestServeMembersCreate, culprit seems to be a recent change in github.com/jonboulle/clockwork. I'm not sure about how to fix that, and anyway that doesn't look like a critical issue, as long as etcd doesn't update the repo in the vendor tree.

@dongsupark dongsupark deleted the dongsu/fix-build-error-go-systemd branch December 1, 2016 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants