Create IP to hostname mapping for Medusa restores #524
Conversation
Miles-Garnsey
force-pushed
the
feature/create-medusa-mapping
branch
3 times, most recently
from
c3ea7ac to
667eed6
Compare
There was a problem hiding this comment.
Great stuff so far @Miles-Garnsey !
I left a few minor comments, but this is clearly headed in the right direction.
pkg/medusa/hostmap.go
Outdated
| ToTargetSourceMap() map[HostName]HostDNSOrIP | ||
| } | ||
|
|
||
| // Transform HostMappingSlide into a map with source IPs as keys and Target IPs as values. |
There was a problem hiding this comment.
| // Transform HostMappingSlide into a map with source IPs as keys and Target IPs as values. | |
| // Transform HostMappingSlice into a map with source IPs as keys and Target IPs as values. |
pkg/medusa/hostmap.go
Outdated
| return nil, err | ||
| } | ||
| out := HostMappingSlice{} | ||
| for sourceRackLocation, sourceRackNodes := range sourceRacks { |
There was a problem hiding this comment.
Maybe we could relax the need to have matching rack names straight away by:
- checking that we have the same number of racks
- picking the dest rack based on the source rack position in a sorted key list instead of its name. This way "rack1" could be matched with "r1" for example.
The racks would have to be sorted alphabetically on both sides so we can have the right assignments.
wdyt?
There was a problem hiding this comment.
There are three solutions to this problem.
Solution 1
In the current solution, rack names must match, and the racks must be the same size between the source and destination topologies, but they do not need to be balanced (i.e. all racks the same size). Additionally, users can control the mapping with precision.
Solution 2
Under your proposal, rack names may differ, but the racks must be balanced and users cannot control the placements with precision (except by manipulating the alphabetical ordering which is OK, but a bit hacky).
Solution 3
If we don't care about being able to control placement, we can eliminate requirements on both balance and naming via the following:
- Add a function which takes a
map[nodeLocation]stringand returnsmap[len(nodes)]nodeLocation. - Put sources and targets though that function.
- Do a "join" on the two maps via a for loop using len(nodes) as the key.
This has the advantage that racks may be unbalanced AND names need not match. But users still cannot control placement since (in the standard case where all racks are the same size) the source-destination mapping will be randomised.
In some ways, I'd rather force the rack names to match as it gives more control. But I'm not committed to any particular solution here.
There was a problem hiding this comment.
Discussion we had offline (with thanks to @adejanovski for the info):
- Network topology strategy actually allocates replicas based on rack names (replicas go to the racks alphabetically close to the primary replica).
- The EC2 aware snitch will actually modify rack names.
As a result of these two facts, it isn't appropriate to (a) force users to use the same rack names between source and destination clusters or (b) allow customers to change the token ordering relative to the rack-name alphabetical ordering.
Further changes to account for this logic incoming.
There was a problem hiding this comment.
The EC2 aware snitch will actually modify rack names
Does it matter that we always use GossipingPropertyFileSnitch?
There was a problem hiding this comment.
I'm not sure @jsanda but it is a worthwhile callout. Based on conversation with Alexander, it sounded like he was also concerned about the replicas being allocated into the same racks (which happens based on the alphabetical ordering of their names). I didn't quite follow that part 100%.
@adejanovski your thoughts are welcome. I have to be honest, the most recent round of changes has left me less than happy with this code as I feel that the data structures etc. that I originally wrote (when I was going to force rack names to match) are now poorly suited to the new requirements around alphabetical ordering.
There was a problem hiding this comment.
Does it matter that we always use GossipingPropertyFileSnitch?
No, it's fine. It's just worth mentioning the EC2Snitch because the source cluster could be using it.
|
It all looks very good to me @Miles-Garnsey 👍 One last thing is missing if you check #486, which is the |
|
@adejanovski my latest commit adds a function Thanks for the kind words on the code too, I'm not personally that happy with it, but if it meets your needs that's great. |
|
Final unit tests finished/added. I also noticed that there is no way that we can actually restore a cluster with unbalanced racks, because we don't allow for setting rack size anywhere at the cass-operator level here so I guess all of those considerations weren't actually worth worrying about too much (or would be part of a different ticket anyway). I think this is ready to merge if there isn't anything else to consider. |
There was a problem hiding this comment.
Just a tiny request for one or two additional unit tests, but otherwise it all looks very good to me 👍
Let's hold off from merging at the moment as this PR relies on another one that's yet to be approved.
This work may need some rebasing if the solution for spinning up the standalone medusa pod turns out to be different than what's currently implemented.
| return out, nil | ||
| } | ||
|
|
||
| func (s HostMappingSlice) IsInPlace() (bool, error) { |
There was a problem hiding this comment.
LGTM 👍
It looks like that function is not tested. Would it be possible to add a test for both outcomes?
There was a problem hiding this comment.
Gah, sorry, I did have these written, I just hadn't committed/pushed. Don't know how I missed that. Check again now.
Allows the operator to communicate with the Medusa storage backend without requiring a Cassandra pod to be up.
Miles-Garnsey
force-pushed
the
feature/create-medusa-mapping
branch
from
8e15c6e to
6b331fa
Compare
|
Closing in favor or #980 |
What this PR does:
Creates a mapping from the hosts in a given remote Medusa backup (on a cloud bucket) to targets derived from a K8ssandraCluster's projected hostnames.
Which issue(s) this PR fixes:
Fixes #486
Checklist