patch(DPE-10361): Charm stabilization#354
Conversation
Gu1nness
changed the title
Gu1nness
force-pushed
the
DPE-10361-mongo-db-charm-stabilization-implementation
branch
from
527d885 to
74df9c1
Compare
…10361-mongo-db-charm-stabilization-implementation
Gu1nness
force-pushed
the
DPE-10361-mongo-db-charm-stabilization-implementation
branch
from
aba580c to
2464508
Compare
…abilization-implementation
| config-server to add the shard to the cluster. | ||
| """ | ||
| # By setting the status we ensure that the former statuses of this component are removed. | ||
| self.state.statuses.set(ShardStatuses.ACTIVE_IDLE.value, scope="unit", component=self.name) |
There was a problem hiding this comment.
shouldn't this be in the reconcile_shard_after_restart?
There was a problem hiding this comment.
No because that would not cover the case where the restart is triggered by TLS manager (which is not this callback but the other one.
Overall this is more a final cleanup than anything (after everything is integrated, let's go back to the proper status.
I'm not happy about this one however, maybe we should find a better way to do taht.
|
|
||
| def _handle_changed_secrets(self, event: SecretChangedEvent): | ||
| """SecretChanged event handler, which is used to propagate the updated passwords.""" | ||
| try: |
There was a problem hiding this comment.
PBM restart also raises a WorkloadServiceError which is not handled here
| self.update_pbm_certificate_in_trust_store() | ||
| if self.update_pbm_certificate_in_trust_store(): | ||
| try: | ||
| # after updating the password of the backup user, restart pbm with correct password |
There was a problem hiding this comment.
the restart after the backup user changed happens in sync_cluster_passwords, this one happens if the PBM certs changed.
There was a problem hiding this comment.
Also, would it be worth it to find a way to just restart PBM once here?
There was a problem hiding this comment.
It's probably worth it, but I'm careful: if we remove the automated restart from the synchronize cluster passwords, then it means that everytime we use that method, we have to take care to restart afterwards.
TBH I think this is probably the safest solution.
the restart after the backup user changed happens in WorkloadServiceError
I think you mean "ends up in WorkloadServiceError" ?
There was a problem hiding this comment.
I made a mistake in what I wrote. I already corrected it.
I meant that the comment "after updating the password of the backup user, restart pbm with correct password" is not correct. Here we are restarting because the PBM certs changed, not because of the password update
| except MissingCredentialsError: | ||
| return False | ||
|
|
||
| def mongodb_config_for_user( |
There was a problem hiding this comment.
I do not see any use of mongodb_config_for_user that includes tls_external_ca
There was a problem hiding this comment.
Not yet (and hopefully won't happen, but I'd rather keep it for consistency (have the same interface as mongos_config_for_user)
patriciareinoso
left a comment
patriciareinoso
left a comment
There was a problem hiding this comment.
Thank you Neha!
just a couple of nits
|
I went trough it again the only thing that seems misplaced to me is the active status in the handle_pbm function which you already explained. Also I would rather not add an unused parameter to Other than that all good Also manual testing toogling TLS went really smooth. Last week I was not able to enable client TLS without seing an error. So thank you |
|
@patriciareinoso You're right for the active status. |
2 participants
🏷️ Type of changes
📝 Description
This PR is mostly refactoring and cleanup.
🧪 Manual testing steps
🔬 Automated testing steps
✅ Checklist