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.

Sign up for GitHub

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

Jump to bottom

[release-2.12] Add MCOA status for CRD dependencies #1608

Merged
merged 4 commits into from
Sep 11, 2024
Merged

[release-2.12] Add MCOA status for CRD dependencies #1608

merged 4 commits into from
Sep 11, 2024

Conversation

JoaoBraveCoding
Copy link
Contributor

@JoaoBraveCoding JoaoBraveCoding commented Sep 6, 2024 •
edited
Loading

Succeeds #1564

The following PR is an amendment/extention to #1470 to improve the user experience when a user decides to use the new capabilities spec and in turn MCOA. Currently the MCOA internal reconciliation loop depends on a set of CustomResourceDefinitions (CRD) owned by other operator dependecies (i.e. cluster-logging-operator, opentelemetrycollector-operator). The underlying implementation checks if the CRDs exist and actually serve the required CRD versions. If any is missing the MCO status shows the MCOADegradedCondition, e.g.:

conditions:
  - lastTransitionTime: "2024-08-07T09:25:01Z"
    message: 'MultiCluster-Observability-Addon degraded because the following CRDs are not installed on this hub cluster: opentelemetrycollectors.opentelemetry.io(v1beta1)'
    reason: MultiClusterObservabilityAddonDegraded
    status: "True"
    type: MultiClusterObservabilityAddonDegraded

cc @periklis

JoaoBraveCoding
JoaoBraveCoding commented Sep 6, 2024
View reviewed changes
...usterobservability/controllers/multiclusterobservability/multiclusterobservability_status.go
msg := fmt.Sprintf(tmpl, strings.Join(missingVersions, ", "))

return &mcoshared.Condition{
Type: reasonMCOADegraded,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was where the problem was. I tried refactoring the way the status was being generated but it made the PR bigger than what's necessary for this. In the future if we want we'll refactor it

JoaoBraveCoding
JoaoBraveCoding commented Sep 6, 2024
View reviewed changes
operators/multiclusterobservability/pkg/config/config.go
const (
OauthProxyImageStreamName = "oauth-proxy"
OauthProxyImageStreamNamespace = "openshift"
)

const (
ClusterLogForwarderCRDName = "clusterlogforwarders.logging.openshift.io"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my POV we can update these once stolostron/multicluster-observability-addon#63 lands, let me know otherwise

periklis
periklis approved these changes Sep 9, 2024
View reviewed changes
Copy link
Contributor

@periklis periklis 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 label Sep 9, 2024
@openshift-ci openshift-ci bot removed the lgtm label Sep 10, 2024
periklis
periklis approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@periklis periklis 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 label Sep 10, 2024
@periklis
Copy link
Contributor

/retest

periklis and others added 4 commits September 10, 2024 13:58
@periklis @JoaoBraveCoding
Add MCOA status for CRD dependencies
4262181 
Signed-off-by: Periklis Tsirakidis <periklis@redhat.com>
@periklis @JoaoBraveCoding
Fix sonar lint issues
703dab8 
Signed-off-by: Periklis Tsirakidis <periklis@redhat.com>
@periklis @JoaoBraveCoding
Split type and reason in condition
57b19e6 
Signed-off-by: Periklis Tsirakidis <periklis@redhat.com>
@JoaoBraveCoding
status: change type of MCOA degraded to allow the status to be unset
4e8fd3c 
Signed-off-by: Joao Marcal <jmarcal@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm label Sep 10, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 10, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
82.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

periklis
periklis approved these changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@periklis periklis 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 label Sep 10, 2024
@periklis
Copy link
Contributor

/refresh

philipgough
philipgough approved these changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@philipgough philipgough 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
Copy link

openshift-ci bot commented Sep 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoaoBraveCoding, periklis, philipgough

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 [JoaoBraveCoding,periklis,philipgough]

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

@openshift-merge-bot openshift-merge-bot bot merged commit 6aa0cb9 into stolostron:main Sep 11, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipgough philipgough philipgough approved these changes

@periklis periklis periklis approved these changes

Assignees

@periklis periklis

@philipgough philipgough

Labels
approved dco-signoff: yes lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JoaoBraveCoding @periklis @philipgough