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

jhernand · 2022-11-08T15:44:09Z

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Tested manually creating a cluster with the console disabled and waiting for the installation to complete.

$ cat > disable-console.json <<.
{
  "capabilities": {
    "baselineCapabilitySet": "None",
    "additionalEnabledCapabilities": [
      "baremetal"
    ]
  }
}
.

$ cat disable-console.json | jq -sR > disable-console.patch

$ ocm patch /api/assisted-install/v2/clusters/.../install-config --body disable-console.patch

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci · 2022-11-08T15:45:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhernand

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]

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

codecov · 2022-11-08T16:00:03Z

Codecov Report

Merging #4594 (7bf4c68) into master (46d5090) will increase coverage by 0.08%.
The diff coverage is n/a.

❗ Current head 7bf4c68 differs from pull request most recent head 24b5d95. Consider uploading reports for the commit 24b5d95 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4594      +/-   ##
==========================================
+ Coverage   66.95%   67.03%   +0.08%     
==========================================
  Files         200      200              
  Lines       28436    28179     -257     
==========================================
- Hits        19040    18891     -149     
+ Misses       7662     7590      -72     
+ Partials     1734     1698      -36

Impacted Files	Coverage Δ
internal/oc/release.go	`77.19% <0.00%> (-1.17%)`	⬇️
internal/host/monitor.go	`77.84% <0.00%> (-0.14%)`	⬇️
internal/common/common.go	`32.08% <0.00%> (ø)`
internal/ignition/ignition.go	`58.05% <0.00%> (ø)`
internal/operators/lvm/config.go	`100.00% <0.00%> (ø)`
internal/host/hostutil/test_utils.go	`11.28% <0.00%> (+0.05%)`	⬆️
internal/bminventory/inventory.go	`69.37% <0.00%> (+0.11%)`	⬆️
...oller/controllers/agentserviceconfig_controller.go	`85.59% <0.00%> (+0.73%)`	⬆️
internal/operators/lso/ls_operator.go	`11.42% <0.00%> (+0.90%)`	⬆️
internal/host/hostcommands/install_cmd.go	`83.88% <0.00%> (+1.42%)`	⬆️
... and 6 more

jhernand · 2022-11-16T14:25:44Z

/test edge-e2e-metal-assisted

jhernand · 2022-11-24T12:41:38Z

@avishayt @mkowalski the bot assigned you as reviewers, I'd appreciate your input.

iranzo

Just a small nit from my PoV

jhernand · 2022-11-24T16:12:01Z

/test edge-e2e-metal-assisted

jhernand · 2022-11-30T10:39:45Z

Dear reviewers (@avishayt and @mkowalski) I need your input to move this forward.

avishayt · 2022-11-30T14:50:05Z

Looks good to me but would prefer another set of eyes.

filanov · 2022-12-06T11:59:35Z

/assign @eranco74
/assign @tsorya

eranco74

looks good, just nit

jhernand · 2022-12-07T16:21:28Z

/hold I having an issue when testing this manually, will unhold when I understand/fix it.

jhernand · 2022-12-09T08:42:14Z

@eranco74 @tsorya while testing this I realized that capabilities are supported since 4.11, but the console capability is only supported since 4.12. I changed the patch accordingly.

While testing with 4.11 I also detected that there is a potential crash in the controller. I am trying to address it here: openshift/assisted-installer#589.

I think this is ready for review again.

jhernand · 2022-12-09T08:42:26Z

/unhold

eranco74

/lgtm

eranco74 · 2022-12-12T16:08:33Z

/lgtm

Currently the service keeps for each cluster a list of the operators that it will wait for before considering the cluster installed. This list includes the console operator. But since version 4.11 that can be disabled via the `capabilities` section of the installer configuration file. For example: ```yaml capabilities: baselineCapabilitySet: None additionalEnabledCapabilities: - baremetal ``` That will configure the cluster so that only the `baremetal` optional capability will be installed, which effectively disables the `Console` capability. This patch changes the service so that it removes the console operator from that list when this kind of update is made to the installer configuration. Related: https://issues.redhat.com/browse/MGMT-12471 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

jhernand · 2022-12-13T08:14:24Z

@eranco74 @tsorya I had to rebase this, the only change was in go.sum, it needs the /lgtm again.

tsorya · 2022-12-13T08:21:44Z

/lgtm

eranco74 · 2022-12-13T08:25:18Z

/lgtm

openshift-ci · 2022-12-13T10:45:04Z

@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.7. It contains the changes in pull request openshift#4594. Currently the service keeps for each cluster a list of the operators that it will wait for before considering the cluster installed. This list includes the console operator. But since version 4.11 that can be disabled via the `capabilities` section of the installer configuration file. For example: ```yaml capabilities: baselineCapabilitySet: None additionalEnabledCapabilities: - baremetal ``` That will configure the cluster so that only the `baremetal` optional capability will be installed, which effectively disables the `Console` capability. This patch changes the service so that it removes the console operator from that list when this kind of update is made to the installer configuration. Related: https://issues.redhat.com/browse/MGMT-13664 Related: https://issues.redhat.com/browse/MGMT-12471 Related: openshift#4594 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com> Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

This is a backport of MGMT-12471 for ACM 2.6. It includes the changes from pull request openshift#4594. Currently the service keeps for each cluster a list of the operators that it will wait for before considering the cluster installed. This list includes the console operator. But since version 4.11 that can be disabled via the `capabilities` section of the installer configuration file. For example: ```yaml capabilities: baselineCapabilitySet: None additionalEnabledCapabilities: - baremetal ``` That will configure the cluster so that only the `baremetal` optional capability will be installed, which effectively disables the `Console` capability. This patch changes the service so that it removes the console operator from that list when this kind of update is made to the installer configuration. Related: https://issues.redhat.com/browse/MGMT-13657 Related: https://issues.redhat.com/browse/MGMT-12471 Related: openshift#4594 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

This is a backport of MGMT-12471 for ACM 2.6. It includes the changes from pull request #4594. Currently the service keeps for each cluster a list of the operators that it will wait for before considering the cluster installed. This list includes the console operator. But since version 4.11 that can be disabled via the `capabilities` section of the installer configuration file. For example: ```yaml capabilities: baselineCapabilitySet: None additionalEnabledCapabilities: - baremetal ``` That will configure the cluster so that only the `baremetal` optional capability will be installed, which effectively disables the `Console` capability. This patch changes the service so that it removes the console operator from that list when this kind of update is made to the installer configuration. Related: https://issues.redhat.com/browse/MGMT-13657 Related: https://issues.redhat.com/browse/MGMT-12471 Related: #4594 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

This is a backport of MGMT-12471 for ACM 2.7. It contains the changes in pull request #4594. Currently the service keeps for each cluster a list of the operators that it will wait for before considering the cluster installed. This list includes the console operator. But since version 4.11 that can be disabled via the `capabilities` section of the installer configuration file. For example: ```yaml capabilities: baselineCapabilitySet: None additionalEnabledCapabilities: - baremetal ``` That will configure the cluster so that only the `baremetal` optional capability will be installed, which effectively disables the `Console` capability. This patch changes the service so that it removes the console operator from that list when this kind of update is made to the installer configuration. Related: https://issues.redhat.com/browse/MGMT-13664 Related: https://issues.redhat.com/browse/MGMT-12471 Related: #4594 Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>

openshift-ci Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 8, 2022

openshift-ci Bot requested review from avishayt and mkowalski November 8, 2022 15:44

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

jhernand changed the title ~~Dont wait for console if disabled~~ MGMT-12471: Don't wait for console if disabled Nov 8, 2022

jhernand changed the title ~~MGMT-12471: Don't wait for console if disabled~~ MGMT-12471: Don't wait for console if it is disabled Nov 8, 2022

jhernand force-pushed the dont_wait_for_console_if_disabled branch from fc21688 to c708295 Compare November 16, 2022 18:02

jhernand mentioned this pull request Nov 24, 2022

TELCODOCS-831: Document capabilities exclusion #4213

Merged

21 tasks

iranzo suggested changes Nov 24, 2022

View reviewed changes

Comment thread internal/bminventory/inventory.go Outdated

openshift-ci Bot assigned iranzo Nov 24, 2022

iranzo removed their assignment Nov 24, 2022

jhernand force-pushed the dont_wait_for_console_if_disabled branch from c708295 to 24b5d95 Compare November 24, 2022 13:22

avishayt reviewed Nov 30, 2022

View reviewed changes

Comment thread internal/bminventory/inventory.go Outdated

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2022

openshift-ci Bot assigned eranco74 and tsorya Dec 6, 2022

eranco74 reviewed Dec 7, 2022

View reviewed changes

Comment thread internal/bminventory/inventory.go Outdated

Comment thread internal/bminventory/inventory.go Outdated

Comment thread internal/bminventory/inventory.go Outdated

Comment thread internal/bminventory/inventory.go Outdated

Comment thread internal/bminventory/inventory.go Outdated

jhernand force-pushed the dont_wait_for_console_if_disabled branch from 24b5d95 to 113675e Compare December 7, 2022 13:48

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2022

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 7, 2022

jhernand force-pushed the dont_wait_for_console_if_disabled branch from 113675e to a5ff743 Compare December 7, 2022 19:38

jhernand force-pushed the dont_wait_for_console_if_disabled branch from a5ff743 to f1590bd Compare December 7, 2022 19:38

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2022

eranco74 reviewed Dec 12, 2022

View reviewed changes

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

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2022

jhernand force-pushed the dont_wait_for_console_if_disabled branch from f1590bd to a76bedf Compare December 13, 2022 08:12

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2022

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022

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

openshift-merge-robot merged commit f814368 into openshift:master Dec 13, 2022

jhernand deleted the dont_wait_for_console_if_disabled branch December 13, 2022 11:09

jhernand mentioned this pull request Feb 14, 2023

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

Merged

20 tasks

jhernand mentioned this pull request Feb 14, 2023

MGMT-13664: Don't wait for console if it is disabled - ACM 2.7 #4981

Merged

20 tasks

Uh oh!

Conversation

jhernand commented Nov 8, 2022

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

Uh oh!

openshift-ci Bot 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!

jhernand commented Nov 16, 2022

Uh oh!

jhernand commented Nov 24, 2022

Uh oh!

iranzo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhernand commented Nov 24, 2022

Uh oh!

jhernand commented Nov 30, 2022

Uh oh!

Uh oh!

avishayt commented Nov 30, 2022

Uh oh!

filanov commented Dec 6, 2022

Uh oh!

eranco74 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhernand commented Dec 7, 2022

Uh oh!

jhernand commented Dec 9, 2022

Uh oh!

jhernand commented Dec 9, 2022

Uh oh!

eranco74 left a comment

Choose a reason for hiding this comment

Uh oh!

eranco74 commented Dec 12, 2022

Uh oh!

jhernand commented Dec 13, 2022

Uh oh!

tsorya commented Dec 13, 2022

Uh oh!

eranco74 commented Dec 13, 2022

Uh oh!

openshift-ci Bot commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov Bot commented Nov 8, 2022 •

edited

Loading