MGMT-12471: Don't wait for console if it is disabled by jhernand · Pull Request #574 · openshift/assisted-installer

jhernand · 2022-11-08T15:52:13Z

This patch changes the controller so that it doesn't wait for the console operator when the console capability is explicitly disabled.

Related: https://issues.redhat.com/browse/MGMT-12471

codecov · 2022-11-08T16:06:39Z

Codecov Report

Merging #574 (6fcf60c) into master (58abce2) will decrease coverage by 0.00%.
The diff coverage is 60.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #574      +/-   ##
==========================================
- Coverage   52.11%   52.10%   -0.01%     
==========================================
  Files          14       14              
  Lines        2715     2727      +12     
==========================================
+ Hits         1415     1421       +6     
- Misses       1171     1175       +4     
- Partials      129      131       +2

Impacted Files	Coverage Δ
...taller_controller/assisted_installer_controller.go	`74.01% <57.14%> (-0.38%)`	⬇️
.../assisted_installer_controller/operator_handler.go	`79.02% <100.00%> (ø)`

omertuc

Looks perfect, just one small remark

omertuc · 2022-11-08T19:09:45Z

 	// zero means -> bring all logs
 	controllerLogsSecondsAgo  = 0
 	consoleOperatorName       = "console"
+	consoleCapabilityName     = "Console"


Can we:

Stop our weird go.mod replace directive for openshift/api and bring it as a proper dependency?

Use the actual constant for this

Let's not hold back this PR though and follow up with a separate PR

Unfortunately updating dependencies is painful, as usual. The "replace" for this one in particular can't be removed because there still exists a "v3.9.0" version in proxy.golang.org:

$ curl https://proxy.golang.org/github.com/openshift/api/@v/list v3.9.0+incompatible

The tag from the source repository was removed, but the entry in the Go module proxy is still there. This means that Go will always prefer that version, as there are no other version tags. So unless the OpenShift team retracts that version we can't remove the "replace".

I could update that replace directive to one that contains the "Console" constant, but unfortunately it seems it is only available in the "release-4.12" branch, and that requires Go 1.19:

$ go mod edit -replace github.com/openshift/api=github.com/openshift/api@release-4.12 $ go mod tidy $ go mod vendor $ go build -o build/installer src/main/main.go # k8s.io/apimachinery/pkg/conversion/queryparams vendor/k8s.io/apimachinery/pkg/conversion/queryparams/convert.go:58:17: undefined: reflect.Pointer vendor/k8s.io/apimachinery/pkg/conversion/queryparams/convert.go:142:7: undefined: reflect.Pointer note: module requires Go 1.19

I can get into that rabbit hole, but it is deeper than it looks.

This would be the first step, I think, just updating to Go 1.19: #575.

openshift-ci · 2022-11-08T19:10:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhernand, omertuc

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jhernand,omertuc]

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

romfreiman · 2022-11-08T20:20:55Z

-
+		clusterOperatorHandler := NewClusterOperatorHandler(c.kc, consoleOperatorName)
+		clusterVersionHandler := NewClusterVersionHandler(c.log, c.kc, timer)
+		disabledCapabilities, err := clusterVersionHandler.GetDisabledCapabilities()


@wking ptal

This patch changes the controller so that it doesn't wait for the console operator when the console capability is explicitly disabled. Related: https://issues.redhat.com/browse/MGMT-12471 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

omertuc · 2022-11-21T15:14:38Z

/lgtm

openshift-ci · 2022-11-21T17:34:50Z

@jhernand: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

This is a backport of MGMT-12471 for ACM 2.6. It contains the changes in pull requests openshift#574 and openshift#589. This patch changes the controller so that it doesn't wait for the console operator when the console capability is explicitly disabled. Related: https://issues.redhat.com/browse/MGMT-13657 Related: https://issues.redhat.com/browse/MGMT-12471 Related: openshift#574 Related: openshift#589 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

This is a backport of MGMT-12471 for ACM 2.7. It includes the changes in pull request openshift#589. The changes in pull request openshift#574 are alse necessary, but they are included in the ACM 2.7 branch already. Currently the operator handler checks if the operator is available in the service, but it doesn't take into account that the operator may have been removed from the list of managed operators. That results in a nil pointer exception and a crash of the controller. This patch fixes that adding a nil check to the relevant code. Related: https://issues.redhat.com/browse/MGMT-13664 Related: https://issues.redhat.com/browse/MGMT-12471 Related: openshift#589 Related: openshift#574 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

This is a backport of MGMT-12471 for ACM 2.6. It contains the changes in pull requests #574 and #589. This patch changes the controller so that it doesn't wait for the console operator when the console capability is explicitly disabled. Related: https://issues.redhat.com/browse/MGMT-13657 Related: https://issues.redhat.com/browse/MGMT-12471 Related: #574 Related: #589 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

This is a backport of MGMT-12471 for ACM 2.7. It includes the changes in pull request #589. The changes in pull request #574 are alse necessary, but they are included in the ACM 2.7 branch already. Currently the operator handler checks if the operator is available in the service, but it doesn't take into account that the operator may have been removed from the list of managed operators. That results in a nil pointer exception and a crash of the controller. This patch fixes that adding a nil check to the relevant code. Related: https://issues.redhat.com/browse/MGMT-13664 Related: https://issues.redhat.com/browse/MGMT-12471 Related: #589 Related: #574 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

openshift-ci Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 8, 2022

openshift-ci Bot requested review from rccrdpccl and romfreiman November 8, 2022 15:54

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2022

omertuc approved these changes Nov 8, 2022

View reviewed changes

openshift-ci Bot assigned omertuc Nov 8, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2022

romfreiman reviewed Nov 8, 2022

View reviewed changes

jhernand force-pushed the dont_wait_for_console_if_capability_is_disabled branch from 618045c to 9dc8264 Compare November 21, 2022 14:50

openshift-ci Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 21, 2022

omertuc reviewed Nov 21, 2022

View reviewed changes

Comment thread src/k8s_client/k8s_client.go Outdated

jhernand force-pushed the dont_wait_for_console_if_capability_is_disabled branch from 9dc8264 to 6fcf60c Compare November 21, 2022 15:12

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2022

openshift-merge-robot merged commit 1d74d0e into openshift:master Nov 21, 2022

jhernand deleted the dont_wait_for_console_if_capability_is_disabled branch November 30, 2022 10:38

jhernand mentioned this pull request Feb 14, 2023

MGMT-13657: Don't wait for console if it is disabled - ACM 2.6 #634

Merged

jhernand mentioned this pull request Feb 14, 2023

MGMT-13664: Don't crash if operator isn't monitored by service - ACM 2.7 #635

Merged

sebrandon1 mentioned this pull request Jul 8, 2026

OCPBUGS-85579: Move Console resources from bundle to runtime with capability detection openshift/cert-manager-operator#455

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

MGMT-12471: Don't wait for console if it is disabled - #574

MGMT-12471: Don't wait for console if it is disabled#574
openshift-merge-robot merged 1 commit into
openshift:masterfrom
jhernand:dont_wait_for_console_if_capability_is_disabled

jhernand commented Nov 8, 2022

Uh oh!

codecov Bot commented Nov 8, 2022 •

edited

Loading

Uh oh!

omertuc left a comment

Uh oh!

omertuc Nov 8, 2022

Uh oh!

jhernand Nov 10, 2022

Uh oh!

jhernand Nov 10, 2022

Uh oh!

openshift-ci Bot commented Nov 8, 2022

Uh oh!

romfreiman Nov 8, 2022

Uh oh!

Uh oh!

omertuc commented Nov 21, 2022

Uh oh!

openshift-ci Bot commented Nov 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jhernand commented Nov 8, 2022

Uh oh!

codecov Bot commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

omertuc left a comment

Choose a reason for hiding this comment

Uh oh!

omertuc Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

jhernand Nov 10, 2022

Choose a reason for hiding this comment

Uh oh!

jhernand Nov 10, 2022

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Nov 8, 2022

Uh oh!

romfreiman Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

omertuc commented Nov 21, 2022

Uh oh!

openshift-ci Bot commented Nov 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Nov 8, 2022 •

edited

Loading