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

Implement a Devfile Registry operator #2

Merged
merged 9 commits into from
Nov 27, 2020

Conversation

johnmcollier
Copy link
Member

@johnmcollier johnmcollier commented Nov 13, 2020

What does this PR do?

Fixes devfile/api#190

Implements a DevfileRegistry Kubernetes custom resource and operator using operator-sdk v1.1, based off of the approved design proposal for such an operator.

To make reviewing easier, I've squashed my changes down to 4 commits. If you want to see the original commit history, take a look at https://github.com/johnmcollier/registry-operator.

Overview

In all, this PR does the following:

  1. Implementation of a DevfileRegistry custom resource in the registry.devfile.io API group.

    • The fields are based off of the design proposal.
    • Includes the following names/shortnames for the resource:
      • name: DevfileRegistry (case-insensitive)
      • plural: DevfileRegistries (case-insensitive)
      • shortnames: devreg, dr (case-sensitive)
  2. Implementation of an operator that manages instances of DevfileRegistry resources and deploys Devfile registries onto a Kubernetes or OpenShift cluster

    • The order in which the Devfile Registry resources are deployed are based off of the design proposal.
    • Supports updating existing DevfileRegistry resources. The default Rolling update strategy is used for the devfile registry deployment to ensure zero-downtime.
    • As well as support for deploying with TLS or persistence enabled/disabled *
  3. Simple integration test framework. Based on a pre-existing framework from the devworkspace-operator (see https://github.com/devfile/devworkspace-operator/tree/master/test/e2e).

  4. GitHub workflow actions for building and pushing the operator image up to quay.io

* Does not include support for creating secrets with cert-manager at this time, as I've found that the cert-manager API (including the necessary Go types) is not compatible with the versions of the client-go and controller-runtime packages used by operator-sdk.

Directory Structure

  • config/

    • Contains the kustomize yaml files for deploying the operator on a Kubernetes
    • These files were generated by the operator-sdk CLI. I only did some minor tweaking by hand where needed (adding labels to the operator deployment, for example.)
  • api/v1alpha1/

    • devfileregistry_types.go contans the type definition for the DevfileRegistry custom resource.
    • groupversion_info.go and zz_generated.deepcopy.go are generated/modified by the operator-sdk CLI.
  • controllers/

    • Contains the main business logic for the operator (creating/updating resources)
    • Reconcile() in devfileregistry_controller.go is where the bulk of this logic is located.
    • ensure.go ensures that the necessary resources for the devfile registry are deployed on the cluster and up-to-date
    • update.go contains the necessary functions for updating the needed devfile registry resources.
  • pkg

    • Helper functions for the operator
    • registry/ functions for generating Kube resource structs (like Deployments, Services, etc) that are then deployed by the operator
    • cluster/ and config/ contains functions for detecting if we're running on OpenShift, and storing that result
  • tests/integration

How to deploy (OpenShift):

From the root of the repository:

  1. Ensure you're logged in to an OpenShift cluster
  2. make install && make deploy`
  3. oc apply -f registry.yaml
  4. oc get dr -w

Testing

  • Integration tests (under tests/integration). Can be run with make test-integration. The following scenarios are covered:
    • Simple devfile registry deploy
    • Simple devfile registry deploy with TLS enabled
    • Updating an existing devfile registry instance
  • Some unit tests implemented in the pkg/registry package. Can be run with make test.
    • The documentation for unit testing controller-runtime code is quite sparse and good examples are near non-existent, so this is something we'll want to look into.

Implements the DevfileRegistry custom resource based on the devfile registry operator design doc. In addition to the spec, `dr` and `devreg` are added as shortnames.

Signed-off-by: John Collier <jcollier@redhat.com>
Implements the core business logic of the Devfile Registry operator, allowing registries to be deployed on Kubernetes and OpenShift.

Signed-off-by: John Collier <jcollier@redhat.com>
Implements a simple integration test framework for the Devfile Registy operator, based on the framework used for the devworkspace operator (see https://github.com/devfile/devworkspace-operator/tree/master/test/e2e)

The following functionality is tested currently:
- Simple devfile registry deploy
- Simple devfile registry deploy with TLS enabled
- Updating an existing devfile registry instance

Signed-off-by: John Collier <jcollier@redhat.com>
- Make sure IsTLSEnabled fucntion is used
- Add sample registry.yaml file

Signed-off-by: John Collier <jcollier@redhat.com>
Now that we've switched over to using an nginx proxy to expose both the index server and OCI registry, we no longer need to expose the OCI registry directly.

Signed-off-by: John Collier <jcollier@redhat.com>
Signed-off-by: John Collier <jcollier@redhat.com>
README.md Outdated Show resolved Hide resolved
config/samples/registry_v1alpha1_devfileregistry.yaml Outdated Show resolved Hide resolved
@GeekArthur
Copy link
Contributor

"DevfileRegistry custom resource" looks good to me, currently I am reviewing "Devfile Registry operator core logic "

controllers/devfileregistry_controller.go Show resolved Hide resolved
controllers/update.go Outdated Show resolved Hide resolved
controllers/update.go Outdated Show resolved Hide resolved
- Updates the out-of-date DevfileRegistry CR sample
- Removes the sample comments from fields in the DevfileRegistry custom resource definition.
- Cleans up the function to delete old PVCs and updates its logic a bit to be more clear
- Use defaults for readiness/liveness probes

Signed-off-by: John Collier <jcollier@redhat.com>
Signed-off-by: John Collier <jcollier@redhat.com>
@johnmcollier
Copy link
Member Author

@GeekArthur @elsony Thanks for reviewing! I've updated the PR based on your review comments

controllers/ensure.go Show resolved Hide resolved
controllers/ensure.go Outdated Show resolved Hide resolved
controllers/update.go Outdated Show resolved Hide resolved
@GeekArthur
Copy link
Contributor

@johnmcollier Thanks for changes! I already done the review and left some minor comments.

@GeekArthur
Copy link
Contributor

@johnmcollier For me, just change the svc/pvc typo is fine for this PR, we can leave all the refactor/improvement work for the followup PRs. I am gonna approve the PR.

Copy link
Contributor

@GeekArthur GeekArthur left a comment

Choose a reason for hiding this comment

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

LGTM!

@johnmcollier
Copy link
Member Author

johnmcollier commented Nov 27, 2020

@GeekArthur The if statement refactor's a quick change so I'm gonna update that too when I update the svc typo. Thanks!

- Fix pvc->svc typo
- Clean up if statement check on deleting pvc

Signed-off-by: John Collier <jcollier@redhat.com>
@johnmcollier
Copy link
Member Author

@GeekArthur Updated! Mind re-adding the lgtm label if you have no concerns? Thanks!

Copy link
Contributor

@GeekArthur GeekArthur left a comment

Choose a reason for hiding this comment

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

Re-add LGTM!

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: elsony, GeekArthur, johnmcollier
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

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

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