Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Replaces the Aggregated API Server with the CustomResourceDefinitions (CRDs) solution #2630

Merged
merged 24 commits into from
Sep 20, 2019

Conversation

mszostok
Copy link
Contributor

This PR is a

  • Feature Implementation
  • Bug Fix
  • Documentation

What this PR does / why we need it:

This PR replaces the Aggregated API Server with the CustomResourceDefinitions (CRDs) solution.

How changes were tested

  • make format, make verify, make test-unit were executed and passed with success
  • The E2E tests from the v0.2.0 tag were executed and all 5 scenarios passed with success
  • The E2E tests from our Kyma platform were executed and passed with success.
  • Manual tests were executed against the minikube and GKE cluster installation.
    Scenarios:
    • creating/deleting cluster and namespace scoped brokers (with basic auth, with bearer auth, without auth)
    • provisioning/deprovisioning instances from cluster and namespace scoped brokers
    • binding/unbinding

All of those steps can be easily reproduced just by checkout this branch.

Here is the Cookbook for CRDs which provides scripts which you can use to easily bootstrap the minikube with the Service Catalog for the testing purpose.


During the testing phase, such issues were found on the master branch:

Overview of changes

The overview can be found in this doc.

During the KubeCon Barcelona we can do the f2f introduction (or even a deep dive session) about the CRD implementation.

Actions required to going towards with CRDs

Which issues this PR solves:

  • Create Custom Resource Definitions on Startup #1087

    (..)we'll need to block the health check until the resources have registered and are gettable.

    The common way for CRD healthcheck was implemented and used both in controller and webhook servers, logs:

    I0519 07:26:10.127589       1 readiness_probe.go:102] CRD "clusterservicebrokers.servicecatalog.k8s.io" is ready
    I0519 07:26:10.127698       1 readiness_probe.go:102] CRD "clusterserviceclasses.servicecatalog.k8s.io" is ready
    I0519 07:26:10.128033       1 readiness_probe.go:102] CRD "clusterserviceplans.servicecatalog.k8s.io" is ready
    I0519 07:26:10.128089       1 readiness_probe.go:102] CRD "servicebindings.servicecatalog.k8s.io" is ready
    I0519 07:26:10.128125       1 readiness_probe.go:102] CRD "servicebrokers.servicecatalog.k8s.io" is ready
    I0519 07:26:10.128142       1 readiness_probe.go:102] CRD "serviceclasses.servicecatalog.k8s.io" is ready
    I0519 07:26:10.128163       1 readiness_probe.go:102] CRD "serviceinstances.servicecatalog.k8s.io" is ready
    I0519 07:26:10.128178       1 readiness_probe.go:102] CRD "serviceplans.servicecatalog.k8s.io" is ready
    I0519 07:26:10.128212       1 readiness_probe.go:111] Readiness probe ready-CRDs checked. There are 8 CRDs
    
  • Implement Custom Resource Definitions storage backend #1088

  • Prototype Moving Service Catalog to CRD #2191

    (..) migrate Service Catalog from using an Aggregated API server toward using CRDs. We should do a prototype of this transition and review it with the larger SIG group.

    CRD approach is implemented and ready for review.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
  • Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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.

@k8s-ci-robot
Copy link
Contributor

Hi @mszostok. Thanks for your PR.

I'm waiting for a kubernetes-incubator or kubernetes 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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 19, 2019
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 19, 2019
@adamwalach
Copy link
Contributor

I signed it

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 20, 2019
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 4, 2019
@jberkhahn
Copy link
Contributor

/lgtm
this is just a test for the new repo

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2019
@jberkhahn jberkhahn removed the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 12, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mszostok

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2019
@mszostok
Copy link
Contributor Author

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 18, 2019
@mszostok
Copy link
Contributor Author

it's ok to review, but I've added hold to ensure that this pr will not be merged by mistake

/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 18, 2019
- remove the contrib/hack/crd folder
- remove reference to Kyma project
- rebase with current master
- restore the image in chart
- extract CRDs defintion to dedicated folder
@mszostok
Copy link
Contributor Author

@jberkhahn PTAL I've applied changes after review:

  • removed reference to Kyma project
  • rebased with current master
  • restored the SC image in chart
  • moved CRDs definition to a dedicated folder crds
  • removed the contrib/hack/crd folder

@jberkhahn
Copy link
Contributor

So, this looks good to me, but I think we need to figure out our Helm chart situation before we merge this.

@jberkhahn jberkhahn removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2019
@jberkhahn
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2019
@jberkhahn
Copy link
Contributor

/test pull-build-all-images-for-arm

@k8s-ci-robot k8s-ci-robot merged commit ccab523 into kubernetes-retired:master Sep 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants