MGMT-14923: add OSImageVersion to InfraEnvSpec #5365

danielerez · 2023-07-18T08:03:59Z

Introduced a new property to InfraEnvSpec: OSImageVersion.
The property can be used for creating an InfraEnv through kube-api with a specific OS image version.

The OSImageVersion is useful for HyperShift flows which use late binding, hence, the InfraEnv isn't created according to a cluster version (thus, the latest OS image is used as a fallback).

Notes:

The specified version should refer to an OSImage from the list in AgentServiceConfig, otherwise the condition is set to failure.
When a ClusterRef is provided, we still use the cluster's version for creating the InfraEnv. Thus, The property should not be provided along with OSImageVersion to avoid a conflict (validated with a WebHook).

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

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-robot · 2023-07-18T08:04:03Z

openshift-ci · 2023-07-18T08:04:04Z

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

danielerez · 2023-07-18T08:04:18Z

/test all

openshift-ci · 2023-07-18T08:05:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielerez

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~~ [danielerez]

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

codecov · 2023-07-18T08:33:25Z

Codecov Report

Merging #5365 (666022c) into master (6fb0883) will increase coverage by 0.07%.
The diff coverage is 84.90%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5365      +/-   ##
==========================================
+ Coverage   67.59%   67.66%   +0.07%     
==========================================
  Files         226      226              
  Lines       33304    33349      +45     
==========================================
+ Hits        22511    22567      +56     
+ Misses       8759     8746      -13     
- Partials     2034     2036       +2

Impacted Files	Coverage Δ
...ks/agentinstall/v1beta1/infraenv_admission_hook.go	`86.13% <75.00%> (-3.40%)`	⬇️
...rnal/controller/controllers/infraenv_controller.go	`67.93% <100.00%> (+4.35%)`	⬆️

... and 1 file with indirect coverage changes

filanov · 2023-07-18T10:16:36Z

/assign @avishayt
/assign @carbonin

carbonin

Can you also ensure there is a test for the case where the cluster is set?

internal/controller/controllers/infraenv_controller_test.go

danielerez · 2023-07-18T17:14:07Z

Can you also ensure there is a test for the case where the cluster is set?

Actually didn't find something specific for this flow, added.

carbonin · 2023-07-18T19:32:24Z

Looks good, any reason it's still in draft?

danielerez · 2023-07-19T09:35:19Z

Looks good, any reason it's still in draft?

Not really:) Moved to ready for review.

openshift-ci-robot · 2023-07-19T10:25:35Z

carbonin · 2023-07-19T17:56:17Z

Looks good to me, but I'll give @avishayt a chance to 👍 the API

Introduced a new property to InfraEnvSpec: 'OSImageVersion'. The property can be used for creating an InfraEnv through kube-api with a specific OS image version. The 'OSImageVersion' is useful for HyperShift flows which use late binding, hence, the InfraEnv isn't created according to a cluster version (thus latest OS image is used as a fallback). Notes: * The specified version should refer to an OSImage from the list in AgentServiceConfig, otherwise the condition is set to failure. * When a ClusterRef is provided, we still use the cluster's version for creating the InfraEnv. Thus, The property should not be provided along with OSImageVersion to avoid a conflict (validated with a WebHook).

openshift-ci · 2023-07-20T00:02:30Z

@danielerez: The following tests 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-e2e-ai-operator-ztp-capi	`666022c`	link	false	`/test edge-e2e-ai-operator-ztp-capi`
ci/prow/edge-e2e-ai-operator-ztp-hypershift-zero-nodes	`666022c`	link	false	`/test edge-e2e-ai-operator-ztp-hypershift-zero-nodes`

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/test-infra repository. I understand the commands that are listed here.

avishayt · 2023-07-23T12:25:12Z

/lgtm

Introduced a new property to InfraEnvSpec: 'OSImageVersion'. The property can be used for creating an InfraEnv through kube-api with a specific OS image version. The 'OSImageVersion' is useful for HyperShift flows which use late binding, hence, the InfraEnv isn't created according to a cluster version (thus latest OS image is used as a fallback). Notes: * The specified version should refer to an OSImage from the list in AgentServiceConfig, otherwise the condition is set to failure. * When a ClusterRef is provided, we still use the cluster's version for creating the InfraEnv. Thus, The property should not be provided along with OSImageVersion to avoid a conflict (validated with a WebHook).

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 18, 2023

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 Jul 18, 2023

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 18, 2023

openshift-ci bot added the api-review Categorizes an issue or PR as actively needing an API review. label Jul 18, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2023

danielerez force-pushed the infraenvspec_osimageversion branch from 529fce7 to fbcb56a Compare July 18, 2023 09:19

openshift-ci bot assigned avishayt and carbonin Jul 18, 2023

carbonin reviewed Jul 18, 2023

View reviewed changes

internal/controller/controllers/infraenv_controller_test.go Outdated Show resolved Hide resolved

internal/controller/controllers/infraenv_controller_test.go Outdated Show resolved Hide resolved

internal/controller/controllers/infraenv_controller_test.go Outdated Show resolved Hide resolved

danielerez force-pushed the infraenvspec_osimageversion branch from fbcb56a to 48a8f5e Compare July 18, 2023 17:14

danielerez marked this pull request as ready for review July 19, 2023 09:35

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 Jul 19, 2023

openshift-ci bot requested review from jhernand and tsorya July 19, 2023 09:35

danielerez force-pushed the infraenvspec_osimageversion branch 2 times, most recently from f0a1e0d to 61ec6da Compare July 19, 2023 16:38

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2023

danielerez force-pushed the infraenvspec_osimageversion branch from 61ec6da to 666022c Compare July 19, 2023 20:36

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2023

openshift-merge-robot merged commit 36d3543 into openshift:master Jul 23, 2023
15 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-14923: add OSImageVersion to InfraEnvSpec #5365

MGMT-14923: add OSImageVersion to InfraEnvSpec #5365

danielerez commented Jul 18, 2023 •

edited

openshift-ci-robot commented Jul 18, 2023 •

edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Jul 18, 2023

danielerez commented Jul 18, 2023

openshift-ci bot commented Jul 18, 2023

codecov bot commented Jul 18, 2023 •

edited

filanov commented Jul 18, 2023

carbonin left a comment

danielerez commented Jul 18, 2023

carbonin commented Jul 18, 2023

danielerez commented Jul 19, 2023

openshift-ci-robot commented Jul 19, 2023 •

edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

carbonin commented Jul 19, 2023

openshift-ci bot commented Jul 20, 2023

avishayt commented Jul 23, 2023

MGMT-14923: add OSImageVersion to InfraEnvSpec #5365

MGMT-14923: add OSImageVersion to InfraEnvSpec #5365

Conversation

danielerez commented Jul 18, 2023 • edited

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Jul 18, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Jul 18, 2023

danielerez commented Jul 18, 2023

openshift-ci bot commented Jul 18, 2023

codecov bot commented Jul 18, 2023 • edited

Codecov Report

filanov commented Jul 18, 2023

carbonin left a comment

Choose a reason for hiding this comment

danielerez commented Jul 18, 2023

carbonin commented Jul 18, 2023

danielerez commented Jul 19, 2023

openshift-ci-robot commented Jul 19, 2023 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

carbonin commented Jul 19, 2023

openshift-ci bot commented Jul 20, 2023

avishayt commented Jul 23, 2023

danielerez commented Jul 18, 2023 •

edited

openshift-ci-robot commented Jul 18, 2023 •

edited by openshift-ci bot

codecov bot commented Jul 18, 2023 •

edited

openshift-ci-robot commented Jul 19, 2023 •

edited by openshift-ci bot