Skip to content

AGENT-857: Agent day2 use clusterinfo#8009

Merged
openshift-merge-bot[bot] merged 19 commits intoopenshift:masterfrom
andfasano:agent-day2-use-clusterinfo
Mar 27, 2024
Merged

AGENT-857: Agent day2 use clusterinfo#8009
openshift-merge-bot[bot] merged 19 commits intoopenshift:masterfrom
andfasano:agent-day2-use-clusterinfo

Conversation

@andfasano
Copy link
Contributor

@andfasano andfasano commented Feb 13, 2024

This patch follows #7997, and starts injecting the ClusterInfo into the agent assets graph. Mainly, the various assets - depending on the current workflow set - will grab the required inputs either from the OptionalInstallConfig or ClusterInfo assets.
So, this patch prepares the ground properly to allow modifying the Ignition asset in a subsequent PR.

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

openshift-ci-robot commented Feb 13, 2024

@andfasano: This pull request references AGENT-857 which is a valid jira issue.

Details

In response to this:

/draft

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2024
@andfasano andfasano force-pushed the agent-day2-use-clusterinfo branch from 61765f2 to d6e5b2b Compare February 13, 2024 16:30
@openshift-ci openshift-ci bot requested review from bfournie and sadasu February 13, 2024 16:36
@andfasano andfasano force-pushed the agent-day2-use-clusterinfo branch 3 times, most recently from 6cfa3c5 to ebc9aa3 Compare February 15, 2024 14:50
@andfasano andfasano force-pushed the agent-day2-use-clusterinfo branch 7 times, most recently from 8b9ea4e to c8c31ce Compare February 19, 2024 16:00
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 20, 2024

@andfasano: This pull request references AGENT-857 which is a valid jira issue.

Details

In response to this:

This patch follows #7997, and starts injecting the ClusterInfo into the agent assets graph. Mainly, the various assets - depending on the current workflow set - will grab the required inputs either from the OptionalInstallConfig or ClusterInfo assets.
So, this patch prepares the ground properly to allow modifying the Ignition asset in a subsequent PR.

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.

@andfasano andfasano changed the title [WIP] AGENT-857: Agent day2 use clusterinfo AGENT-857: Agent day2 use clusterinfo Feb 20, 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 Feb 20, 2024
@andfasano andfasano force-pushed the agent-day2-use-clusterinfo branch from 99488f4 to b561aba Compare February 20, 2024 17:44
@rwsu
Copy link
Contributor

rwsu commented Feb 26, 2024

/test e2e-aws-ovn-edge-zones-manifest-validation

Copy link
Contributor

@rwsu rwsu left a comment

Choose a reason for hiding this comment

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

Small spelling issue.

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rwsu

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:

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 Feb 26, 2024
@andfasano andfasano force-pushed the agent-day2-use-clusterinfo branch from cb42b4a to 0f029ff Compare February 27, 2024 09:08
@andfasano andfasano force-pushed the agent-day2-use-clusterinfo branch from 0f029ff to 6912900 Compare March 1, 2024 18:02
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 1, 2024

@andfasano: 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/e2e-gcp-ovn-xpn 8b9ea4eea3ab85fbb4b94e8fb25130e6068606c4 link false /test e2e-gcp-ovn-xpn
ci/prow/e2e-vsphere-zones-techpreview 8b9ea4eea3ab85fbb4b94e8fb25130e6068606c4 link false /test e2e-vsphere-zones-techpreview
ci/prow/e2e-gcp-ovn-shared-vpc 8b9ea4eea3ab85fbb4b94e8fb25130e6068606c4 link false /test e2e-gcp-ovn-shared-vpc
ci/prow/e2e-agent-compact-ipv4-appliance-diskimage 6912900 link false /test e2e-agent-compact-ipv4-appliance-diskimage
ci/prow/e2e-agent-compact-ipv4-appliance 6912900 link false /test e2e-agent-compact-ipv4-appliance
ci/prow/okd-e2e-aws-ovn-upgrade 6912900 link false /test okd-e2e-aws-ovn-upgrade
ci/prow/okd-e2e-agent-compact-ipv4 6912900 link false /test okd-e2e-agent-compact-ipv4

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.

ImageDigestSources []types.ImageDigestSource
DeprecatedImageContentSources []types.ImageContentSource
PlatformType hiveext.PlatformType
SSHKey string
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to add InstallConfig fields like CPUPartitioning,FIPS, or any of the Networking fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question @bfournie. @zaneb @rwsu @pawanpinjarkar what are your thoughts about it? I'd say that in this first iteration we could try to address just the min fields required, and eventually add them later in successive iterations

Copy link
Member

Choose a reason for hiding this comment

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

OK, with the caveat that we can't get beyond Dev Preview without this stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a first iteration I meant the current PR. Since there are others still pending based on this one (#8080 and #8093), and others will follow as part the owner story AGENT-850, my preference would be to land the current PR as it is and address such points in successive PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bfournie from a first pass it looks like the mentioned settings are cluster dependent, so anything that could be set directly on the node. During the joining process I'd expect the fetched ignition would contain any necessary bits, I will verify for the case of FIPS for example.

@bfournie
Copy link
Contributor

bfournie commented Mar 8, 2024

Looks reasonable to me, just one question which may be covered in follow-on.

namespace: cluster0
spec:
cpuArchitecture: x86_64
ipxeScriptType: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

The usage of ipxeScriptType is unclear to me. The only possible values in assisted are DiscoveryImageAlways and BootOrderControl. How is the agent installer using these values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I re-checked, and the test changes were triggered by the refactoring/changes in infraenv.go

@andfasano
Copy link
Contributor Author

/retest-required

@bfournie
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2024
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD ca71358 and 2 for PR HEAD 6912900 in total

@openshift-merge-bot openshift-merge-bot bot merged commit 4e5fe0d into openshift:master Mar 27, 2024
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants