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.

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

ACM-14502: Allow updates to InfraEnv version #6808

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

Conversation

carbonin
Copy link
Member

@carbonin carbonin commented Sep 25, 2024

Allow updating the InfraEnv OpenshiftVersion in both the REST and kube APIs.

This is useful when after initial install a user wants to add a new node to a cluster, but the cluster has been upgraded between install and the scale out. Previously a user would need to create a new infraenv with identical settings to the old one and a newer version.

With this change users will be able to update the infraenv os image version just like other attributes.

For some background:

The field was added to the InfraEnv CR in #5365 for https://issues.redhat.com//browse/MGMT-14923 and I don't see any discussion there about updates.

Based on the PR that added infraenv update (#2319) the update params were originally created without the version with no discussion I could find.

Based on these it doesn't seem like this was an intentional omission, so it should be safe to add it here.

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?

Deployed assisted service kube-api, created an infraenv, updated osImageVersion several times.
Observed the URL change accordingly each time and an error in the infraenv status when a non-existent version was requested.

Additionally subsystem tests were added to cover this.

  • 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

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 openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 25, 2024
@openshift-ci-robot openshift-ci-robot added the jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. label Sep 25, 2024
Copy link

openshift-ci bot commented Sep 25, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 25, 2024
@openshift-ci-robot
Copy link

@carbonin: This pull request references Jira Issue OCPBUGS-42151, which is invalid:

  • expected the bug to target the "4.18.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

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

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 openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Sep 25, 2024
@carbonin
Copy link
Member Author

/test ?

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 25, 2024
Copy link

openshift-ci bot commented Sep 25, 2024

@carbonin: The following commands are available to trigger required jobs:

  • /test e2e-agent-compact-ipv4
  • /test edge-assisted-operator-catalog-publish-verify
  • /test edge-ci-index
  • /test edge-e2e-ai-operator-ztp
  • /test edge-e2e-ai-operator-ztp-sno-day2-workers
  • /test edge-e2e-ai-operator-ztp-sno-day2-workers-late-binding
  • /test edge-e2e-metal-assisted
  • /test edge-e2e-metal-assisted-4-12
  • /test edge-e2e-metal-assisted-4-control-planes-4-17
  • /test edge-e2e-metal-assisted-4-control-planes-4-18
  • /test edge-e2e-metal-assisted-5-control-planes-4-17
  • /test edge-e2e-metal-assisted-5-control-planes-4-18
  • /test edge-e2e-metal-assisted-cnv-4-16
  • /test edge-e2e-metal-assisted-lvm
  • /test edge-e2e-metal-assisted-odf-4-16
  • /test edge-images
  • /test edge-lint
  • /test edge-subsystem-aws
  • /test edge-subsystem-kubeapi-aws
  • /test edge-unit-test
  • /test edge-verify-generated-code
  • /test images
  • /test mce-images

The following commands are available to trigger optional jobs:

  • /test e2e-agent-ha-dualstack
  • /test e2e-agent-sno-ipv6
  • /test edge-e2e-ai-operator-disconnected-capi
  • /test edge-e2e-ai-operator-ztp-3masters
  • /test edge-e2e-ai-operator-ztp-capi
  • /test edge-e2e-ai-operator-ztp-compact-day2-masters
  • /test edge-e2e-ai-operator-ztp-compact-day2-workers
  • /test edge-e2e-ai-operator-ztp-disconnected
  • /test edge-e2e-ai-operator-ztp-hypershift-zero-nodes
  • /test edge-e2e-ai-operator-ztp-multiarch-3masters-ocp
  • /test edge-e2e-ai-operator-ztp-multiarch-sno-ocp
  • /test edge-e2e-ai-operator-ztp-node-labels
  • /test edge-e2e-ai-operator-ztp-remove-node
  • /test edge-e2e-ai-operator-ztp-sno-day2-masters
  • /test edge-e2e-ai-operator-ztp-sno-day2-workers-ignitionoverride
  • /test edge-e2e-metal-assisted-4-13
  • /test edge-e2e-metal-assisted-4-14
  • /test edge-e2e-metal-assisted-4-15
  • /test edge-e2e-metal-assisted-4-16
  • /test edge-e2e-metal-assisted-bond
  • /test edge-e2e-metal-assisted-bond-4-12
  • /test edge-e2e-metal-assisted-bond-4-13
  • /test edge-e2e-metal-assisted-bond-4-14
  • /test edge-e2e-metal-assisted-bond-4-15
  • /test edge-e2e-metal-assisted-bond-4-16
  • /test edge-e2e-metal-assisted-day2
  • /test edge-e2e-metal-assisted-day2-arm-workers
  • /test edge-e2e-metal-assisted-day2-single-node
  • /test edge-e2e-metal-assisted-external
  • /test edge-e2e-metal-assisted-external-4-14
  • /test edge-e2e-metal-assisted-ipv4v6
  • /test edge-e2e-metal-assisted-ipv6
  • /test edge-e2e-metal-assisted-kube-api-late-binding-single-node
  • /test edge-e2e-metal-assisted-kube-api-late-unbinding-ipv4-single-node
  • /test edge-e2e-metal-assisted-kube-api-net-suite
  • /test edge-e2e-metal-assisted-mce-4-16
  • /test edge-e2e-metal-assisted-mce-sno-4-16
  • /test edge-e2e-metal-assisted-metallb
  • /test edge-e2e-metal-assisted-none
  • /test edge-e2e-metal-assisted-onprem
  • /test edge-e2e-metal-assisted-single-node
  • /test edge-e2e-metal-assisted-static-ip-suite
  • /test edge-e2e-metal-assisted-static-ip-suite-4-12
  • /test edge-e2e-metal-assisted-static-ip-suite-4-13
  • /test edge-e2e-metal-assisted-static-ip-suite-4-14
  • /test edge-e2e-metal-assisted-static-ip-suite-4-15
  • /test edge-e2e-metal-assisted-static-ip-suite-4-16
  • /test edge-e2e-metal-assisted-tang
  • /test edge-e2e-metal-assisted-tpmv2
  • /test edge-e2e-metal-assisted-upgrade-agent
  • /test edge-e2e-nutanix-assisted
  • /test edge-e2e-nutanix-assisted-2workers
  • /test edge-e2e-nutanix-assisted-4-14
  • /test edge-e2e-oci-assisted
  • /test edge-e2e-oci-assisted-4-14
  • /test edge-e2e-oci-assisted-iscsi
  • /test edge-e2e-vsphere-assisted
  • /test edge-e2e-vsphere-assisted-4-14
  • /test edge-e2e-vsphere-assisted-4-15
  • /test edge-e2e-vsphere-assisted-4-16
  • /test edge-e2e-vsphere-assisted-umn
  • /test okd-scos-images
  • /test push-pr-image

Use /test all to run the following jobs that were automatically triggered:

  • pull-ci-openshift-assisted-service-master-e2e-agent-compact-ipv4
  • pull-ci-openshift-assisted-service-master-edge-ci-index
  • pull-ci-openshift-assisted-service-master-edge-e2e-ai-operator-disconnected-capi
  • pull-ci-openshift-assisted-service-master-edge-e2e-ai-operator-ztp
  • pull-ci-openshift-assisted-service-master-edge-e2e-ai-operator-ztp-capi
  • pull-ci-openshift-assisted-service-master-edge-e2e-metal-assisted
  • pull-ci-openshift-assisted-service-master-edge-images
  • pull-ci-openshift-assisted-service-master-edge-lint
  • pull-ci-openshift-assisted-service-master-edge-subsystem-aws
  • pull-ci-openshift-assisted-service-master-edge-subsystem-kubeapi-aws
  • pull-ci-openshift-assisted-service-master-edge-unit-test
  • pull-ci-openshift-assisted-service-master-edge-verify-generated-code
  • pull-ci-openshift-assisted-service-master-images
  • pull-ci-openshift-assisted-service-master-mce-images

In response to this:

/test ?

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.

@openshift-ci openshift-ci bot added api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 25, 2024
@carbonin carbonin changed the title [WIP] OCPBUGS-42151: Allow updates to InfraEnv version [WIP] ACM-14502: Allow updates to InfraEnv version Sep 25, 2024
@openshift-ci-robot openshift-ci-robot removed the jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. label Sep 25, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 25, 2024

@carbonin: This pull request references ACM-14502 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 bug to target the "4.18.0" version, but no target version was set.

In response to this:

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

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 openshift-ci-robot removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Sep 25, 2024
@carbonin
Copy link
Member Author

/test edge-subsystem-kubeapi-aws

@carbonin
Copy link
Member Author

/test edge-subsystem-aws

@carbonin
Copy link
Member Author

/test edge-subsystem-kubeapi-aws

@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 26, 2024

@carbonin: This pull request references ACM-14502 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 bug to target the "4.18.0" version, but no target version was set.

In response to this:

Allow updating the InfraEnv OpenshiftVersion in both the REST and kube APIs.

This is useful when after initial install a user wants to add a new node to a cluster, but the cluster has been upgraded between install and the scale out. Previously a user would need to create a new infraenv with identical settings to the old one and a newer version.

With this change users will be able to update the infraenv os image version just like other attributes.

For some background:

The field was added to the InfraEnv CR in #5365 for https://issues.redhat.com//browse/MGMT-14923 and I don't see any discussion there about updates.

Based on the PR that added infraenv update (#2319) the update params were originally created without the version with no discussion I could find.

Based on these it doesn't seem like this was an intentional omission, so it should be safe to add it here.

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?

Deployed assisted service kube-api, created an infraenv, updated osImageVersion several times.
Observed the URL change accordingly each time and an error in the infraenv status when a non-existent version was requested.

Additionally subsystem tests were added to cover this.

  • 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

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.

@carbonin carbonin marked this pull request as ready for review September 26, 2024 19:20
@carbonin carbonin changed the title [WIP] ACM-14502: Allow updates to InfraEnv version ACM-14502: Allow updates to InfraEnv version Sep 26, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2024
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

Attention: Patch coverage is 65.90909% with 15 lines in your changes missing coverage. Please review.

Project coverage is 68.73%. Comparing base (0fde766) to head (15d4ac1).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
internal/bminventory/inventory.go 69.04% 6 Missing and 7 partials ⚠️
...rnal/controller/controllers/infraenv_controller.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6808      +/-   ##
==========================================
- Coverage   68.73%   68.73%   -0.01%     
==========================================
  Files         249      249              
  Lines       37278    37299      +21     
==========================================
+ Hits        25624    25636      +12     
- Misses       9373     9377       +4     
- Partials     2281     2286       +5     
Flag Coverage Δ
68.73% <65.90%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...rnal/controller/controllers/infraenv_controller.go 63.89% <0.00%> (-0.24%) ⬇️
internal/bminventory/inventory.go 70.70% <69.04%> (-0.02%) ⬇️

... and 2 files with indirect coverage changes

@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 26, 2024
This function is related only towards validating the data against the
cluster rather than generally for the infraenv.

Additionally when the version is updated that should be used for
validation against the cluster.
Previously this was only validated after the new data was saved to the
database and the new image URL was being built.
@@ -4988,7 +4998,7 @@ func (b *bareMetalInventory) UpdateInfraEnvInternal(ctx context.Context, params
cluster = nil
}
}
if err = validateArchitectureAndVersion(b.versionsHandler, cluster, infraEnv.CPUArchitecture, infraEnv.OpenshiftVersion); err != nil {
if err = validateClusterArchitectureAndVersion(b.versionsHandler, cluster, infraEnv.CPUArchitecture, openshiftVersion); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if it's a late-binding infraEnv? Would we still want to validate the version with this function or does it not matter if the infraEnv isn't bound to a cluster?

Copy link
Member Author

@carbonin carbonin Sep 27, 2024

Choose a reason for hiding this comment

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

I don't think so. IIRC we don't actually require someone to have defined cluster image sets (for the kubeapi case) before defining the infraenv. I'd rather leave that validation to the cluster API rather than tie infraenv and cluster concerns more tightly together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, we don't require a cluster image set for InfraEnv creation. That makes sense. Thanks!

Copy link

openshift-ci bot commented Sep 27, 2024

@carbonin: 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/edge-subsystem-kubeapi-aws 15d4ac1 link true /test edge-subsystem-kubeapi-aws

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.

Copy link
Contributor

@CrystalChun CrystalChun 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 Indicates that a PR is ready to be merged. label Sep 30, 2024
Copy link

openshift-ci bot commented Sep 30, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, CrystalChun

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 [CrystalChun,carbonin]

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants