Skip to content

CORS-3139: Move CAPI behind infrastructure provider interface#7824

Merged
openshift-merge-bot[bot] merged 8 commits intoopenshift:masterfrom
patrickdillon:infra-interface-assets
Jan 29, 2024
Merged

CORS-3139: Move CAPI behind infrastructure provider interface#7824
openshift-merge-bot[bot] merged 8 commits intoopenshift:masterfrom
patrickdillon:infra-interface-assets

Conversation

@patrickdillon
Copy link
Contributor

@patrickdillon patrickdillon commented Dec 11, 2023

This PR:

  • moves the CAPI implementation behind the infrastructure provider interface
  • adds an interface/platform for various cloud providers to implement

Moving the CAPI implementation behind the infrastructure provider platform encapsulates CAPI implementation details. It also allows pkg/infrastructure/platform to be the canonical source of truth of which infrastructure provider a cloud platform is using.

The CAPIInfraHelper interface provides a simple implementation for cloud platforms to interact with the CAPI provisioning pattern. This is intended to be a baseline interface that can be build upon and extended. It is not intended to be complete, but intends to be flexible enough to allow handling needs that will arise across multiple cloud platforms.

@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 Dec 11, 2023
@openshift-ci openshift-ci bot requested review from bfournie and sadasu December 11, 2023 21:01
@patrickdillon
Copy link
Contributor Author

/hold
WIP

@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 Dec 12, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2023
@pierreprinetti
Copy link
Member

/test e2e-openstack-ovn

@pierreprinetti
Copy link
Member

/test e2e-openstack-ovn openstack-manifests

@pierreprinetti
Copy link
Member

/test whyareyouignoringme

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 2, 2024

@pierreprinetti: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test agent-integration-tests
  • /test altinfra-e2e-aws-ovn-upi
  • /test altinfra-images
  • /test aro-unit
  • /test e2e-agent-compact-ipv4
  • /test e2e-aws-ovn
  • /test e2e-aws-ovn-upi
  • /test e2e-azure-ovn
  • /test e2e-azure-ovn-upi
  • /test e2e-gcp-ovn
  • /test e2e-gcp-ovn-upi
  • /test e2e-metal-ipi-ovn-ipv6
  • /test e2e-openstack-ovn
  • /test e2e-vsphere-ovn
  • /test e2e-vsphere-upi
  • /test gofmt
  • /test golint
  • /test govet
  • /test images
  • /test okd-images
  • /test okd-scos-images
  • /test okd-unit
  • /test okd-verify-codegen
  • /test openstack-manifests
  • /test shellcheck
  • /test terraform-images
  • /test terraform-verify-vendor
  • /test tf-lint
  • /test unit
  • /test verify-codegen
  • /test verify-vendor
  • /test yaml-lint

The following commands are available to trigger optional jobs:

  • /test altinfra-e2e-aws-custom-security-groups
  • /test altinfra-e2e-aws-ovn
  • /test altinfra-e2e-aws-ovn-fips
  • /test altinfra-e2e-aws-ovn-imdsv2
  • /test altinfra-e2e-aws-ovn-localzones
  • /test altinfra-e2e-aws-ovn-proxy
  • /test altinfra-e2e-aws-ovn-public-subnets
  • /test altinfra-e2e-aws-ovn-shared-vpc
  • /test altinfra-e2e-aws-ovn-shared-vpc-localzones
  • /test altinfra-e2e-aws-ovn-shared-vpc-wavelengthzones
  • /test altinfra-e2e-aws-ovn-single-node
  • /test altinfra-e2e-aws-ovn-upgrade
  • /test altinfra-e2e-aws-ovn-wavelengthzones
  • /test altinfra-e2e-aws-upi-proxy
  • /test altinfra-e2e-azure-ovn
  • /test altinfra-e2e-azure-ovn-resourcegroup
  • /test altinfra-e2e-azure-ovn-shared-vpc
  • /test altinfra-e2e-vsphere-ovn
  • /test altinfra-e2e-vsphere-static-ovn
  • /test altinfra-e2e-vsphere-zones
  • /test azure-ovn-marketplace-images
  • /test e2e-agent-compact-ipv4-appliance
  • /test e2e-agent-compact-ipv4-none-platform
  • /test e2e-agent-ha-dualstack
  • /test e2e-agent-sno-ipv4-pxe
  • /test e2e-agent-sno-ipv6
  • /test e2e-alibaba
  • /test e2e-aws-custom-security-groups
  • /test e2e-aws-overlay-mtu-ovn-1200
  • /test e2e-aws-ovn-fips
  • /test e2e-aws-ovn-imdsv2
  • /test e2e-aws-ovn-localzones
  • /test e2e-aws-ovn-proxy
  • /test e2e-aws-ovn-public-subnets
  • /test e2e-aws-ovn-shared-vpc
  • /test e2e-aws-ovn-shared-vpc-localzones
  • /test e2e-aws-ovn-shared-vpc-wavelengthzones
  • /test e2e-aws-ovn-single-node
  • /test e2e-aws-ovn-upgrade
  • /test e2e-aws-ovn-wavelengthzones
  • /test e2e-aws-ovn-workers-rhel8
  • /test e2e-aws-upi-proxy
  • /test e2e-azure-ovn-resourcegroup
  • /test e2e-azure-ovn-shared-vpc
  • /test e2e-azurestack
  • /test e2e-azurestack-upi
  • /test e2e-crc
  • /test e2e-gcp-ovn-shared-vpc
  • /test e2e-gcp-ovn-xpn
  • /test e2e-gcp-secureboot
  • /test e2e-gcp-upgrade
  • /test e2e-gcp-upi-xpn
  • /test e2e-ibmcloud-ovn
  • /test e2e-libvirt
  • /test e2e-metal-assisted
  • /test e2e-metal-ipi-ovn-dualstack
  • /test e2e-metal-ipi-sdn
  • /test e2e-metal-ipi-sdn-swapped-hosts
  • /test e2e-metal-ipi-sdn-virtualmedia
  • /test e2e-metal-single-node-live-iso
  • /test e2e-nutanix-ovn
  • /test e2e-nutanix-sdn
  • /test e2e-openstack-ccpmso
  • /test e2e-openstack-ccpmso-zone
  • /test e2e-openstack-dualstack
  • /test e2e-openstack-externallb
  • /test e2e-openstack-nfv-intel
  • /test e2e-openstack-proxy
  • /test e2e-openstack-sdn-parallel
  • /test e2e-openstack-upi
  • /test e2e-vsphere-static-ovn
  • /test e2e-vsphere-upi-zones
  • /test e2e-vsphere-zones
  • /test e2e-vsphere-zones-techpreview
  • /test okd-e2e-agent-compact-ipv4
  • /test okd-e2e-agent-ha-dualstack
  • /test okd-e2e-agent-sno-ipv6
  • /test okd-e2e-aws-ovn
  • /test okd-e2e-aws-ovn-upgrade
  • /test okd-e2e-gcp
  • /test okd-e2e-gcp-ovn-upgrade
  • /test okd-e2e-vsphere
  • /test okd-scos-e2e-aws-ovn
  • /test okd-scos-e2e-aws-upgrade
  • /test okd-scos-e2e-gcp
  • /test okd-scos-e2e-gcp-ovn-upgrade
  • /test okd-scos-e2e-vsphere
  • /test okd-scos-unit
  • /test okd-scos-verify-codegen
  • /test tf-fmt

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

  • pull-ci-openshift-installer-master-altinfra-e2e-aws-custom-security-groups
  • pull-ci-openshift-installer-master-altinfra-e2e-aws-ovn-fips
  • pull-ci-openshift-installer-master-altinfra-e2e-aws-ovn-single-node
  • pull-ci-openshift-installer-master-altinfra-images
  • pull-ci-openshift-installer-master-aro-unit
  • pull-ci-openshift-installer-master-e2e-aws-custom-security-groups
  • pull-ci-openshift-installer-master-e2e-aws-ovn
  • pull-ci-openshift-installer-master-e2e-aws-ovn-fips
  • pull-ci-openshift-installer-master-e2e-aws-ovn-imdsv2
  • pull-ci-openshift-installer-master-e2e-aws-ovn-localzones
  • pull-ci-openshift-installer-master-e2e-aws-ovn-shared-vpc
  • pull-ci-openshift-installer-master-e2e-aws-ovn-shared-vpc-localzones
  • pull-ci-openshift-installer-master-e2e-aws-ovn-single-node
  • pull-ci-openshift-installer-master-gofmt
  • pull-ci-openshift-installer-master-golint
  • pull-ci-openshift-installer-master-govet
  • pull-ci-openshift-installer-master-images
  • pull-ci-openshift-installer-master-okd-e2e-aws-ovn-upgrade
  • pull-ci-openshift-installer-master-okd-images
  • pull-ci-openshift-installer-master-okd-scos-images
  • pull-ci-openshift-installer-master-okd-scos-unit
  • pull-ci-openshift-installer-master-okd-scos-verify-codegen
  • pull-ci-openshift-installer-master-okd-unit
  • pull-ci-openshift-installer-master-okd-verify-codegen
  • pull-ci-openshift-installer-master-shellcheck
  • pull-ci-openshift-installer-master-tf-fmt
  • pull-ci-openshift-installer-master-tf-lint
  • pull-ci-openshift-installer-master-unit
  • pull-ci-openshift-installer-master-verify-codegen
  • pull-ci-openshift-installer-master-verify-vendor
  • pull-ci-openshift-installer-master-yaml-lint
Details

In response to this:

/test whyareyouignoringme

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.

@pierreprinetti
Copy link
Member

uh. So if our tests are skipped because of a regexp in openshift/release, then it is not possible to trigger them manually?

@patrickdillon patrickdillon force-pushed the infra-interface-assets branch from a88ea87 to 2949dde Compare January 8, 2024 16:08
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 8, 2024
@patrickdillon patrickdillon changed the title WIP: Update infrastructure provider interface to accept asset.Parents Move CAPI behind infrastructure provider interface Jan 8, 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 Jan 8, 2024
@patrickdillon
Copy link
Contributor Author

/hold cancel

@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 Jan 8, 2024
Copy link
Contributor

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

small nits, other than that lgtm

@r4f4
Copy link
Contributor

r4f4 commented Jan 11, 2024

/retitle CORS-3139: Move CAPI behind infrastructure provider interface

@openshift-ci openshift-ci bot changed the title Move CAPI behind infrastructure provider interface CORS-3139: Move CAPI behind infrastructure provider interface Jan 11, 2024
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 11, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 11, 2024

@patrickdillon: This pull request references CORS-3139 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 story to target the "4.16.0" version, but no target version was set.

Details

In response to this:

This PR:

  • moves the CAPI implementation behind the infrastructure provider interface
  • adds an interface/platform for various cloud providers to implement

Moving the CAPI implementation behind the infrastructure provider platform encapsulates CAPI implementation details. It also allows pkg/infrastructure/platform to be the canonical source of truth of which infrastructure provider a cloud platform is using.

The CAPIInfraHelper interface provides a simple implementation for cloud platforms to interact with the CAPI provisioning pattern. This is intended to be a baseline interface that can be build upon and extended. It is not intended to be complete, but intends to be flexible enough to allow handling needs that will arise across multiple cloud platforms.

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.

@patrickdillon patrickdillon force-pushed the infra-interface-assets branch from 2949dde to 440ce68 Compare January 11, 2024 21:29
@patrickdillon
Copy link
Contributor Author

/retitle CORS-3139: Move CAPI behind infrastructure provider interface

Thanks!

c986b07 moves tfvars to its own package, which sets the linter off like crazy 440ce68 is an effort to fix the linter errors but not sure if there's a better way or if it's worth it

@r4f4
Copy link
Contributor

r4f4 commented Jan 12, 2024

c986b07 moves tfvars to its own package, which sets the linter off like crazy 440ce68 is an effort to fix the linter errors but not sure if there's a better way or if it's worth it

We could temporarily disable the linter for that whole file and solve the linting issues in a follow-up PR. TBH I think what you're doing is fine, since we don't get many opportunities to work on tech debt.

@patrickdillon
Copy link
Contributor Author

Polished, rebased and squashed the WIP from yesterday. Still need to do some final testing.

@pierreprinetti
Copy link
Member

pierreprinetti commented Jan 24, 2024

We noticed that the PreTerraform hook is executed in the CAPI flow. Is that expected? In that hook we run different infra preparation steps compared to what we do pre CAPI.

@patrickdillon patrickdillon force-pushed the infra-interface-assets branch from c279158 to 68ee163 Compare January 26, 2024 20:22
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2024
Moves tfvars to its own package to decouple it from the cluster asset.
After moving tfvars.go to its own package, the linter is kicking up a
lot of dust.
Updates the infrastructure provider interface to accept Parent assets--
rather than a list of files. This allows for easier handling of assets
by the infrastructure provider.
Moves LoadMetadata to a separate package to allow implementers of
the infrastructure provider interface to utilize the function. The
cluster asset/package does not use LoadMetadata but does depend on
the implementers of the interface. Moving to a separate package
breaks the dependency loop.
Move the path for capi manifests to capiutils and export it.
This will allow the use of the same constant when breaking
CAPI machine manifests into a separate asset.
Creates an independent asset for CAPI machines, rather than
being a part of the cluster API manifests asset. This separation
will make it easier to apply the CAPI machines in a second stage,
after the CAPI infrastructure assets.
@patrickdillon patrickdillon force-pushed the infra-interface-assets branch from 68ee163 to 462d8b7 Compare January 26, 2024 20:25
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 26, 2024
@patrickdillon
Copy link
Contributor Author

We noticed that the PreTerraform hook is executed in the CAPI flow. Is that expected? In that hook we run different infra preparation steps compared to what we do pre CAPI.

Let's discuss/handle in a separate PR.

Implements the infrastructure provider interface with the CAPI system.
This encapsulates the CAPI implementation similar to Terraform. It also
maintains pkg/infrastructure/platform.go (and build variants) as the
canonical source of truth for choosing an infrastructure provider.

This also adds an interface that cloud platforms utilizing the CAPI
provisioning should implement to provision additional resources.
@patrickdillon patrickdillon force-pushed the infra-interface-assets branch 2 times, most recently from ff0cc60 to 3cb0c98 Compare January 29, 2024 16:58
Implements basic logic for handling bootstrap destroy through
the CAPI infra provider. Further bootstrap destroy logic, including
platform-specific implementations, may be needed to ensure proper
destruction of all bootstrap resources.
@patrickdillon patrickdillon force-pushed the infra-interface-assets branch from 3cb0c98 to a03afe9 Compare January 29, 2024 18:07
Copy link
Contributor

@vincepri vincepri 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 Jan 29, 2024
@patrickdillon
Copy link
Contributor Author

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 29, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon, vincepri

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 Jan 29, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit ba9211c into openshift:master Jan 29, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 29, 2024

@patrickdillon: 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/altinfra-e2e-aws-ovn-single-node a03afe9 link false /test altinfra-e2e-aws-ovn-single-node

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.

@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-installer-altinfra-container-v4.16.0-202401300919.p0.gba9211c.assembly.stream for distgit ose-installer-altinfra.
All builds following this will include this PR.

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.