-
Notifications
You must be signed in to change notification settings - Fork 123
CSPL-1301: Trigger app install for modified app pkgs #503
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
While an app package modification does trigger rewritteing the configmap. There are cases where this rewrite will result in the same data section as the previous version of the configmap (for example, when a single app is installed initially then modified.) When this is applied no change is detected and the Pod does not reset or install the new app. By adding a label to the configmap metadata, when an app package change is detected, we can increment this label. This will not affect the data in the configmap since the label is in the metadata section, however the label change will force a new ResourceVersion of the configmap and restart the pod, triggering the modified app install.
pkg/splunk/controller/configmap.go
Outdated
if forceUpdate { | ||
// Add or Increment the revision label to force a new ResourceVersion for the ConfigMap | ||
labels := current.GetLabels() | ||
if labels == nil { | ||
newLabels := make(map[string]string) | ||
newLabels["revision"] = "1" | ||
current.SetLabels(newLabels) | ||
} else if val, ok := labels["revision"]; ok { | ||
revision, _ := strconv.Atoi(val) | ||
labels["revision"] = strconv.Itoa(revision + 1) | ||
current.SetLabels(labels) | ||
} else { | ||
labels["revision"] = "1" | ||
current.SetLabels(labels) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be wrong, but AFAIK, doing a UpdateResource()
always gives us a new revision. In that case, we don't need the extra labels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so too but thats not the case. If you call UpdateResource()
with the same info, it doesn't update the resourceVersion (which is opaque to the operator).
jryb:AppFramework$ k get configmap/splunk-demo-s1-standalone-app-list -o=yaml
apiVersion: v1
data:
app-list-local.yaml: |-
splunk:
app_paths_install:
default:
- "/init-apps/admin-apps/demo_my_dashboards.spl"
kind: ConfigMap
metadata:
creationTimestamp: "2021-09-08T22:02:47Z"
name: splunk-demo-s1-standalone-app-list
namespace: default
ownerReferences:
- apiVersion: enterprise.splunk.com/v2
controller: true
kind: Standalone
name: demo-s1
uid: fa455462-2046-46a1-aeb3-70db12fa03a7
resourceVersion: "68382724"
selfLink: /api/v1/namespaces/default/configmaps/splunk-demo-s1-standalone-app-list
uid: 21261b76-6e96-4e19-aaa9-8adbab9a9b6b
jryb:AppFramework$ ##### UPDATE S3
{"level":"info","ts":1631139348.5761461,"logger":"splunk.enterprise.handleAppRepoChanges","msg":"received App listing","kind":"Standalone","name":"demo-s1","namespace":"default","for App sources":1}
{"level":"info","ts":1631139348.5761857,"logger":"splunk.enterprise.AddOrUpdateAppSrcDeploymentInfoList","msg":"App change detected. Marking for an update.","Called with length: ":1,"appName":"demo_my_dashboards.spl"}
{"level":"info","ts":1631139348.5796883,"logger":"splunk.reconcile.ApplyConfigMap","msg":"Updating existing ConfigMap","name":"splunk-demo-s1-standalone-app-list","namespace":"default","forceUpdate":true,"ResourceVerison":"68382724"}
{"level":"info","ts":1631139348.584546,"logger":"splunk.reconcile.UpdateResource","msg":"Updated resource","name":"splunk-demo-s1-standalone-app-list","namespace":"default"}
jryb:AppFramework$ k get configmap/splunk-demo-s1-standalone-app-list -o=yaml
apiVersion: v1
data:
app-list-local.yaml: |-
splunk:
app_paths_install:
default:
- "/init-apps/admin-apps/demo_my_dashboards.spl"
kind: ConfigMap
metadata:
creationTimestamp: "2021-09-08T22:02:47Z"
name: splunk-demo-s1-standalone-app-list
namespace: default
ownerReferences:
- apiVersion: enterprise.splunk.com/v2
controller: true
kind: Standalone
name: demo-s1
uid: fa455462-2046-46a1-aeb3-70db12fa03a7
resourceVersion: "68382724"
selfLink: /api/v1/namespaces/default/configmaps/splunk-demo-s1-standalone-app-list
uid: 21261b76-6e96-4e19-aaa9-8adbab9a9b6b
pkg/splunk/enterprise/util.go
Outdated
if len(appListingConfigMap.Data) > 0 || appsModified { | ||
configMapDataChanged, err = splctrl.ApplyConfigMap(client, appListingConfigMap, appsModified) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is a unique situation for the Phase-23 app framework, i feel it m
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya this is only applicable for as long as we use the configmap method. But its needed for the case were the set of applications that has been modified is the same as the previous change (i.e. no change in the configmap data section).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, seems like I messed up my comment. What I wanted to mention was, since it is a unique case just for phase-2, we can simply delete the existing configMap and then write a new one, so we don't need to change the ApplyConfigMap API. Probably, we need to have a new API for deleting the configMap.
Changing the conf rev. was a kind of workaround we are using to reset the Pod, otherwise, we never see a use case for the force update to happen the configMap for the same content. If somebody is using the force option, they should make sure that the code path that gets there is not re-entrant across the reconciles, otherwise it may keep updating unnecessarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, either way is fine. They both require an additional API or API parameter that we can eventually remove once we no longer need the resets. Only app framework will use the force
parameter, which does not rerun this code every reconcile due to the appContext status. I would suspect that even if we created a new ConfigMap it would also have to be non-reentrant or the same issue would occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, having the delete API is a better option for multiple reasons:
- We are getting a new API :-).
- We don't need to change the API behavior that is not something for a standard use case.
- We don't need to address it in several places, which we need to revert again after some time.
Otherwise, I am fine with either option :-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, looking back at it, even the DELETE and CREATE calls are expensive, and we can avoid it.
All that we need is, just one more UPDATE call. i.e calling splctrl.ApplyConfigMap
once with an empty data map. That should solve our unique situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya I agree that a delete and readd is likely much more expensive than a modify. So I can basically add a reset of the config prior to the set. This means if we have any app updates and the appListing changes, we would now update the configmap twice: once setting the Data
section to "" and a second time to set it to the actual appListing
as opposed to the original fix that only updated the configmap once. This way there are no changes to the ApplyConfigMap()
API and this is completely isolated to the AppFramework code.
Since the two updates are in quick secession, there shouldn't be any adverse side effects. I'm testing it out to verify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way we can avoid extra update calls is by doing a deep equal comparison on the existing configMap data. However, that needs a READ CRUD call. So, I guess, we are fine with this approach, considering we are patching a unique use case. :-)
Instead of using a label in the ConfigMap metadata, reset the data in the app listing ConfigMap to nothing prior to setting it, forcing a new resourceVerison.
Verification details in CSPL-1312, 1.0.3 operator build with fix behaviour is as expected. |
* move m4 test to integration to clear smoke run (#487) * Feature circleci migration (#490) * Migration of Unit Test and Smoke Test pipelines from CircleCi to Github Actions (#441) * Update cron schedule * Fix a bug where standalone with replicas>1 won't come up (#489) * Added int test workflow and nightly workflows. (#493) * Modify monitoring console selection name to avoid eks cluster creation failure (#494) * Update the name of int test step (#496) * CSPL-1219 (#470) * [CSPL-1283] Fix AWS & minio S3 client code to support App framework on GCS (#498) * Fix minio S3 client code & incorrect minio initContainer ut Add changes to minio client code to handle generic S3 compatable remote stores, namely GCS. * CSPL-1301: Trigger app install for modified app pkgs (#503) * Trigger app install for modified app pkgs While an app package modification does trigger rewritteing the configmap. There are cases where this rewrite will result in the same data section as the previous version of the configmap (for example, when a single app is installed initially then modified.) When this is applied no change is detected and the Pod does not reset or install the new app. By adding a label to the configmap metadata, when an app package change is detected, we can increment this label. This will not affect the data in the configmap since the label is in the metadata section, however the label change will force a new ResourceVersion of the configmap and restart the pod, triggering the modified app install. [UPDATE] Instead of using a label in the ConfigMap metadata, reset the data in the app listing ConfigMap to nothing prior to setting it, forcing a new resourceVerison. * CSPL-1302: Bias-language removal Phase 1 [Comments & Docs] (#497) * Bias-language removal Phase 1 * CSPL-1316 : Avoid app framework flow from re-entrancy (#506) * Automated pre release workflow (#508) * CSPL-1230 Remove need for Secret keys in IAM env (#505) * CSPL-1230 Remove need for Secret keys in IAM env The operator code can be changed to allow no secretRef to be a valid volume config. If no secretRef is configured for a SmartStore or AppFramework volume, then assume the credentials are available on the env itself, possibly through a tool such as kube2iam. * Moved action file to right location (#510) * Update the URL on relese RULE (#512) * CSPL-1339: Fix make_bundle.sh to check for version change (#517) * Splunk Operator 1.0.3 release (#511) * Cspl 1335 official release workflow (#523) * Automated Release Workflow * Added automated workflow to merge develop to master * Fix typo in kubectl create secret command (#524) * Update README.md (#526) * Fix image push jobs from master branch (#530) * Fixing merge issues * CSPL-1298 (#521) * CSPL-1307: Bias-language removal Phase 1 (#504) Implement Github Actions workflow for Bias Language * Update spunk-operator image tag in release directory (#535) * CSPL-1327 App framework: App installation isn't triggered if 1 appsource is empty. (#519) * CSPL-1327 App install isn't triggered if 1 appsource is empty An empty `appSource` is not necessarily an error. Treat it as a no-op and throw a log message. * Adding fix for CSPL-1316 for MC * Update test case to accomodate Standalone Updating Phase * Merge issues * Fix Automation test case * Fix automation test * CSPL-1305 - Remove bias language from Paths & URLs (#509) * Bias-Lang Removal P1 - Handling Exceptions Consolidate Paths & URLs from external sources Include exceptions to GithubActions workflow * Fix URLs
* Merge master1.0.1 (#370) 1.0.1 Release * release/1.0.2 (#473) * release/1.0.2 - Addressing doc references to release 1.0.2 * - doc change * - Fixing the olm-catalog CRDs for the versions 1.0.0-RC, 1.0.0, and 1.0.1 (#475) * Fix role yaml * CSPL:1217 Added new cases for app version downgrade (#452) Co-authored-by: Sirish Mohan <68884189+smohan-splunk@users.noreply.github.com> * move m4 test to integration to clear smoke run (#487) * Feature circleci migration (#490) * Migration of Unit Test and Smoke Test pipelines from CircleCi to Github Actions (#441) * Update cron schedule * Fix a bug where standalone with replicas>1 won't come up (#489) * Added int test workflow and nightly workflows. (#493) * Modify monitoring console selection name to avoid eks cluster creation failure (#494) * Update the name of int test step (#496) * CSPL-1219 (#470) * [CSPL-1283] Fix AWS & minio S3 client code to support App framework on GCS (#498) * Fix minio S3 client code & incorrect minio initContainer ut Add changes to minio client code to handle generic S3 compatable remote stores, namely GCS. * CSPL-1301: Trigger app install for modified app pkgs (#503) * Trigger app install for modified app pkgs While an app package modification does trigger rewritteing the configmap. There are cases where this rewrite will result in the same data section as the previous version of the configmap (for example, when a single app is installed initially then modified.) When this is applied no change is detected and the Pod does not reset or install the new app. By adding a label to the configmap metadata, when an app package change is detected, we can increment this label. This will not affect the data in the configmap since the label is in the metadata section, however the label change will force a new ResourceVersion of the configmap and restart the pod, triggering the modified app install. [UPDATE] Instead of using a label in the ConfigMap metadata, reset the data in the app listing ConfigMap to nothing prior to setting it, forcing a new resourceVerison. * CSPL-1302: Bias-language removal Phase 1 [Comments & Docs] (#497) * Bias-language removal Phase 1 * CSPL-1316 : Avoid app framework flow from re-entrancy (#506) * Automated pre release workflow (#508) * CSPL-1230 Remove need for Secret keys in IAM env (#505) * CSPL-1230 Remove need for Secret keys in IAM env The operator code can be changed to allow no secretRef to be a valid volume config. If no secretRef is configured for a SmartStore or AppFramework volume, then assume the credentials are available on the env itself, possibly through a tool such as kube2iam. * Moved action file to right location (#510) * Update the URL on relese RULE (#512) * CSPL-1339: Fix make_bundle.sh to check for version change (#517) * Splunk Operator 1.0.3 release (#511) * Cspl 1335 official release workflow (#523) * Automated Release Workflow * Added automated workflow to merge develop to master * Fix typo in kubectl create secret command (#524) * Update README.md (#526) * Fix image push jobs from master branch (#530) * CSPL-1298 (#521) * CSPL-1307: Bias-language removal Phase 1 (#504) Implement Github Actions workflow for Bias Language * Update spunk-operator image tag in release directory (#535) * CSPL-1327 App framework: App installation isn't triggered if 1 appsource is empty. (#519) * CSPL-1327 App install isn't triggered if 1 appsource is empty An empty `appSource` is not necessarily an error. Treat it as a no-op and throw a log message. * CSPL-1305 - Remove bias language from Paths & URLs (#509) * Bias-Lang Removal P1 - Handling Exceptions Consolidate Paths & URLs from external sources Include exceptions to GithubActions workflow * CSPL-1305 - Resolved Nightly builds regression (#547) Fixing Bias Language URLs to use correct tokens * CSPL-1277: Check for status of bundle push on CM (#536) * CSPL-1310 Merge MC feature branch to develop (#548) * CSPL-1306 - Remove bias language from Functions & Local variables (#543) * Bias-Lang P1 - Removal from Functions & Vars Renamed Functions and variables Renamed non-CRD files Added local script for bias language * Generating CRDs after changes * Removing CRD changes for olm legacy versions * Added new image push to int test (#566) * CSPL:1387 Modify app upload logic to selectively upload apps within test case (#557) Co-authored-by: Sirish Mohan <68884189+smohan-splunk@users.noreply.github.com> * CSPL-1379 IDXC fails to scale up when MC CR is deployed with pre-existing IDXC (#562) * IDXC fails to scale up when MC CR is deployed in the namespace with pre existing IDXC * Review comments * CSPL-1438: Add missing mc test develop (#565) * Add Missing MC test in custom resource test * Add missing MC Test in License Manger * Add missing MC test in secret test cases * Cleanup for MC Missing test * CSPL-1412/CSPL-1413 Break down app framework tests (#567) * CSPL-1412 * Update appframework_test.go * CSPL-1447-updated-version-for-find-replace-action (#576) * Updated go version to 1.17.3 (#581) * Cspl 1456 fix nightly test failures by changing testenv length and cleanup env (#579) * CSPL-1443 increase length of random string in appframework test to avoid reuse of same namespace * CSPL-1443 change cleanup logic to delete env every time unless DEBUG is set to true * disabling failing smoke tests in S1 and C3
Problem
App packages that are modified in a tracked
appRepo
are identified by the Splunk Operator, however this does not always trigger a change in the configmap. If the configmap data section does not change, then this modified app is never installed on the Splunk instance.Solution
While an app package modification does trigger rewriting the configmap. There are cases where this rewrite will result in the same data section as the previous version of the configmap (for example, when a single app is installed initially then modified.) When this is applied no change is detected and the Pod does not reset or install the new app.
By resetting the data in the app listing configmap when an app package change is detected, we can force a new resourceVersion of the configmap which will trigger the app install. The new ResourceVersion of the configmap will restart the pod, triggering the modified app install.
Also fix the logging in
splunk.enterprise.AddOrUpdateAppSrcDeploymentInfoList
so it no longer throws errors:Testing
Start a standalone with one app to install:
Validate that the initial confimap is correct:
Update that single app in the
appRepo
. Validate that the pod gets restarted and changed app installed even though the data in the configmap remains the same:Show configmap with revision label to force configmap update and pod restart:
Check app on Splunk instance:
Switch app in
appRepo
again and make sure this keeps working:Check configmap:
Check app installed on Splunk instance: