Skip to content

MGMT-20233: Recompute operator dependencies after discovering the hosts #7524

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pastequo
Copy link
Contributor

@pastequo pastequo commented Apr 9, 2025

Reopening #7227

cc @jhernand

Also adding

  • Write DB changes in one transaction
  • Add call to EnsureOperatorPrerequisite after the operator list is updated

At first I added some logic to

  • Reset auto-assign roles if anything changed

but I removed it since it's already done in the HostStateMonitor

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

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

Was tested locally with others changes to have a single openshift AI bundle that add/remove nvidia or amd operator based on the host inventory during the discovery

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?

Currently operator dependencies are only calculated when a cluster is
created or updated. But certain dependencies are dynamic, and may
change when new hosts are added. For example, if a cluster has the
OpenShift AI operator installed, it will also require the NVIDIA GPU
operator only if there are hosts that have NVIDIA GPUs. To support those
dynamic dependencies this patch modifies the cluster monitor so that it
recalculates the operator dependencies before checking validations.

Signed-off-by: Juan Hernandez <[email protected]>
(cherry picked from commit 5371230)
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 9, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 9, 2025

@pastequo: This pull request references MGMT-20233 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 task to target the "4.19.0" version, but no target version was set.

In response to this:

Reopening #7227

cc @jhernand

Also adding

  • Write DB changes in one transaction
  • Add call to EnsureOperatorPrerequisite after the operator list is updated
  • Reset auto-assign roles if anything changed

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

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

Was tested locally with others changes to have a single openshift AI bundle that add/remove nvidia or amd operator based on the host inventory during the discovery

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?

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.

@openshift-ci openshift-ci bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 9, 2025
@openshift-ci openshift-ci bot requested review from eranco74 and mlorenzofr April 9, 2025 13:57
Copy link

openshift-ci bot commented Apr 9, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pastequo

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2025
Copy link

codecov bot commented Apr 9, 2025

Codecov Report

Attention: Patch coverage is 40.67797% with 70 lines in your changes missing coverage. Please review.

Project coverage is 67.87%. Comparing base (7e3ad83) to head (a34aedc).
Report is 24 commits behind head on master.

Files with missing lines Patch % Lines
internal/cluster/refresh_status_preprocessor.go 40.36% 50 Missing and 15 partials ⚠️
internal/operators/common/common.go 0.00% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7524      +/-   ##
==========================================
+ Coverage   67.29%   67.87%   +0.57%     
==========================================
  Files         334      335       +1     
  Lines       42293    43541    +1248     
==========================================
+ Hits        28461    29553    +1092     
- Misses      11258    11392     +134     
- Partials     2574     2596      +22     
Files with missing lines Coverage Δ
internal/bminventory/inventory.go 71.57% <ø> (ø)
internal/cluster/cluster.go 65.56% <100.00%> (ø)
internal/operators/manager.go 79.09% <100.00%> (ø)
internal/operators/common/common.go 22.38% <0.00%> (-1.81%) ⬇️
internal/cluster/refresh_status_preprocessor.go 70.08% <40.36%> (-24.25%) ⬇️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pastequo pastequo force-pushed the feat/recalculate-operator-dependencies branch from 1aaf644 to 99ad9fb Compare April 10, 2025 07:53
@pastequo
Copy link
Contributor Author

/retest

@pastequo
Copy link
Contributor Author

/retest-required

1 similar comment
@pastequo
Copy link
Contributor Author

/retest-required

* Run all db changes within a transaction
* Call to EnsureOperatorPrerequisite
@pastequo pastequo force-pushed the feat/recalculate-operator-dependencies branch from 99ad9fb to a34aedc Compare April 11, 2025 07:51
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 11, 2025

@pastequo: This pull request references MGMT-20233 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 task to target the "4.19.0" version, but no target version was set.

In response to this:

Reopening #7227

cc @jhernand

Also adding

  • Write DB changes in one transaction
  • Add call to EnsureOperatorPrerequisite after the operator list is updated

At first I added some logic to

  • Reset auto-assign roles if anything changed
    but I removed it since it's already done in the HostStateMonitor

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

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

Was tested locally with others changes to have a single openshift AI bundle that add/remove nvidia or amd operator based on the host inventory during the discovery

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?

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.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 11, 2025

@pastequo: This pull request references MGMT-20233 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 task to target the "4.19.0" version, but no target version was set.

In response to this:

Reopening #7227

cc @jhernand

Also adding

  • Write DB changes in one transaction
  • Add call to EnsureOperatorPrerequisite after the operator list is updated

At first I added some logic to

  • Reset auto-assign roles if anything changed

but I removed it since it's already done in the HostStateMonitor

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD

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

Was tested locally with others changes to have a single openshift AI bundle that add/remove nvidia or amd operator based on the host inventory during the discovery

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?

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.

@pastequo
Copy link
Contributor Author

/retest

@pastequo
Copy link
Contributor Author

/retest-required

4 similar comments
@pastequo
Copy link
Contributor Author

/retest-required

@pastequo
Copy link
Contributor Author

/retest-required

@pastequo
Copy link
Contributor Author

/retest-required

@pastequo
Copy link
Contributor Author

/retest-required

@jhernand
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 22, 2025
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 31a16a4 and 2 for PR HEAD a34aedc in total

@pastequo
Copy link
Contributor Author

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 31a16a4 and 2 for PR HEAD a34aedc in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 6125d3d and 1 for PR HEAD a34aedc in total

@pastequo
Copy link
Contributor Author

/retest

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD d222e13 and 0 for PR HEAD a34aedc in total

@pastequo
Copy link
Contributor Author

/retest-required

@openshift-ci-robot
Copy link

/hold

Revision a34aedc 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 Apr 23, 2025
@pastequo
Copy link
Contributor Author

/retest

@pastequo
Copy link
Contributor Author

/unhold

@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 Apr 24, 2025
Copy link

openshift-ci bot commented Apr 24, 2025

@pastequo: 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/okd-scos-e2e-aws-ovn a34aedc link false /test okd-scos-e2e-aws-ovn

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.

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. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants