execution_profile: whitelist/blacklist filtering

[muzarski]

Fixes: #243

This PR introduces whitelist/blacklist host filtering to the driver.

Filtering rules

There are currently 4 filtering rules (we could introduce 2 more in the future - rack filtering):

• ip whitelist
• ip blacklist
• dc whitelist
• dc blacklist

The logic for checking if a host is accepted is following (in the specified order):

1. if ip whitelist is non-empty, and if it does not contain host's ip -> reject
2. if ip blacklist is non-empty, and if it contains host's ip -> reject
3. if dc whitelist is non-empty, and if it does not contain host's dc -> reject
4. if dc blacklist is non-empty, and if it contains host's dc -> reject

HostFilter

Since all of the execution profiles are defined before session creation (in other words: user cannot define new profiles after session was created and connected), cpp-driver (and so, cpp-rust-driver as well) can see which hosts are disabled by all execution profiles.

If there is a host which is rejected by all execution profiles (including the default one), the driver does not open the connection to such host. This is exactly what we do in this PR - after collecting all of the filtering rules of each profile, we compute the filtering rules for our custom HostFilter implementor - namely CassHostFilter. This is done by computing the unions of whitelists and intersection of blacklists.

Vec or HashSet

For now, I decided to use Vec to represent the set of items in the whitelists/blacklists. I'd say that we do not expect them to contain a lot of items, so I think Vec is more efficient than HashSet in this specific case. One could argue that this code is not critical (and I agree) - this is a configuration phase. I'm open to suggestions.

Integration tests

ExecutionProfile suite

I have not enabled any yet - they require cass_future_coordinator, which is used to check the ip of host the request was routed to. TBH, I think this is a blocker for merging - I prefer to wait for the cass_future_coordinator, and enable the corresponding tests (and maybe implement some additional tests on my own). I still believe that this PR is ready for review, though.

HostFilter suite

This is the suite I implemented myself - currently there are two tests that reject all nodes using execution profiles - in result, session does not have any connections to route the requests to.

DisconnectedNullStringApiArgs suite

Enabled remaining 4 test cases from this suite - they check whether driver behaves as expected when null string is provided to filtering config methods.

Pre-review checklist

• I have split my patch into logically separate commits.
• All commit messages clearly explain what they change and why.
• PR description sums up the changes and reasons why they should be introduced.
• I have implemented Rust unit tests for the features/changes introduced.
• I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
• I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

[muzarski]

I have already found a bug in my HostFilter filtering rules computation. Consider following scenario:
We have two nodes: "ip1" and "ip2". Let's say we have two execution profiles, one with the whitelist being "ip1", the other with empty (disabled) whitelist. In result, the HostFilter whitelist will be a union of these -> "ip1". In result, we filter out the "ip2" node - even though it is accepted by at least one execution profile (the one with empty whitelist).

I'll fix it - the nonempty_union should return None, if at least one of the provided sets is empty (i.e. one of the whitelist rules is disabled). This proves that we need the tests before merging this.

Note: the nonempty_intersection looks good in such scenario, because if at least one of the blacklists is empty, the resulting HostFilter blacklist will be empty (disabled) as well.

[muzarski]

Rebased on master.

[muzarski]

v2:

• addressed @Lorak-mmk comments (haven't transitioned to HashSet yet - still to be discussed)
• fixed the bug I mentioned above
• introduced HostFilterTest suite and implemented two test cases where we disable all nodes (driver has no connections to route the queries).

[muzarski]

v2.1: Previously I implemented new tests, but forgot to enable them..

[Lorak-mmk]

Looks good to me. I don't really care if we use Vec or HashSet, so I'm approving now. If you decide to change the container, then I'll re-approve.

[muzarski]

v2.2:

• Rebased on master
• Implemented set_[whitelist/blacklist]_filtering methods using update_comma_delimited_list utility method introduced here. I removed the set_filtering helper function which was in my first version of this PR - it's the same as update_comma_delimited_list.
• Enabled remaining DisconnectedNullStringApiArgs tests

[wprzytula]

❓ You haven't marked:

I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

Did you just overlook them?

[muzarski]

❓ You haven't marked:

I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

Did you just overlook them?

The ExecutionProfile tests that are testing filtering methods require support for exposing/enforcing coordinator. I'm waiting for these to be implemented. Then, we can enable the corresponding tests and merge this PR (I don't want to merge it without tests enabled).

[muzarski]

v2.2: Addressed @wprzytula comments (some of them still need an answer)

[muzarski]

v2.3: Next iteration of addressing @wprzytula comments. As a bonus (after discussion on Slack), I added casts to fn(&str) -> T for closures with empty env.

[muzarski]

Thanks for the approves - I'm not merging yet. I'll wait for cass_future_coordinator support to enable the corresponding integration tests.

[Lorak-mmk]

As a bonus (after discussion on Slack), I added casts to fn(&str) -> T for closures with empty env.

Why? Did you check that it really improves something? In other words, are multiple copies of the function generated (in release mode) if we remove those casts, as compared to the current version?
We never did such casts for a gazzilion methods that accept a closure (e.g. in every iterator usage).

[muzarski]

As a bonus (after discussion on Slack), I added casts to fn(&str) -> T for closures with empty env.

Why? Did you check that it really improves something? In other words, are multiple copies of the function generated (in release mode) if we remove those casts, as compared to the current version? We never did such casts for a gazzilion methods that accept a closure (e.g. in every iterator usage).

Without casts:

nm -an target/release/libscylla_cpp_driver.so | c++filt | grep comma_delimi

0000000000141730 t scylla_cpp_driver::cluster::update_comma_delimited_list::h625b382240391297
0000000000141970 t scylla_cpp_driver::cluster::update_comma_delimited_list::ha5386510b8545111
0000000000141b70 t scylla_cpp_driver::cluster::update_comma_delimited_list::hb5560b92aa22d880
0000000000141d70 t scylla_cpp_driver::cluster::update_comma_delimited_list::hea0076943954fdf8
0000000000466eb8 d scylla_cpp_driver::cluster::update_comma_delimited_list::__CALLSITE::META::hf3d04d6f74274d08
00000000004ce2e8 d scylla_cpp_driver::cluster::update_comma_delimited_list::__CALLSITE::h1a7572fa57806091

With casts:

nm -an target/release/libscylla_cpp_driver.so | c++filt | grep comma_delimi

00000000001427b0 t scylla_cpp_driver::cluster::update_comma_delimited_list::ha48524fffb4b98c2
00000000001429f0 t scylla_cpp_driver::cluster::update_comma_delimited_list::hf1fb12ec9c9e5165
0000000000465fe8 d scylla_cpp_driver::cluster::update_comma_delimited_list::__CALLSITE::META::hf3d04d6f74274d08
00000000004cd2e8 d scylla_cpp_driver::cluster::update_comma_delimited_list::__CALLSITE::h1a7572fa57806091

[Lorak-mmk]

So probably it is a tradeoff of inlining vs less copies, right? In other words, with casts we get 2 less copies, but the functions won't be inlined. Do we have reasons to believe this is the right tradeoff to do here?

[muzarski]

Do we have reasons to believe this is the right tradeoff to do here?

I don't have any. Maybe @wprzytula has some.

[wprzytula]

So probably it is a tradeoff of inlining vs less copies, right? In other words, with casts we get 2 less copies, but the functions won't be inlined. Do we have reasons to believe this is the right tradeoff to do here?

By telling the compiler that this is the same function, we leave the decision up to it whether to inline or not.
By not saying it, the compiler may still inline (and then the code ends up equivalent) or not. If not, this is the worst case: there are multiple copies of the same function, each called from another place in code. With our hint, all calls can target the same function.

Tl;dr
@muzarski's experiment proves that the compiler can do better if hinted in a way @muzarski did.

[muzarski]

So probably it is a tradeoff of inlining vs less copies, right? In other words, with casts we get 2 less copies, but the functions won't be inlined. Do we have reasons to believe this is the right tradeoff to do here?

By telling the compiler that this is the same function, we leave the decision up to it whether to inline or not. By not saying it, the compiler may still inline (and then the code ends up equivalent) or not. If not, this is the worst case: there are multiple copies of the same function, each called from another place in code. With our hint, all calls can target the same function.

I misunderstood @Lorak-mmk at first as well - he meant inlining of the closure provided to update_comma_delimited_list, and not the update_comma_delimited_list itself.

[muzarski]

v2.4: Removed the casts for closures (after a thorough discussion on Slack)