Skip to content

Conversation

@jryb
Copy link
Contributor

@jryb jryb commented Dec 16, 2020

Problem

As part of #126 it mentions an optimization for installing apps on a SHC since there is not separate CRD for the deployer.

On SearchHeadClusters CRDs, the defaultsUrl property of the CRD is shared by the deployer and the search-heads. When the defaultsUrl is updated with new applications, this would trigger a rolling rebuild of the search-head pods and deployer. However the deployer would trigger an apply shcluster-bundle, potentially triggering a rolling restart of the searchhead cluster in parallel. Rebuilding the SH pods is not necessary and can lengthen the downtime of the the SHC.

This problem does not exist on the IndexerCluster CRD since the applications are handled by the cluster-master which has a dedicated CRD. So the defaultsUrl from indexers in the cluster can be different. If a separate CRD is implement for the SHC Deployer, this would be the same case for SHC as well. However until that CRD is created, this problem exists.

Solution

Add a defaultsUrlApps parameter to the CRDs for Standalone, IndexerClusters, and SearchHeadClusters. This parameter would be used specifically for app installs and used on Standalones, SH Deployer, and IDC CM only. Individual SHs and indexers in clusters would ignore this parameter. This would allow the applications for a clustered env to be changed without rebuilding the SH member pods. The SH Deployer would then push out the bundle and install the apps on all SH members.

This could have been implement specific to SH Deployer (since the SHC is the only deployment with this issue), however keeping it more generic allows for future optimizations, potentially installing apps without the need pod rebuilds.

Testing

Create a SHC using a defaults.yml in defaultsUrlApps:

shc.yml:

apiVersion: enterprise.splunk.com/v1beta1
kind: SearchHeadCluster
metadata:
  name: shc1
  finalizers:
  - enterprise.splunk.com/delete-pvc
spec:
  clusterMasterRef:
    name: cm1
  volumes:
    - name: apps
      configMap:
        name: splunk-apps
  defaultsUrlApps: /mnt/apps/defaults_apps.yml
NAME                                  READY   STATUS    RESTARTS   AGE
splunk-cm1-cluster-master-0           1/1     Running   0          23m
splunk-default-monitoring-console-0   1/1     Running   0          17m
splunk-lm1-license-master-0           1/1     Running   0          23m
splunk-operator-7d868fb689-mqtrb      1/1     Running   0          27m
splunk-shc1-deployer-0                1/1     Running   0          22m
splunk-shc1-search-head-0             1/1     Running   0          22m
splunk-shc1-search-head-1             1/1     Running   0          22m
splunk-shc1-search-head-2             1/1     Running   0          22m

Then adjust the defaultsUrlApps parameter and verify that only the shc deployer gets rebuilt and that the correct apps from the respective defaults_apps.yml are installed.

  defaultsUrlApps: /mnt/apps/defaults_apps2.yml

Apply changes and validate only deployer is rebuilt:

NAME                                  READY   STATUS    RESTARTS   AGE
splunk-cm1-cluster-master-0           1/1     Running   0          36m
splunk-default-monitoring-console-0   1/1     Running   0          31m
splunk-lm1-license-master-0           1/1     Running   0          36m
splunk-operator-7d868fb689-mqtrb      1/1     Running   0          41m
splunk-shc1-deployer-0                1/1     Running   0          100s
splunk-shc1-search-head-0             1/1     Running   0          36m
splunk-shc1-search-head-1             1/1     Running   0          36m
splunk-shc1-search-head-2             1/1     Running   0          36m

@jryb jryb force-pushed the bugfix/CSPL-663_defaultsUrlDeployer branch from f13c1e9 to 12393d6 Compare December 16, 2020 22:29

// prepare defaults variable
splunkDefaults := "/mnt/splunk-secrets/default.yml"
// Check for apps defaults and add it to only the standalone or deployer/cm instances
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I have understood this right, to install apps on Standalones, CM & Deployer, setup defaultsUrlApps

  • Am I right in saying that on LM, we still need to use defaultsUrl? For consistency purposes, should LM also be included?
  • If both defaultsUrlApps & defaultsUrl are configured with apps_locaton, wonder how Ansible would treat this? I am guessing it would be much more efforts to expect operator to be smart enough to parse & detect this duplicate, & strip off one of the duplicates. Completely dropping apps_location from defaultsUrl would also need parsing logic.
  • We should document the new argument & best practices in Examples.md

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I right in saying that on LM, we still need to use defaultsUrl? For consistency purposes, should LM also be included?

So currently yes, but I can add LM to this list as well. Or probably better, just exclude Indexer and SearchHead types since they will always be in the cluster.

If both defaultsUrlApps & defaultsUrl are configured with apps_locaton, wonder how Ansible would treat this? I am guessing it would be much more efforts to expect operator to be smart enough to parse & detect this duplicate, & strip off one of the duplicates. Completely dropping apps_location from defaultsUrl would also need parsing logic.

I will test this and verifiy that ansible behaves correctly.

We should document the new argument & best practices in Examples.md

We can add this to the documentation. Though if we plan on making the Deployer its own CRD, we may not need this parameter. So it maybe a temporary fix and might not need documenting if it's going away soon. However we can pull the docs once that occurs. I'll make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated in testing the use of multiple defaults.yml files works correctly, including with overlapping apps.

docs/Examples.md Outdated

Unlike `defaultsUrl` which is applied at every instance created by the CR, the
`defaultsUrlApps` will be applied on instances that will **not** get the application
installed via a bundle push. earch head and indexer cluster members will not have
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Typo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

docs/Examples.md Outdated
the `defaultsUrlApps` parameter applied. This means:

- For standalones the apps will be installed as normal.
- For SHC these applications will be installed in the SHC Deployer and push it out
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Just so that both statements are consistent

  • For SHC, these applications will only be installed on the SHC Deployer and pushed to the members via SH Bundle push.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

docs/Examples.md Outdated
installed via a bundle push. earch head and indexer cluster members will not have
the `defaultsUrlApps` parameter applied. This means:

- For standalones the apps will be installed as normal.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: For standalone & License Master, the apps will be installed as normal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

docs/Examples.md Outdated
pushed to the indexers in the cluster via CM Bundle push.

Application and other ansible defaults can be still be installed via `defaultsUrl`
and both options can be used in conjunction:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Installing Applications via defaultsUrl, while continues to work, is not going to be a recommended practice anymore, right? Especially to avoid what this PR is fixing (the double pod resets).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes while technically you can still have apps in the defaultsUrl this would not be preferred but would be left in for backward compatibility. The new block would be:

For application installation the preferred method will be through the defaultsUrlApps
while other ansible defaults can be still be installed via defaultsUrl. For backwards
compatibility applications could be installed via defaultsUrl though this is not
recommended. Both options can be used in conjunction:

Copy link
Contributor

@kashok-splunk kashok-splunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good :)

@smohan-splunk smohan-splunk self-requested a review December 18, 2020 23:11
@jryb jryb force-pushed the bugfix/CSPL-663_defaultsUrlDeployer branch from 5ff389b to 85e3a16 Compare December 23, 2020 18:25
@jryb
Copy link
Contributor Author

jryb commented Dec 23, 2020

Squashing commits

Add a new parameter defaultsUrlApps to install apps listed here only on Standalone,
SHC Deployer, and IDC CM.  This parameter would be ignored on individual SHs and indexers
in a clustered environment as the install of app there would be handled by bundle push
from the deployer or CM.
@jryb jryb force-pushed the bugfix/CSPL-663_defaultsUrlDeployer branch from 17aa7a1 to ab35bb7 Compare December 23, 2020 20:10
@smohan-splunk smohan-splunk merged commit 33db9ad into develop Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants