-
Notifications
You must be signed in to change notification settings - Fork 459
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
WIP: MON-3547: Configuration CRD for the Cluster Monitoring Operator #1627
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @jan--f |
f9b89e6
to
0705ade
Compare
@marioferh: This pull request references MON-3547 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set. In response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
|
||
### Transition to the user | ||
|
||
- How the user could adopt CR instead of configmap. |
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.
IMHO the upgrade experience brings many questions (not limited to):
- Which tools/mechanisms do we provide to cluster admins to minimize the migration burden? Should CMO automatically create the custom resource from the current configmap when the CRD becomes GA? Or do we want a semi-automated approach with explicit approval from the cluster admins?
- Should we provide an escape hatch (via some annotation?) telling CMO to ignore the custom resource and only use the configmap in case something goes bad?
- How do we tell cluster admins that they need to migrate? CMO conditions could be an option as well as alerting and Insights?
- How do we know how many clusters have migrated to the new CRD? Telemetry I guess.
- How long should we keep the support for the configmap?
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.
Thinking about it
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.
Given how CMO works I think we might want to prioritize a low friction experience. Ideally admins won't have to do anything even after we enable the feature gate by default...if possible.
It seems that either the configmaps and CRs coexisting or a translation from configmap to CRD will have to be part of this.
Currently I lean towards the coexisting approach. I think this allows more flexibility when adding CRD fields. For example we might want to add stricter validations to a field in the CRD. With the translation approach we might not be able to pull something in without user intervention.
Otoh both config artifacts coexisting might raise issues with how a feature is configured. Perhaps that could be clarified through status conditions and additional logging?
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
Co-authored-by: Jan Fajerski <jan--f@users.noreply.github.com>
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
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.
At the moment, it seems there are a lot of open questions and things that have not yet been thought through. It would be good to get some of those open questions discussed before we get too far down the road of implementing an API
ClusterMonitoringConfiguration | ||
UserWorkloadConfiguration |
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.
Why 2 CRDs instead of one?
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.
Because they can have different personas managing each. For instance with managed OpenShift clusters (OSD/ROSA), the platform SREs own the cluster (platform) monitoring stack while platform users (e.g. customers) own the user-defined monitoring stack.
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
f803b85
to
d5b2dac
Compare
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.
It looks like the proposal lacks a few sections from the template (like "Risks and mitigations" for instance). Can you add them?
|
||
### Overview | ||
|
||
- Replace confimgaps with CRD: |
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.
Can you add a bit more details on the current implementation and the overall context for readers not familiar with the product?
- Describe the 2 stacks (platform & UWM)
- How does an admin configure each stack? Relationship between the 2?
- Explain that there can be 2 personas responsible for each stack (the managed OpenShift use case).
A link to https://github.com/openshift/enhancements/blob/master/enhancements/monitoring/multi-tenant-alerting.md would be useful.
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.
Add improvements
UserWorkloadConfiguration *UserWorkloadConfiguration `json:"-"` | ||
} | ||
``` | ||
|
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.
Maybe add here the general direction that we're going to keep more or less the structure of ClusterMonitoringConfiguration
and UserWorkloadConfiguration
but align them to the OpenShift API standards.
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.
Add improvements
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Signed-off-by: Mario Fernandez <mariofer@redhat.com>
@marioferh: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
## Motivation | ||
|
||
* The specification is well known and to a degree self-documenting. | ||
* The Kubernetes API Server validate custom resources based on their API specification, so users get immediate feedback on errors instead of checking the ClusterOperator object and the cluster monitoring operator's logs. Custom expression language (CEL) even allows to write complex validation logic, including cross-field tests. |
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.
Nit: Common expressions language, not Custom expressions language
- Replace confimgaps with CRD: | ||
|
||
``` | ||
type Config struct { |
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.
Is this the existing config?
|
||
|
||
``` | ||
type ClusterMonitoringConfiguration struct { |
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.
Is this the existing state? Or desired?
|
||
|
||
- Switch mecanishm | ||
- Both ConfigMap and CRD will coexist for a while until the transition is complete. |
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.
How will clusters that do not currently have a CR configured, but do have the configmap, continue to function without manual intervention as you go through upgrades?
- Extensive testing will be required to validate the new behavior. | ||
- If there are different fields in CRD and ConfigMap, the field in the ConfigMap will take precedence. | ||
- Some fields in CRD and ConfigMap diverge due to API requirements. Helper functions will be needed to resolve this. | ||
- The end user will be able to choose to use ConfigMap or CRD at any time, keeping in mind that the fields in the CRD will gradually expand. |
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.
Will this be a conscious choice? Is there some explicit switch for this or is it just a case of if one or the other exists?
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.
does user still can choose to use ConfigMap or CRD after the feature GA?
[More info](https://github.com/openshift/enhancements/blob/master/enhancements/monitoring/multi-tenant-alerting.md) | ||
|
||
|
||
- Replace confimgaps with CRD: |
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.
- Replace confimgaps with CRD: | |
- Replace configmaps with CRD: |
Add example CustomResource: config/examples/clustermonitoringoperator.yaml | ||
- Client codegen | ||
- Reconcile logic: https://github.com/openshift/cluster-monitoring-operator/pull/2350 | ||
- Add decoupling Confimgap / CustomResource: |
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.
- Add decoupling Confimgap / CustomResource: | |
- Add decoupling Configmap / CustomResource: |
## Motivation | ||
|
||
* The specification is well known and to a degree self-documenting. | ||
* The Kubernetes API Server validate custom resources based on their API specification, so users get immediate feedback on errors instead of checking the ClusterOperator object and the cluster monitoring operator's logs. Custom expression language (CEL) even allows to write complex validation logic, including cross-field tests. |
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.
* The Kubernetes API Server validate custom resources based on their API specification, so users get immediate feedback on errors instead of checking the ClusterOperator object and the cluster monitoring operator's logs. Custom expression language (CEL) even allows to write complex validation logic, including cross-field tests. | |
* The Kubernetes API Server validates custom resources based on their API specification, so users get immediate feedback on errors instead of checking the ClusterOperator object and the cluster monitoring operator's logs. Custom expression language (CEL) even allows to write complex validation logic, including cross-field tests. |
### Migration path | ||
|
||
|
||
- Switch mecanishm |
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.
- Switch mecanishm | |
- Switch mechanism |
|
||
## Design Details | ||
|
||
To initiate the process, let's establish a feature gate that will serve as the entry point for implementing a CRD configuration approach. This strategy enables us to make incremental advancements without the immediate burden of achieving complete feature equivalence with the config map. We can commence with a the basics and progressively incorporate additional functionalities as they develop. |
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.
To initiate the process, let's establish a feature gate that will serve as the entry point for implementing a CRD configuration approach. This strategy enables us to make incremental advancements without the immediate burden of achieving complete feature equivalence with the config map. We can commence with a the basics and progressively incorporate additional functionalities as they develop. | |
To initiate the process, let's establish a feature gate that will serve as the entry point for implementing a CRD configuration approach. This strategy enables us to make incremental advancements without the immediate burden of achieving complete feature equivalence with the config map. We can commence with the basics and progressively incorporate additional functionalities as they develop. |
- "@danielmellado" | ||
- "@jan--f" | ||
- "@moadz" | ||
- "simonpasquier" |
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.
- "simonpasquier" | |
- "@simonpasquier" |
reviewers: | ||
- "@jan--f" | ||
- "@moadz" | ||
- "simonpasquier" |
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.
- "simonpasquier" | |
- "@simonpasquier" |
approvers: | ||
- "@jan--f" | ||
- "@moadz" | ||
- "simonpasquier" |
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.
- "simonpasquier" | |
- "@simonpasquier" |
|
||
### Overview | ||
|
||
Currently in CMO a config map provides a way to inject configuration data into pods. There are two configmaps for the different stacks: |
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.
Currently in CMO a config map provides a way to inject configuration data into pods. There are two configmaps for the different stacks: | |
Currently in CMO, a config map provides a way to inject configuration data into pods. There are two configmaps for the different stacks: |
|
||
Currently in CMO a config map provides a way to inject configuration data into pods. There are two configmaps for the different stacks: | ||
|
||
cluster-monitoring-configmap: Default platform monitoring components. A set of platform monitoring components are installed in the openshift-monitoring project by default during an OpenShift Container Platform installation. This provides monitoring for core cluster components including Kubernetes services. The default monitoring stack also enables remote health monitoring for clusters. |
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.
cluster-monitoring-configmap: Default platform monitoring components. A set of platform monitoring components are installed in the openshift-monitoring project by default during an OpenShift Container Platform installation. This provides monitoring for core cluster components including Kubernetes services. The default monitoring stack also enables remote health monitoring for clusters. | |
cluster-monitoring-config: Default platform monitoring components. A set of platform monitoring components are installed in the openshift-monitoring project by default during an OpenShift Container Platform installation. This provides monitoring for core cluster components including Kubernetes services. The default monitoring stack also enables remote health monitoring for clusters. |
|
||
One proposal for a minimal DoD was: | ||
- Feature gate in openshift/api | ||
- Api types moved to openshift/api |
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.
- Api types moved to openshift/api | |
- API types moved to openshift/api |
To initiate the process, let's establish a feature gate that will serve as the entry point for implementing a CRD configuration approach. This strategy enables us to make incremental advancements without the immediate burden of achieving complete feature equivalence with the config map. We can commence with a the basics and progressively incorporate additional functionalities as they develop. | ||
|
||
One proposal for a minimal DoD was: | ||
- Feature gate in openshift/api |
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.
- Feature gate in openshift/api | |
- Feature gate in openshift/api |
it seems you want to get a list, but the format is
One proposal for a minimal DoD was: - Feature gate in openshift/api - Api types moved to openshift/api - CRD Initial dev https://github.com/openshift/api/pull/1929 Add controller-gen logic to makefile Add API to api/config/v1 Add Generated CRD: config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitoring.crd.yaml Add example CustomResource: config/examples/clustermonitoringoperator.yaml - Client codegen - Reconcile logic: https://github.com/openshift/cluster-monitoring-operator/pull/2350 - Add decoupling Confimgap / CustomResource: Controller logic is strongly dependant of *manifests.Config struct.
resources: | ||
requests: | ||
storage: 20Gi | ||
|
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.
could remove the empty line
spec: | ||
resources: | ||
requests: | ||
storage: 20G` |
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.
storage: 20G` | |
storage: 20G |
or
storage: 20G` | |
storage: 20Gi |
### Test Plan | ||
|
||
- Unit tests for the feature | ||
- e2e tests covering the feature |
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 think upgrade testing should also be covered
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
No description provided.