-
Notifications
You must be signed in to change notification settings - Fork 124
CSPL-592 Trigger app install on defaultsURL change #199
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
Conversation
6bf02d5
to
7eb7436
Compare
Thanks a lot for the PR! Just commenting to cross reference as this partially addresses issue #126 |
Only for awareness... I don't mean to disturb the merge of this PR which is really needed on our side: |
@romain-bellanger Thank you for the thorough review & testing of the changes so far. We will create an internal bug for the MC issues with Multisite clusters. Just FYI - in the future, we plan to remove MC's dependency on Ansible. |
Hi @romain-bellanger This should not affect MC with the multi-site cluster as we are never using DefaultsURL to provide multisite_master info to MC. On creation or update of MC, we enquire CM and check if it has multisite configuration set to true then we pass the parameters required to MC through its (MC's) configmap https://github.com/splunk/splunk-operator/blob/develop/pkg/splunk/enterprise/monitoringconsole.go#L82 cc @smohan-splunk |
I verified this with a multisite cluster and the MC comes up correctly:
|
Hi, @kashok-splunk I trust you about the ConfigMap which I think is rather used for the configuration of the MC app. The impact I was speaking about is rather related to the clustering configuration of the monitoring console as a searchhead. I've done many tests around this, and I could see for instance MC failing to start at the multisite clustering step when inheriting its @jryb after seeing the result of your test, I tried the same. I also observed the pod starting and reported ready in kubernetes. However looking at splunkd logs, I had streaming errors:
And Splunk was reporting red state on Search Head Connectivity (here from health.log, but same visible from UI):
Looking at the
And then with the commit from this PR preventing the inheritance of
Additional impact noticed during the test: We have requirements for full encryption in public cloud, especially for connections such as DMC UI through which cluster admin credentials are sent. So we use the I'm sorry if these comments are causing some disruption to the merge of this PR, that's really not the goal. I just mean to share information about possible impacts, in case other people would experience them, and about gaps to fix later around the MC. We very much need the first commit of this PR and are thankful for this change! And we don't have any good proposal to avoid such impacts, as discussed the only way would probably be to create a dedicated CRD for MC. On our side, we patch the operator to have the MC StatefulSet creation only triggered by SHC so that it has stable specs with correct parameters, but that's not suitable for all users. |
When applying an updated CRD, check the environment variables and push those changes to the running pods.
Thanks for the comments and diligent testing @romain-bellanger. It sound like blocking the
I tried this out on a deployment with multiple standalones with differing |
b4e83ff
to
7642f9f
Compare
Thanks a lot @jryb! I agree it's probably the best solution in the current situation. |
Problem
When applying an updated CRD, check the environment variables and push those changes to the running pods. For example, if the
defaultsUrl
parameter is changed in the CRD yaml and applied; the change is accepted however it is not reflected in the stateful set or the pod itself:Edit standalone6.yml:
Reapply:
History
This change was actually put in place and removed during the development of the monitor console integration for separate reasons ( 74abe92#diff-f051fa8775a356c08c90ac411f88b50e7455ec17e8f3241a810b64eb7f00a747 ). It was removed because it was no longer needed for the MC ( 2ca799f ), however it is still needed for app installation via defaults.yaml.
Solution
When merging the changes from the pod spec, check the environment variables. If they have changed, mark the specs as differing.
Testing
Validated that the
defaultsUrl
parameter is propagated to the pods using the method described above. Validate that the stateful set parameters are updated and the correct apps are installed when a newdefaults.yaml
is applied.