Skip to content
This repository has been archived by the owner on Aug 19, 2024. It is now read-only.

Reconcile Backstage when ConfigMap/Secret configuration changed #236

Closed
gazarenkov opened this issue Feb 26, 2024 · 12 comments
Closed

Reconcile Backstage when ConfigMap/Secret configuration changed #236

gazarenkov opened this issue Feb 26, 2024 · 12 comments
Labels
jira Issue will be sync'ed to Red Hat JIRA
Milestone

Comments

@gazarenkov
Copy link
Member

Need to make Backstage application automatically reload (which means BS container restart) if specified (in spec.Application) configuration changed.
Liiks like we need to:

  • Watch ConfigMaps and Secrets and make controller reconcile on changes
  • To do it optimal way consider to label respective ConfigMaps and Secrets with special labels
  • Document it
@github-actions github-actions bot added the jira Issue will be sync'ed to Red Hat JIRA label Feb 26, 2024
@rm3l
Copy link
Member

rm3l commented Feb 26, 2024

Seems related to #74?

@gazarenkov gazarenkov added this to the M3 milestone Feb 28, 2024
@jianrongzhang89
Copy link
Contributor

Performance impact should be considered and tested due to watching of the configmaps/secrets.

@rasheedamir
Copy link

This can be used to detect the change and restart - https://github.com/stakater/Reloader

@masayag
Copy link
Contributor

masayag commented Mar 27, 2024

I'd re-evaluate the need for this issue to be solved:
Since the Backstage CR supports mounting the deployment/pod to multiple secrets and configmaps, by enabling auto-restart per resource(secret/CM) change, the ability to perform an "atomic" restart after modifying several resources is lost.

Restarting Backstage pod isn't done quickly. It takes several minutes till the pod is restarted and this time increases as a direct dependency on the amount of enabled plugins.
For that reason, I'd leave it to the hands of the admin to decide when the changes to configmap and secret are completed and to trigger the application restart on demand.

However, I'd expect a deployment restart to work, while I checked this locally, and reported #281 for this.

@gazarenkov
Copy link
Member Author

@masayag that is a good point.

While I predict in most of the cases auto-restart would be prefferable, in some cases manual restart would be better by the reasons you mentioned.

And I am actually thinking about additional setting for operator, something like:
AutoRestartOnChanging=[true]|false

WDYT?

@masayag
Copy link
Contributor

masayag commented Mar 28, 2024

Allowing the admin to decide which method to use is a good option, +1 for AutoRestartOnChanging
Will this configuration be added to the Backstage CR and will be enabled per Backstage instance?

@rm3l rm3l mentioned this issue Mar 29, 2024
2 tasks
@gazarenkov
Copy link
Member Author

I think so, the idea is to have dedicated label per ConfigMap and make controller watch it and reconcile CR when associated CM changed.

@psav
Copy link

psav commented Apr 19, 2024

We had some issues with secrest/configmaps being continually written, updated by something in some of our environments and it led to the operator reconciling far more often than it needed to. We solved this by having an internal cache of hashes of configmaps that we cared about. That way we could filter events and only reconcile if we actually needed to.

@gazarenkov
Copy link
Member Author

@psav what exactly was updated by something else? Some metainfo?

@psav
Copy link

psav commented Apr 25, 2024

Sometimes it was another operator going wrong etc

@gazarenkov
Copy link
Member Author

gazarenkov commented Apr 30, 2024

Notes for design documentations:

Operator will recreate the Backstage Pod if new and old hash values of watched ConfigMaps/Secrets's data field is not equal.
Old hash is stored as a value of "rhdh.redhat.com/config-hash" annotation of deployment.spec.template
New hash is calculated when watched Config resources(ConfigMaps/Secrets) modified (created/updated/deleted).
Watched Configuration includes ConfigMaps/Secrets associated with Operator (created by User as a Raw config, App-config, extra values, dynamic-plugins resources) and contained:

  • label rhdh.redhat.com/config-sync=true
  • annotation rhdh.redhat.com/backstage-name=
    By default (will consider to make it configurable) Operator adds these labels/annotations for all managed and associated Configuration Resources.

In principle User is able to change it individually making Config Resource not watchable by setting rhdh.redhat.com/config-sync=false, even if there are no visible reasons why with exception of (more likely Cluster scoped) Security consideration below.

Security consideration:
To make this feature work Operator's SA should have read permissions to the Secret which contains potential risk for the case if this SA's token is compromised.
In general, Secret read permissions is not prohibited for Operators but in order if the cluster policy is that stricted, we can consider (for Day 2) the option/setting disabling wathching/counting associated Secrets, so in this case the feature will not work for the Secrets but will not fail with "permission denied" either.

Backstage configuration pattern:
Even if sharing the same CMs/Secrets for different (in one namespace) Backstage.spec.application.appConfig|extraFiles|extraEnvs is possible, it is highly NOT recommended to do so. In particular, it is not guaranteed Backstage Pod auto-recreation feature will work correct for all the CR instances sharing configuration.

@gazarenkov
Copy link
Member Author

gazarenkov commented May 7, 2024

I'd re-evaluate the need for this issue to be solved: Since the Backstage CR supports mounting the deployment/pod to multiple secrets and configmaps, by enabling auto-restart per resource(secret/CM) change, the ability to perform an "atomic" restart after modifying several resources is lost.

Restarting Backstage pod isn't done quickly. It takes several minutes till the pod is restarted and this time increases as a direct dependency on the amount of enabled plugins. For that reason, I'd leave it to the hands of the admin to decide when the changes to configmap and secret are completed and to trigger the application restart on demand.

However, I'd expect a deployment restart to work, while I checked this locally, and reported #281 for this.

@masayag current implementation is based on CM/Secret label/annotation so in principle it can be very atomic.
We decided to make simplify UX by setting those by Operator by default for all associated CMs/Secrets which could probably be reasonable taking into account there not a lot of CM/Secrets and they hardly changed that often in real life but there is a (operator scoped for the time) flag to disable Pod recreating as well. So, let's see how it goes and consider changing if needed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
jira Issue will be sync'ed to Red Hat JIRA
Projects
None yet
Development

No branches or pull requests

6 participants