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

app: node: Add raft snapshot parameter options #143

Merged
merged 2 commits into from
Jun 23, 2021

Conversation

MathieuBordere
Copy link
Contributor

This PR uses the new dqlite API dqlite_node_set_snapshot_params available from dqlite 1.8.0.

I want to update the SONAME of dqlite from libdqlite.so.0 to libdqlite.so.1 so that go-dqlite binaries built against a version of dqlite that contain this API fail when ran against an older version of the dqlite library. The problem is, I already released a version of libdqlite with SONAME libdqlite.so.0 that contains the dqlite_node_set_snapshot_params functionality.

I'm not entirely sure if I should:
a) make a new dqlite release 1.9.0 and update the SONAME ? <-- This probably seems most reasonable
b) just update the SONAME on master and not make a release ?
c) ... ?

@stgraber
Copy link
Contributor

@MathieuBordere hmm, to me moving from libdqlite.so.0 to libdqlite.so.1 would normally be the sign of an API break, not simple symbol addition.

Unless we need to move to libdqlite.so.1 so that SEMVER works properly and then lets us do libdqlite.so.1.1.0, libdqlite.so.1.2.0, ... in the future and the same isn't currently true with libdqlite.so.0.

@MathieuBordere
Copy link
Contributor Author

@MathieuBordere hmm, to me moving from libdqlite.so.0 to libdqlite.so.1 would normally be the sign of an API break, not simple symbol addition.

Unless we need to move to libdqlite.so.1 so that SEMVER works properly and then lets us do libdqlite.so.1.1.0, libdqlite.so.1.2.0, ... in the future and the same isn't currently true with libdqlite.so.0.

I'll experiment some more with it, I need libtool to somehow spit out a libdqlite.so.0.1 SONAME for it to work properly I think.

@MathieuBordere
Copy link
Contributor Author

@stgraber So there's the -Wl,-z,relro,-z,now linker option to fail the loading of a binary containing unknown symbols, but go only allows setting them when explicitly allowed through the environment variable CGO_LDFLAGS_ALLOW, so go get will fail when users haven't set this environment variable.

There's another alternative, link but that doesn't seem to be available on all platforms, so some more configuration magic would be needed in libraries that want to use this and it won't work immediately, it will work once 2 versions of the library have applied this method.

There's no clear winning alternative for me, what would you prefer?

@stgraber
Copy link
Contributor

@MathieuBordere I think if we can tweak the Makefile in lxd that may be good enough.

@MathieuBordere
Copy link
Contributor Author

Let me have a look at those TestIntegration_ExecBindError failures, they only started coming up recently.

@MathieuBordere
Copy link
Contributor Author

MathieuBordere commented Jun 16, 2021

@MathieuBordere I think if we can tweak the Makefile in lxd that may be good enough.

Adapting export CGO_LDFLAGS_ALLOW="-Wl,-wrap,pthread_create" to something like export CGO_LDFLAGS_ALLOW="(-Wl,-wrap,pthread_create)|(-Wl,-z,now)" should do the trick normally.

@MathieuBordere MathieuBordere force-pushed the snapshot-params branch 2 times, most recently from f9e69f7 to 604df1c Compare June 16, 2021 13:49
@MathieuBordere
Copy link
Contributor Author

test/roles.sh is hanging a bit more than expected, want to make sure it's not related to this change in any way.

Mathieu Borderé added 2 commits June 23, 2021 15:14
A user should set the environment variable CGO_LDFLAGS_ALLOW="-Wl,-z,now"
see https://github.com/golang/go/wiki/InvalidFlag

Signed-off-by: Mathieu Borderé <mathieu.bordere@canonical.com>
@MathieuBordere MathieuBordere marked this pull request as ready for review June 23, 2021 13:43
@stgraber stgraber merged commit b2e31f3 into canonical:master Jun 23, 2021
@MathieuBordere MathieuBordere deleted the snapshot-params branch December 9, 2022 10:29
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.

None yet

2 participants