Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

chore: refactor core functionality to be more extensible #673

Merged
merged 71 commits into from
Jan 22, 2019

Conversation

MatousJobanek
Copy link
Contributor

@MatousJobanek MatousJobanek commented Nov 1, 2018

This PR refactors the core logic of the tenant service that is mainly placed in openshift and controller packages. As part of it it also changes the way the update and delete is done.
For a better understanding of the changes (mainly in the openshift package), I would recommend to fetch this PR and follow the code starting from main.go or controller package, not checking the changes in GH web UI as most of the code is totally different.

Why this PR

In the original code there were code duplications; several complicated if-else statements to handle exceptions in the behavior; inconsistent way in creating/updating/deleting objects in OS, etc. It was really hard to extend the logic by new features and make it ready for OSIO on OSD

Design

The new design I'm proposing in this PR encapsulates the logic in objects, which means that every environment type or action performs what is needed without any complicated if-else statements. The approach could be understood as:
"We need to perform an action(eg. POST) on an environment type (eg. Che) for a particular tenant. To do this, we need to apply a list of template objects (specific for the type) using OpenShift endpoints. Every object (eg. projectrequest, rolebinding, ...) has a different endpoint and for every HTTP method may have different strategy for handling the request or possible errors."
The new and most important objects proposed in the PR are:

  • openshift.NamespaceAction - Represents the action that should be applied on the namespaces for the particular tenant - [post|update|delete]. It is mainly responsible for operation on DB and provides additional information specific to the action that is needed by other objects
  • cluster.ForType - it is not a object but a function that returns cluster for environment type. This is kind of preparation for the multicluster environment - in other words, it represents cluster mapping per env-type.
  • openshift.EnvironmentTypeService - Represents service operating with information related to environment types(template, objects, cluster,...). It is responsible for getting and filtering objects to be applied, provides information (eg. needed token) specific for the particular type and performs after-apply-callback (needed by user namespace)
  • openshift.ObjectEndpoints - is list of MethodDefinitions for a particular object endpoint (eg. /oapi/v1/projectrequests). In other words, is saying which methods (Post/Delete/Get/Patch) are allowed to be performed for the endpoint
  • openshift.MethodDefinition - represents defined actions (beforeDoCallbacks, afterDoCallbacks,requestCreator) to be executed when the method is performed for an endpoint.
  • openshift.Service - knowing which action is requested it starts for every environment type a new goroutine. The goroutine gets template objects to be applied and the target cluster and then sends a request for every object to the OS cluster.

The flow of every action

The flow of every action is described in comments of every action - the resulting flow should reflect the original implementation

Benefits of the changes

  • since the specific logic is encapsulated inside of the objects, then it is much easier eg. to introduce any new environment (test, custom) by just adding a new implementation of the type
  • using the HTTP requests for all actions (update and delete) instead of oc commands gives us a possibility to easily handle errors (which is badly needed for PVC that are in terminating state for a long time) and should be also faster
  • since the method ApplyAll(nsTypes ...environment.Type) takes any number of env-types it prepares the logic for lazy initialization of single namespace. For the lazy-init also the function filterMissingAndExisting(namespaces []*tenant.Namespace) will be very useful
  • the cluster.ForType mapping prepares the code for multi-cluster environment
  • it will be much easier to introduce support for contributors in OSD spaces (creation of namespaces in spaces on OSD cluster)

Fixes #678

tenant/repository_test.go Outdated Show resolved Hide resolved
tenant/repository_test.go Outdated Show resolved Hide resolved
utils/url.go Outdated Show resolved Hide resolved
utils/url.go Outdated Show resolved Hide resolved
openshift/action.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 1, 2018

Codecov Report

Merging #673 into master will increase coverage by 13.11%.
The diff coverage is 80.57%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #673       +/-   ##
===========================================
+ Coverage   61.51%   74.62%   +13.11%     
===========================================
  Files          34       37        +3     
  Lines        2614     2893      +279     
===========================================
+ Hits         1608     2159      +551     
+ Misses        839      555      -284     
- Partials      167      179       +12
Impacted Files Coverage Δ
auth/service.go 73.73% <ø> (+3.68%) ⬆️
controller/update.go 69.14% <ø> (ø) ⬆️
utils/error.go 100% <100%> (ø)
controller/convert.go 100% <100%> (ø)
jsonapi/jsonapi_utility.go 70.1% <100%> (+0.95%) ⬆️
tenant/tenant.go 77.41% <100%> (+5.54%) ⬆️
configuration/configuration.go 71.25% <100%> (-1.92%) ⬇️
migration/migration.go 38.13% <100%> (+0.52%) ⬆️
controller/tenants.go 63.96% <46.87%> (-11.04%) ⬇️
environment/template.go 89.13% <50%> (+2.76%) ⬆️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 663fa75...da497e4. Read the comment docs.

@MatousJobanek MatousJobanek force-pushed the core-v2-merge branch 3 times, most recently from ef5c44d to 582e496 Compare November 8, 2018 14:07
@MatousJobanek MatousJobanek changed the title WIP chore: refactor core functionality to be more extensible chore: refactor core functionality to be more extensible Nov 8, 2018
Copy link
Contributor

@xcoulon xcoulon left a comment

Choose a reason for hiding this comment

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

Also, regarding the openshift package, I wonder if we could replace some parts with https://github.com/openshift/client-go ?

cluster/service.go Show resolved Hide resolved
cluster/service.go Show resolved Hide resolved
cluster/service.go Show resolved Hide resolved
controller/convert_whitebox_test.go Outdated Show resolved Hide resolved
controller/tenant.go Outdated Show resolved Hide resolved
tenant/repository.go Outdated Show resolved Hide resolved
tenant/repository.go Outdated Show resolved Hide resolved
tenant/repository.go Outdated Show resolved Hide resolved
tenant/repository.go Outdated Show resolved Hide resolved
tenant/repository.go Outdated Show resolved Hide resolved
@xcoulon
Copy link
Contributor

xcoulon commented Jan 17, 2019

Good job overall, and congratulations for raising the test coverage by 13.03%! 🙌💪

@MatousJobanek
Copy link
Contributor Author

Also, regarding the openshift package, I wonder if we could replace some parts with https://github.com/openshift/client-go ?

That was one of my first ideas - to use openshift/k8s clients for handling the communication with OS. I tried to use it in my code, but I came across a few issues - there are two most important ones:

  • As you probably noticed, the tenant code handles the response codes and based on the values it reacts in various ways. That wasn't possible with the clients - the API wraps the calls and hides the response codes, so it is hard to figure out what failed, why it failed (if anything failed) etc... something similar as we have with the oc client now. There is maybe some way of getting the response codes from the clients, but I wasn't able to figure it out.
  • At the time when I was playing with it (September 2018) there was missing client support for at least one object kind (I cannot recall which one - maybe RoleBindingsRestriction or ResourceQuota)

So instead of mixing two ways of doing that (because of missing client support for some object kinds) and trying to workaround/hack the missing response code information, I decided to go with one consistent way that is proposed in this PR.

Just to be clear - I'm definitely not against using it and if we figure out that the go-clients offer us everything we need, we can replace it. I believe that it shouldn't be so hard with my code as everything is hidden behind the endpoints/method abstraction.

Good job overall, and congratulations for raising the test coverage by 13.03%! raised_handsmuscle

Thanks. Actually, the increase was originally higher - at one time even around 16-17%, but went lower as I introduced a few tests separately. :-)

@xcoulon
Copy link
Contributor

xcoulon commented Jan 17, 2019

Also, regarding the openshift package, I wonder if we could replace some parts with https://github.com/openshift/client-go ?

That was one of my first ideas - to use openshift/k8s clients for handling the communication with OS. I tried to use it in my code, but I came across a few issues - there are two most important ones:

  • As you probably noticed, the tenant code handles the response codes and based on the values it reacts in various ways. That wasn't possible with the clients - the API wraps the calls and hides the response codes, so it is hard to figure out what failed, why it failed (if anything failed) etc... something similar as we have with the oc client now. There is maybe some way of getting the response codes from the clients, but I wasn't able to figure it out.
  • At the time when I was playing with it (September 2018) there was missing client support for at least one object kind (I cannot recall which one - maybe RoleBindingsRestriction or ResourceQuota)

So instead of mixing two ways of doing that (because of missing client support for some object kinds) and trying to workaround/hack the missing response code information, I decided to go with one consistent way that is proposed in this PR.

ok, I didn't know there were some missing parts. Maybe we could open one or more issues on the upstream repo if we really want to use this lib (that is, if other problems that you encountered can be lifted as well)

Just to be clear - I'm definitely not against using it and if we figure out that the go-clients offer us everything we need, we can replace it. I believe that it shouldn't be so hard with my code as everything is hidden behind the endpoints/method abstraction.

Yes, that's my thinking as well, and my point is we could have a simpler wrapper around the library. But again, it depends if the client library has all the features that we need

Good job overall, and congratulations for raising the test coverage by 13.03%! raised_handsmuscle

Thanks. Actually, the increase was originally higher - at one time even around 16-17%, but went lower as I introduced a few tests separately. :-)
Yeah, well, this is still a significant increase ;)

@fabric8-services fabric8-services deleted a comment from fabric8cd Jan 21, 2019
@MatousJobanek MatousJobanek changed the base branch from master to develop January 22, 2019 09:24
@MatousJobanek MatousJobanek merged commit 5db658b into fabric8-services:develop Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants