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

WIP - Add support for OpenShift cluster provider #421

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

adetalhouet
Copy link
Member

@adetalhouet adetalhouet commented Oct 20, 2023

The following PR nephio-project/nephio-example-packages#86 adds the proper RBAC to the nephio bootstrap controller

Signed-off-by: Alexis de Talhouët <adetalhouet89@gmail.com>
@nephio-prow
Copy link
Contributor

nephio-prow bot commented Oct 20, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign henderiw for approval by writing /assign @henderiw in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@nephio-prow
Copy link
Contributor

nephio-prow bot commented Oct 20, 2023

Hi @adetalhouet. Thanks for your PR.

I'm waiting for a nephio-project member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@johnbelamaric
Copy link
Member

johnbelamaric commented Oct 23, 2023

My only question here is whether this should instead be part of a separate, OpenShift set of controllers.

If you look, for example, at K8s, they started with kube controller manager that embedded cloud-specific things, just like this. But eventually they realized maintaining that was hard - you end up with a binary with many, many, many dependencies, and those dependencies are updated on different schedules for different providers. To fix this, they spent literally years splitting all that code out into separate, per cloud-provider controller managers.

My guess is we will find ourselves in a similar situation, where there are various controllers we need for each environment; those can all be bundled up into a vendor-specific controller manager where the user can run that controller-manager if they want to work with that environment. I would rather we start with the right set up for our code base, rather than a major refactor later on.

So, I would suggest:

  • If there are common bits/interfaces, they will live in the Nephio repo in a "pkg" directory.
  • The current bootstrap controller becomes the "capi" bootstrap controller. It will continue to live in the nephio-project/nephio repository.
  • We create a separate, openshift controller manager to house all of the openshift-specific code. It can use the shared interfaces and routines from the pkg directory so we don't duplicate a bunch of code.
  • The openshift controller manager will be a separate binary. I think it can still live in the "nephio-project/nephio" repo for now, but ideally it would split out into a separate openshift-specific repository (within nephio-project, ideally, so it can be part of our distribution).

In the openshift environment (either for the mgmt cluster or even just for workload clusters), users would enable the openshift-controller-manager.

WDYT?

I have a couple GCP controllers (well, one that I use, but there are others) that are part of Porch right now. Ultimately I want to do the same with those.

@johnbelamaric
Copy link
Member

/ok-to-test

val, ok := secret.Labels["hive.openshift.io/secret-type"]
if ok && val == kubeconfig {
return &openshift.OpenShift{Client: r.Client, Secret: secret}, true
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there another approach here rather than hard coding the Openshift specific call? Perhaps a way of configuring this so that the label and the function to call is read from config/metadata would be a better approach.

@efiacor
Copy link
Contributor

efiacor commented Jan 15, 2024

Can we get a status update on this PR @adetalhouet ?

@adetalhouet
Copy link
Member Author

Can we get a status update on this PR @adetalhouet ?

What kind of update would you like? I don't have the time to test this between now and mid-Feb. So if this is a gating factor, then discard this PR for R2. I tested this back then when I submitted the PR. I'll have some time after mid-Feb.

@efiacor
Copy link
Contributor

efiacor commented Jan 15, 2024

Can we get a status update on this PR @adetalhouet ?

What kind of update would you like? I don't have the time to test this between now and mid-Feb. So if this is a gating factor, then discard this PR for R2. I tested this back then when I submitted the PR. I'll have some time after mid-Feb.

Hi @adetalhouet , I was just trying to get some movement on the outstanding PRs really. Support for alternative cluster provisioning is definitely needed so maybe we mark as WIP and revisit in R-3

@adetalhouet
Copy link
Member Author

Can we get a status update on this PR @adetalhouet ?

What kind of update would you like? I don't have the time to test this between now and mid-Feb. So if this is a gating factor, then discard this PR for R2. I tested this back then when I submitted the PR. I'll have some time after mid-Feb.

Hi @adetalhouet , I was just trying to get some movement on the outstanding PRs really. Support for alternative cluster provisioning is definitely needed so maybe we mark as WIP and revisit in R-3

OpenShift cluster provisioning works without this. This PR is simply to use the bootstrap sequence defined in Nephio, with the ultimate goal to install configsync and being able to then sync anything from there.
OpenShift portfolio has other means to do so that will be leveraged if this can't be merged.

Given the tight timeline for R2 and the actual bandwidth I have, I doubt I can revisit this. I'll mark as WIP and put in draft for now.

@adetalhouet adetalhouet changed the title Add support for OpenShift cluster provider WIP - Add support for OpenShift cluster provider Jan 15, 2024
@adetalhouet adetalhouet marked this pull request as draft January 15, 2024 19:30
@efiacor
Copy link
Contributor

efiacor commented Jan 15, 2024

Can we get a status update on this PR @adetalhouet ?

What kind of update would you like? I don't have the time to test this between now and mid-Feb. So if this is a gating factor, then discard this PR for R2. I tested this back then when I submitted the PR. I'll have some time after mid-Feb.

Hi @adetalhouet , I was just trying to get some movement on the outstanding PRs really. Support for alternative cluster provisioning is definitely needed so maybe we mark as WIP and revisit in R-3

OpenShift cluster provisioning works without this. This PR is simply to use the bootstrap sequence defined in Nephio, with the ultimate goal to install configsync and being able to then sync anything from there. OpenShift portfolio has other means to do so that will be leveraged if this can't be merged.

Given the tight timeline for R2 and the actual bandwidth I have, I doubt I can revisit this. I'll mark as WIP and put in draft for now.

Thank you sir :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants