Skip to content

OCPCLOUD-1910: Installing Cluster API components in OpenShift#1461

Merged
openshift-ci[bot] merged 1 commit into
openshift:masterfrom
damdo:add-cluster-api-install
Oct 12, 2023
Merged

OCPCLOUD-1910: Installing Cluster API components in OpenShift#1461
openshift-ci[bot] merged 1 commit into
openshift:masterfrom
damdo:add-cluster-api-install

Conversation

@damdo

@damdo damdo commented Aug 25, 2023

Copy link
Copy Markdown
Member

This enhancement describes how we (the Cluster Infrastructure team) intend to rework the current flow for the Installation of Cluster API components in OpenShift, at the moment only available in Tech Preview, by addressing some of the criticalities of the current implementation, leveraging some of the lessons learned since the first Tech Preview release.

The focus areas of this refactor are mainly around reducing complexity of the architecture and improving the development, stability and maintainability of Standalone Cluster API on OpenShift by preventing payload breakage due to non atomic repository merges, while maintaining the functionality provided up until now.

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

openshift-ci-robot commented Aug 25, 2023

Copy link
Copy Markdown

@damdo: This pull request references OCPCLOUD-1910 which is a valid jira issue.

Details

In response to this:

This enhancement describes how we (the Cluster Infrastructure team) intend to rework the current Cluster API Install workflow, at the moment only available in Tech Preview, by addressing some of the criticalities of the current implementation, leveraging some of the lessons learned since the first Tech Preview release.

The focus areas of this refactor are mainly around reducing complexity of the architecture and improving the development, stability and maintainability of Standalone Cluster API on OpenShift by preventing payload breakage due to non atomic repository merges, while maintaining the functionality provided up until now.

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.

@openshift-ci openshift-ci Bot requested review from lmzuccarelli and mrunalp August 25, 2023 11:13
@damdo damdo force-pushed the add-cluster-api-install branch 4 times, most recently from 7cf5994 to f073388 Compare August 25, 2023 14:07

@JoelSpeed JoelSpeed left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have left a few comments throughout and I like the direction this is going, simplifying the process is great.

However, I'm struggling right now to grasp what the end result is. This document outlines the changes that we want to make, but doesn't give me a comprehensive detail on what the end flow for a provider is, which I think would be really helpful. I would probably expect the main focus of the doc to be the end goal with the changes as additional context.

I think it would help if we had something that goes into the detail of:

  • Provider creates repository in payload
  • Provider runs manifest-gen which does XYZ (creates a configmap with a specific annotation?)
  • Provider includes manifest-gen output in image
  • Operator reads configmaps based on ... (format and specific detail missing from this right now)
  • Operator selects correct platform and applies correct configmaps (I'm not sure this is even correct, how does it know from a configmap which platform the manifest should be applied onto?)

I suspect there's also a bunch of annotation based APIs going into this, these contracts should be defined here too

Comment thread enhancements/cluster-api/cluster-api-install-redesign.md
Comment thread enhancements/cluster-api/cluster-api-install-redesign.md Outdated
Comment thread enhancements/cluster-api/cluster-api-install-redesign.md
Comment thread enhancements/cluster-api/cluster-api-install-redesign.md Outdated
Comment thread enhancements/cluster-api/cluster-api-install-redesign.md Outdated
Comment thread enhancements/cluster-api/cluster-api-install-redesign.md Outdated
Comment thread enhancements/cluster-api/cluster-api-install-redesign.md Outdated
Comment thread enhancements/cluster-api/cluster-api-install-redesign.md Outdated
Comment thread enhancements/cluster-api/cluster-api-install-redesign.md Outdated
Comment thread enhancements/cluster-api/cluster-api-install-redesign.md Outdated
@damdo damdo changed the title OCPCLOUD-1910: Cluster API Install redesign OCPCLOUD-1910: Installing Cluster API components in OpenShift Aug 31, 2023
@openshift-ci-robot

openshift-ci-robot commented Aug 31, 2023

Copy link
Copy Markdown

@damdo: This pull request references OCPCLOUD-1910 which is a valid jira issue.

Details

In response to this:

This enhancement describes how we (the Cluster Infrastructure team) intend to rework the current flow for the Installation of Cluster API components in OpenShift, at the moment only available in Tech Preview, by addressing some of the criticalities of the current implementation, leveraging some of the lessons learned since the first Tech Preview release.

The focus areas of this refactor are mainly around reducing complexity of the architecture and improving the development, stability and maintainability of Standalone Cluster API on OpenShift by preventing payload breakage due to non atomic repository merges, while maintaining the functionality provided up until now.

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.

@damdo damdo force-pushed the add-cluster-api-install branch from c17a94f to a701d8e Compare September 4, 2023 16:04
@mdbooth

mdbooth commented Sep 14, 2023

Copy link
Copy Markdown
Contributor

Re the CRDs discussion: it occurred to me that we could simply include the CRDs with the other infrastructure-components we're putting in the provider ConfigMap, which is how upstream CAPI providers do this. However, upstream also includes RBAC rules in the same bundle. We could also include the RBAC rules, but that would require us to give cluster-capi-operator sufficient privileges to deploy them, which I think may be frowned upon.

@JoelSpeed

Copy link
Copy Markdown
Contributor

Re the CRDs discussion: it occurred to me that we could simply include the CRDs with the other infrastructure-components we're putting in the provider ConfigMap, which is how upstream CAPI providers do this. However, upstream also includes RBAC rules in the same bundle. We could also include the RBAC rules, but that would require us to give cluster-capi-operator sufficient privileges to deploy them, which I think may be frowned upon.

I think this is good resource for a conversation with OTA, "please handle platform specific manifests so we don't have to have RBAC to deploy all the things (CRDs and other RBAC)"

Comment thread enhancements/cluster-api/installing-cluster-api-components-in-ocp.md Outdated
Comment thread enhancements/cluster-api/installing-cluster-api-components-in-ocp.md Outdated
Comment thread enhancements/cluster-api/installing-cluster-api-components-in-ocp.md Outdated
@damdo

damdo commented Sep 25, 2023

Copy link
Copy Markdown
Member Author

Also cc. @sdodson just an FYI on the design we are doing here.

@JoelSpeed

Copy link
Copy Markdown
Contributor

I think I'm happy enough with this now, is there any major outstanding feedback to be addressed?

@sdodson

sdodson commented Sep 26, 2023

Copy link
Copy Markdown
Member

I think this is good resource for a conversation with OTA, "please handle platform specific manifests so we don't have to have RBAC to deploy all the things (CRDs and other RBAC)"

@JoelSpeed Is this still an outstanding point of discussion or is there agreement that cluster-capi-operator will take care of this?

@JoelSpeed

Copy link
Copy Markdown
Contributor

@JoelSpeed Is this still an outstanding point of discussion or is there agreement that cluster-capi-operator will take care of this?

I think we have landed on, cluster-capi-operator will handle this but we plan to have a dedicated pod to run the installation. All other controllers related to running in CAPI will run in a separate pod to provide isolation from the highly privileged credentials.

We believe in the future there will be a want from users to be able to install more than one provider. To do that, we either need to teach CVO how to apply for multiple platforms, or, handle this ourselves. We expect if this is as we think, and folk will want to be able to bootstrap openshift clusters on various platforms, from a single management cluster, it makes more sense to just keep that within the cluster-capi-operator, as they won't need other platform specific manifests that the CVO would apply, eg CCM or CSI

@sdodson

sdodson commented Sep 29, 2023

Copy link
Copy Markdown
Member

/approve

@openshift-ci

openshift-ci Bot commented Sep 29, 2023

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sdodson

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 Sep 29, 2023
@openshift-ci-robot

openshift-ci-robot commented Sep 29, 2023

Copy link
Copy Markdown

@damdo: This pull request references OCPCLOUD-1910 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 spike to target the "4.15.0" version, but no target version was set.

Details

In response to this:

This enhancement describes how we (the Cluster Infrastructure team) intend to rework the current flow for the Installation of Cluster API components in OpenShift, at the moment only available in Tech Preview, by addressing some of the criticalities of the current implementation, leveraging some of the lessons learned since the first Tech Preview release.

The focus areas of this refactor are mainly around reducing complexity of the architecture and improving the development, stability and maintainability of Standalone Cluster API on OpenShift by preventing payload breakage due to non atomic repository merges, while maintaining the functionality provided up until now.

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.

@damdo damdo force-pushed the add-cluster-api-install branch from d4698e8 to ad4a07f Compare October 12, 2023 10:20
@damdo damdo force-pushed the add-cluster-api-install branch from ad4a07f to 1031235 Compare October 12, 2023 10:24
@openshift-ci

openshift-ci Bot commented Oct 12, 2023

Copy link
Copy Markdown
Contributor

@damdo: all tests passed!

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.

@JoelSpeed

Copy link
Copy Markdown
Contributor

Thanks for your work on this @damdo @nrb
/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2023
@openshift-ci openshift-ci Bot merged commit ebc0646 into openshift:master Oct 12, 2023
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.

7 participants