Switch mongo client to connect to a fully defined replicaset. #11360
Conversation
|
Jenkins results:
|
|
And is a printout from a manual connection to the already up an running MongoDBAsAService cluster in testbed as a proof that we are having the full set of the replilcaset participants visible from the client. And that we are connected to the master. While the full MongoDB configuration was as follows: NOTE: With this we can basically consider this discussion here: https://its.cern.ch/jira/browse/CMSKUBERNETES-175 to be resolved. The few minor details about creating the proper username for FYI: @vkuznet @amaltaro @muhammadimranfarooqi @arooshap @goughes |
There was a problem hiding this comment.
Todor, I added a few questions and comments along the code, some for my own education.
I just wanted to highlight that the following is still pending (before the final review can be done):
- fix the failing unit tests
- create all the required configuration changes
- make sure the new secrets are also properly created and in place.
|
Just to put some more details on:
Here is what the current production MongoDB server ports setup looks like: So a fully defined relicaset with a list of Simply, times out: While, having it configured with just a single entry point and a port pointing to the defaults of the DNS loadbalancer which sitting in front of the replicaset, works just fine: If we need to have it all defined properly, we need to point directly to the pods like: Which is not optimal. We'd like to have this single server name of TODO: |
|
Jenkins results:
|
|
Jenkins results:
|
|
@todor-ivanov , I'm not sure I'm following what you are suggesting. Here is how I see the issue from another end (the point of k8s infrastructure): and you can eaily look-up node IP addresses. The Its IP address points to external IP within k8s cluster when I look at services: Now, with this information I conclude:
Now, if you want full access to individual replica set nodes, then we need to have a different setup:
But of course such redesign has its pros/cons (especially I doubt CERN network team will be happy about firewall requests). Said that, I'm trying to understand the actual issue here and why DMWM can't use provided load balancer. |
|
Thanks @vkuznet for looking into this.
This is exactly what I was referring to... I am quite aware what the reason of the timeout is - there is indeed the loadbalancer in front of the pods. I, on the other hand, am not aware of the specific K8 setup of this cluster. Back in the time IIRC it was because of our request to have a single name as an entry point that the loadbalancer was put inforont of the cluster. Now we can indeed connect to the pods directly - they are visible (I do not know how) from inside the CERN network. Of course this is not optimal..... What we would need is a simple connection name/s of the replicaset members to connect to (excluding the hash from the pods' names). Now, how exactly (from K8 perspective) the solution would best be, I am not the right person to tell. But we already know one thing from past experience: Having one dynamic component chosing a service/pod to serve the connection (from K8) and then another one chossing the repicaset member (from MongoDB) to connect to is still working, but we do experience from time to time exceptions of the kind |
|
@todor-ivanov please clearly outline requirements as I cannot guess them. For instance:
Based on requirements we may need to setup k8s infrastructure. Then, we can discuss implementation details outlined in this PR. Sorry, but so far I see no need to review PR without clear idea how cluster should be set and what should be exposed. Said that, it does not mean we can do everything. For instance, if we need to open firewall to each minion on specific port I can't guarantee that such request can be granted by CERN security team. Anything related to firewall should be audited first, etc. And I would like to avoid doing unnecessary steps if later we'll decide not to use specific setup. Therefore, we need clear set of requirements and discuss them first if we can provide setup to match them. |
|
Hi @vkuznet, I do not understand why are we deviating in discussing firewall rules etc.
No, this is not supposed to be seen from outside CERN. We do not need to deal with firewall rules or what so ever here. This PR is consistent. It does not put any requirement on how the K8 setup is already or will be done. With this PR I only give the ability of our code to utilize all possible connection setups that the standard MongoClient supports. The whole problem in the past was that we were not setting up the members of the replicaset explicitly and we were relying on some external component (in this case a piece from K8) to give us the ability to use a single name as an entry point.
I do not understand what you mean here. The only thing that I was hoping we may get at some point is to avoid using names for connecting to database servers containing some random "hash-like" strings in them - like Again, this PR does not require any change in the Kubernetes setup. I have already tested multiple times both |
|
Actually, I can see what may have lead to a confusion here. Maybe this comment: #11360 (comment) may have left people with the wrong impression that we MUST change the production cluster. No, we do not need to change anything. We can connect to the single server name as we were doing before or to an explicitly listed members of the replicaset. I did put that many details in this previous comment only to point out the difference of the two setups we have in production and in testbed. And also to justify my note that if we want to be backwards compatible with the current change we do need to mind this difference. |
|
Jenkins results:
|
|
Jenkins results:
|
Todor, in light of the points made by Valentin, I think I don't understand what the goal is here either. Right now, this is what we use in the production system: so I failed to see anywhere where we need to connect - or to pass in - a specific pod url (which includes the pod hash).
can you please clarify where we see this problem? Which service? In which scenario? |
|
Jenkins results:
|
todor-ivanov
force-pushed
the
feature_MongoDBAAS_validateTestClusters_fix-11210
branch
from
3bd7bc2 to
0025ac2
Compare
|
Jenkins results:
|
|
From the chat that Todor and I had yesterday, my understanding of this development is:
Ideally, we should have the same architecture/setup for both testbed and production MongoDB clusters. I updated the JIRA ticket on that regard. In addition to that, I understand that MongoDB replicaset works as the following:
I confess that I need to educate myself on this. So far, my understanding is mostly with discussions here and there and I trust on your investigation to make the best choices here. Please let me know if I got anything wrong and/or missed anything. |
|
Thanks @amaltaro. Your comment here does reflect the situation quite accurate. Just to add some official documentation from MongoDB explaining and visualizing that exact bit about the Again I need to mention, we are capable of supporting both cases - with an external loadbalancer providing a single entry point to connect to and without it - defining separately all replicaset members in the connection string. And it is up to us to chose which one we would like to have (preferable the same in both production and testbed), but this PR does not put a strong requirement that we must change things in K8 now. This may easely be followed with a separate GH issue. p.s. Just a note here: Further recommendations for best HA setup and data redundancy from the MongoDB documentation suggest to have the different members of the replicaset existing on physically separate nodes. [1] |
|
thanks @vkuznet |
There was a problem hiding this comment.
Todor, these changes are looking good to me. However, I left a few comments that might actually need to be considered for this PR.
I also have a question concerning the services configuration, I see you have updated the mongoDBReplicaset name. Does it require any synchronization between WMCore deployment and changes to the MongoDB cluster?
| try: | ||
| if mockMongoDB: | ||
| self.client = mongomock.MongoClient() | ||
| self.logger.info("NOTICE: MongoDB is set to use mongomock, instead of real database.") | ||
| elif replicaset: | ||
| self.client = MongoClient(self.server, self.port, replicaset=replicaset, **kwargs ) | ||
| self.client = MongoClient(host=self.server, port=self.port, replicaset=replicaset, **kwargs ) |
There was a problem hiding this comment.
Different comment on this same line. It looks like there is no need for this elif-else block for the replicaset and we could be passing None value in case it's not defined, which is the default value.
There was a problem hiding this comment.
That I believe is a leftover from when we were trying to build the full connection string for the server Url ourselves and not splitting all the parameters to be passed to the client separately. Now this is not needed, indeed.
Something more, I now notice that the client parameter from the documentation is actually replicaSet not replicaset, but I did check and it was properly recognized even with the typo in it. If I change it to a non existent replicaSet it does timeout [1]. And it works because obviously all the client parameters get lower-cased at runtime: [2]. I am fixing it anyway because I'd like to have all the variables following our proper naming convention. (here and in all the relevant config files as well).
[1]
$ ipython -i -- /data/tmp/WMCore.venv3/srv/WMCore/bin/adhoc-scripts/mongoInit.py -c /data/tmp/WMCore.venv3/srv/current/config/reqmgr2ms-output/config-output.py
2022-11-15 10:50:06,353:INFO:mongoInit:<module>(): Connecting to MongoDB using the following mongoDBConfig:
{'connect': True,
'create': False,
'database': 'msOutputDBPreProd',
'directConnection': False,
'logger': <Logger __main__ (INFO)>,
'mockMongoDB': False,
'password': '****',
'port': None,
'replicaset': 'cmsweb-dev',
'server': ['mongodb-test.cern.ch:32001',
'mongodb-test.cern.ch:32002',
'mongodb-test.cern.ch:32003'],
'username': '****'}
...
ServerSelectionTimeoutError: No replica set members available for replica set name "cmsweb-dev", Timeout: 30s, Topology Description: <TopologyDescription id: 637360cef3d1bc38070ce01b, topology_type: ReplicaSetNoPrimary, servers: []>
[2]
$ ipython -i -- /data/tmp/WMCore.venv3/srv/WMCore/bin/adhoc-scripts/mongoInit.py -c /data/tmp/WMCore.venv3/srv/current/config/reqmgr2ms-output/config-output.py
2022-11-15 10:53:25,406:INFO:mongoInit:<module>(): Connecting to MongoDB using the following mongoDBConfig:
{'connect': True,
'create': False,
'database': 'msOutputDBPreProd',
'directConnection': False,
'logger': <Logger __main__ (INFO)>,
'mockMongoDB': False,
'password': '****',
'port': None,
'replicaSet': 'cmsweb-test',
'server': ['mongodb-test.cern.ch:32001',
'mongodb-test.cern.ch:32002',
'mongodb-test.cern.ch:32003'],
'username': '****'}
In [1]: mongoClt
Out[1]: MongoClient(host=['mongodb-test.cern.ch:32002', 'mongodb-test.cern.ch:32001', 'mongodb-test.cern.ch:32003'], document_class=dict, tz_aware=False, connect=True, replicaset='cmsweb-test', directconnection=False)
In [2]: mongoClt.topology_description.replica_set_name
Out[2]: 'cmsweb-test'
|
Jenkins results:
|
|
test this please |
|
Jenkins results:
|
There was a problem hiding this comment.
Todor, your latest changes triggered further review, please find them along the code.
I also repeat my question from a previous review: "I also have a question concerning the services configuration, I see you have updated the mongoDBReplicaset name. Does it require any synchronization between WMCore deployment and changes to the MongoDB cluster?"
| msg += "Skipping the current run." | ||
| self.logger.error(msg) | ||
| return summary | ||
| # if not protectedLFNsT0: |
There was a problem hiding this comment.
If you want to touch this code, I would then suggest to remove it completely (and the same applies to the service configuration). Otherwise, just leave it as it was before. There is no need to fix all the pylint messages, especially when it's about code that you did not touch within the PR.
There was a problem hiding this comment.
That's fine with me. The less I change in this PR the cleaner it looks at the end. I am all for leaving it as it was before.
|
Jenkins results:
|
|
Thanks @amaltaro!
I missed to answer that question indeed. Sorry for that. No, the parameter name and the MongoDBAsAService cluster configuration have nothing to do with each other. It is only about following our proper naming practices. I did reflect the new variable name in all relevant |
Fix some config variable names && Print out a blured configuration dictionry for mongoInit.py Fix missing ex var && Bad collection assignment from mongoDB object. Fixing replicaSet variable name. Document some more parameters in MongoDB docstring && Revert a commented code blocks in MSUnmerged.
Unit tests fixes - pylint warnings. Pylint fixes.
todor-ivanov
force-pushed
the
feature_MongoDBAAS_validateTestClusters_fix-11210
branch
from
bf0495f to
c3fc065
Compare
|
Jenkins results:
|
Fixes #11210
Status
Ready
Description
With the MongoDBAsAService configuration that we were having so far in production, for the sake of simplicity and stability (not having volatile names of a set of pods to connect to), we were relying on a DNS loadbalancer setup in front of the pods serving the database. That way we were having a single entry point. While being quite comfortable for us, this was causing some additional work and effort for CMSWEB team and we were also having some retried writes when we were to stumble on the situation of not having a session with the replicaset master. This was due to the fact that the loadbalancer in front of the replicaset was choosing randomly the hostname from the backends to connect to, rather than letting the full client/server communication to take place and have the proper discovery of the replica master among all the participating pods in the replicaset. This is easily avoidable if we have a hostname alias set in the pod's setup itslef (as it is now for the test MongoDBAsAService cluster) and have the pods serving the replicaset always been named as e.g.
mongodb-test.cern.ch. In that case on our end we may have the mongo client pointing not to a fully constructed and "percent-escaped"MongoDB URIof the kind:with a single port, but rather having all
hostname:portstuples listed in the connection configuration and avoiding anyurllibescaping for thepasswordandusernamestrings. This makes things much more reliable but was requiring the code changes from this PR on our end.Is it backward compatible (if not, which system it affects?)
YES
BUT strongly depends on the
services_configchanges in theprodbranch and the production MongoDB cluster setup.Related PRs
Her to be added the relative
services_configPRs:https://gitlab.cern.ch/cmsweb-k8s/services_config/-/merge_requests/171
https://gitlab.cern.ch/cmsweb-k8s/services_config/-/merge_requests/173
https://gitlab.cern.ch/cmsweb-k8s/services_config/-/merge_requests/172
NOTE:
Similarly, we also need the same changes for MSUnmerged configuration and for all branches
prod,preprod,testExternal dependencies / deployment changes
NOTE:
We need to test if this connection is properly working with the current production cluster of MongoDBAsAService setup such that we can connect to the cluster skipping the DNS loadbalancer and avoiding any additional change in this setup.