Skip to content
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

First minimal PoC version for moving the configuration to configmap #827

Merged
merged 8 commits into from
Oct 19, 2023

Conversation

tremes
Copy link
Contributor

@tremes tremes commented Sep 27, 2023

This is minimal PoC for introducing the Insights Operator's configuration in the new insights-config configmap instead of the support secret configuration. The support secret configuration continues to work (to keep backward compatibility), but the configmap takes precedence if exists.

This PR introduces following new files:

  • pkg/config/configobserver/configmapobserver.go - basic configmap observer watching the configmap's changes using Kubernetes informer
  • pkg/config/configobserver/config_aggregator.go - this is a listener for both - the original "legacy" secret configobserver as well as the new configmap observer. The idea here is to aggregate or merge the config from both sources at one place.

The insights-config configmap is supposed to live in the openshift-insights namespace and right now it can look like:

apiVersion: v1
kind: ConfigMap
metadata:
  name: 'insights-config'
  namespace: openshift-insights
data:
 config.yaml: |
   dataReporting:
     interval: 1h
     uploadEndpoint: https://console.redhat.com/api/ingress/v1/upload
binaryData: {}
immutable: false

Right now the config map allows you to override the following config options:

  • interval
  • upload endpoint
  • conditional gatherer endpoint
  • storage path
  • download endpoint for tech preview
  • processing status endpoint (tech preview only)

The configmap (as well as the config) structure is defined in the pkg/config/types.go so please review this one very carefully and feel free to suggest changes (for example naming, structure, etc.)

Categories

  • Bugfix
  • Data Enhancement
  • Feature
  • Backporting
  • Others (CI, Infrastructure, Documentation)

Sample Archive

  • no new data

Documentation

Unit Tests

Privacy

Yes. There are no sensitive data in the newly collected information.

Changelog

Breaking Changes

Yes/No

References

https://issues.redhat.com/browse/???
https://access.redhat.com/solutions/???

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2023
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2023
@tremes tremes changed the title WIP First minimal PoC version for moving the configuration to configmap First minimal PoC version for moving the configuration to configmap Oct 3, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 3, 2023
@tremes
Copy link
Contributor Author

tremes commented Oct 3, 2023

/test insights-operator-e2e-tests

@openshift-merge-robot
Copy link
Contributor

@tremes: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/insights-operator-e2e-tests 4add5bb link true /test insights-operator-e2e-tests

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

pkg/config/configobserver/configmapobserver.go Outdated Show resolved Hide resolved
pkg/config/configobserver/configmapobserver.go Outdated Show resolved Hide resolved
pkg/config/types.go Show resolved Hide resolved
pkg/config/types.go Outdated Show resolved Hide resolved
Copy link
Contributor

@rluders rluders left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2023
@openshift-ci
Copy link

openshift-ci bot commented Oct 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncaak, rluders, tremes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [ncaak,rluders,tremes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tremes
Copy link
Contributor Author

tremes commented Oct 17, 2023

/retest

@JoaoFula
Copy link
Contributor

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Oct 17, 2023
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD f6c7583 and 2 for PR HEAD 2f0cc66 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 67f607e and 1 for PR HEAD 2f0cc66 in total

@tremes
Copy link
Contributor Author

tremes commented Oct 17, 2023

/test insights-operator-e2e-tests

1 similar comment
@tremes
Copy link
Contributor Author

tremes commented Oct 18, 2023

/test insights-operator-e2e-tests

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD d25171c and 0 for PR HEAD 2f0cc66 in total

@tremes
Copy link
Contributor Author

tremes commented Oct 18, 2023

/override ci/prow/e2e-metal-ipi-ovn-ipv6

@openshift-ci
Copy link

openshift-ci bot commented Oct 18, 2023

@tremes: Overrode contexts on behalf of tremes: ci/prow/e2e-metal-ipi-ovn-ipv6

In response to this:

/override ci/prow/e2e-metal-ipi-ovn-ipv6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Contributor

/hold

Revision 2f0cc66 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2023
@tremes
Copy link
Contributor Author

tremes commented Oct 18, 2023

/unhold
/test insights-operator-e2e-tests

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2023
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD d25171c and 2 for PR HEAD 2f0cc66 in total

@tremes
Copy link
Contributor Author

tremes commented Oct 19, 2023

/test e2e-gcp-ovn-techpreview

@tremes
Copy link
Contributor Author

tremes commented Oct 19, 2023

One test failure - I am working on the test fix.
/override ci/prow/e2e-gcp-ovn-techpreview

@openshift-ci
Copy link

openshift-ci bot commented Oct 19, 2023

@tremes: Overrode contexts on behalf of tremes: ci/prow/e2e-gcp-ovn-techpreview

In response to this:

One test failure - I am working on the test fix.
/override ci/prow/e2e-gcp-ovn-techpreview

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tremes
Copy link
Contributor Author

tremes commented Oct 19, 2023

/override ci/prow/e2e-gcp-ovn-techpreview

@openshift-ci
Copy link

openshift-ci bot commented Oct 19, 2023

@tremes: Overrode contexts on behalf of tremes: ci/prow/e2e-gcp-ovn-techpreview

In response to this:

/override ci/prow/e2e-gcp-ovn-techpreview

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link

openshift-ci bot commented Oct 19, 2023

@tremes: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-ci openshift-ci bot merged commit d0e7565 into openshift:master Oct 19, 2023
9 checks passed
DennisPeriquet added a commit to DennisPeriquet/insights-operator that referenced this pull request Oct 19, 2023
openshift-ci bot pushed a commit that referenced this pull request Oct 19, 2023
tremes added a commit to tremes/insights-operator that referenced this pull request Oct 20, 2023
openshift-ci bot pushed a commit that referenced this pull request Oct 20, 2023
* Revert "Revert "First minimal PoC version for moving the configuration to configmap (#827)" (#845)"

This reverts commit d589796.

* fix possibly negative time values
JoaoFula pushed a commit to JoaoFula/insights-operator that referenced this pull request Jan 23, 2024
…penshift#827)

* First minimal PoC version for moving the configuration to configmap

* fix uploader in case of initialDelay = 0

* initiate idea with static one-off config merge

* add permissions for secrets

* updates after review

* minor changes

* rebase

* use mutex as previously
JoaoFula pushed a commit to JoaoFula/insights-operator that referenced this pull request Jan 23, 2024
JoaoFula pushed a commit to JoaoFula/insights-operator that referenced this pull request Jan 23, 2024
* Revert "Revert "First minimal PoC version for moving the configuration to configmap (openshift#827)" (openshift#845)"

This reverts commit d589796.

* fix possibly negative time values
@tremes tremes deleted the config-refactor branch August 15, 2024 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants