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

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

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

damdo
Copy link
Member

@damdo damdo commented Aug 25, 2023

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
Copy link

openshift-ci-robot commented Aug 25, 2023

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

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.

@damdo damdo force-pushed the add-cluster-api-install branch 4 times, most recently from 7cf5994 to f073388 Compare August 25, 2023 14:07
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

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

enhancements/cluster-api/cluster-api-install-redesign.md Outdated Show resolved Hide resolved
enhancements/cluster-api/cluster-api-install-redesign.md Outdated Show resolved Hide resolved
enhancements/cluster-api/cluster-api-install-redesign.md Outdated Show resolved Hide resolved
enhancements/cluster-api/cluster-api-install-redesign.md Outdated Show resolved Hide resolved
enhancements/cluster-api/cluster-api-install-redesign.md Outdated Show resolved Hide resolved
enhancements/cluster-api/cluster-api-install-redesign.md Outdated Show resolved Hide resolved
enhancements/cluster-api/cluster-api-install-redesign.md Outdated Show resolved Hide resolved
enhancements/cluster-api/cluster-api-install-redesign.md Outdated Show resolved Hide resolved
enhancements/cluster-api/cluster-api-install-redesign.md Outdated Show resolved Hide resolved
enhancements/cluster-api/cluster-api-install-redesign.md Outdated Show resolved Hide resolved
@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
Copy link

openshift-ci-robot commented Aug 31, 2023

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

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.

@mdbooth
Copy link
Contributor

mdbooth commented Sep 14, 2023

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
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)"

@damdo
Copy link
Member Author

damdo commented Sep 25, 2023

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

@JoelSpeed
Copy link
Contributor

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

@sdodson
Copy link
Member

sdodson commented Sep 26, 2023

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
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
Copy link
Member

sdodson commented Sep 29, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 29, 2023

[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

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
Copy link

openshift-ci-robot commented Sep 29, 2023

@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.

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 12, 2023

@damdo: all tests passed!

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.

@JoelSpeed
Copy link
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
2 checks passed
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.

None yet

7 participants