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

Proposal: architectural changes #30

Closed
6 tasks
snowdrop-bot opened this issue Nov 5, 2020 · 0 comments
Closed
6 tasks

Proposal: architectural changes #30

snowdrop-bot opened this issue Nov 5, 2020 · 0 comments

Comments

@snowdrop-bot
Copy link

Issues with the current architecture

Lack of abstraction

The main issue with the architecture, in my opinion, is that there is not enough abstraction over concepts and their actual
representation / implementation. This manifests itself at several levels, the main ones being:

  • no platform abstraction (OpenShift vs. plain Kubernetes vs. Docker), each requiring their own clients when a single one could work for both
  • little to no component concept abstraction: each component implementation is actually a combination of several structures and representations which are directly accessed

This results in code complexity because client code is forced to deal with the low-level structures and particularities instead of being able to rely on a common abstraction. This has the following consequences:

  • parallel code bases where code to deal with one component type is copied over and sightly changed, all of it hidden behind lots of if/else conditionals, to implement a different component type
  • client code needs to be aware of low-level representations, accessing them directly making it very difficult to change them because it impacts code almost everywhere
  • cluster representations are not abstracted either tying odo to particular cluster representations rendering the code base
    fragile in case these representations need to be changed
  • changes cannot easily be made because there is often not one single spot to change or seemingly innocuous changes have unintended consequences

Context under-utilization

Context was created to provide contextual information to commands so that each command didn't need to perform the same logic over and over. It seems that the concept hasn't been developed as much as it could, leading to commands performing logic to test things in slightly different ways resulting in differing behavior between commands. This is especially evident when looking at how commands identify whether they're dealing with a devfile component or not. This points also loops back to the lack of abstraction issue.

Moreover, commands and utility classes could be simplified if that contextual information could be reliably accessed at
different levels of the code instead of having to pass some parts of it around as function arguments often leading to functions with more than 5 arguments, which very often don't always need to be present at the same time depending on the execution context.

Recommendations

Create proper abstractions for the core concepts

The odo core concepts should have proper abstractions hiding the implementation details so that client code (e.g. commands) don't need to know or deal with the messy details, instead manipulating concepts as opposed to implementations. More precisely, odo should have Component, App and Project abstractions at the very least, each hiding how they are implemented. In particular, for the Component concept, it should hide whether we're dealing with an s2i or devfile component, provide an interface to access its different elements without having to know how they are implemented. In particular, it should hide how the information is represented locally or on the cluster: client code should not have to know how to parse an env / devfile / whatever to extract the name of the component or its URLs. Similarly, client code should not need to know that a Component is represented by a DeploymentConfig on OpenShift but a Deployment on plain Kubernetes. All these implementations should be hidden.

Another aspect of this is that these abstractions should provide an interface, and their data should only be accessed via
accessor functions, not directly. This is a basic encapsulation idea, but it is very helpful for several reasons:

  • it unifies client code because the accessor exposes the internal data in a unified way (in particular when dealing with
    missing data, providing sane defaults or at least properly defined behavior) instead of letting client code figure out what to do and, thus, possibly doing different things at different spots
  • it makes lazy initialization possible leading to more efficient code where data is only retrieved / parsed / created when
    needed, this also means that availability of resources can also be delayed (for example, a connection to the cluster might not be required at the onset)
  • it makes it a lot easier to change the underlying representations / implementations because now you only have one spot to change instead of the implementation being spread across client code and different structures

Related issues:

Unify cluster access

There should only be one client to access the cluster and that client should also provide an abstraction above the platform
being accessed. There is no reason a command should know whether it deals with a plain Kubernetes cluster or an OpenShift one: the client should hide the differences. Coupled with proper abstractions for concepts, this would lead to dramatically simpler code base because now commands would only deal with abstractions instead of having to know the details of how a component is implemented for each supported type and platform. The client should only return high level Component or App abstractions instead of low-level representations such as DeploymentConfig or Ingress. It's not excluded that several client implementations be provided; these implementations should, however, be accessed via a unified interface and the specific implementation hidden from client code: a command should not need to know how to create a client, just that it can retrieve the appropriate one when needed.

Related issues:

Generalize Context use

Commands require lots of information to be able to do their work and gathering that information (or making it easy to do so), is the role of the Context. Combined with the previous two points (abstractions over concepts and cluster access), moving gathering all the information that commands require into Context would make command implementations simpler and more coherent. Context would build the appropriate abstractions based on the command's execution context: which flags have been specified, what files are locally present, which cluster can be accessed and its type, etc. All that logic would be encapsulated in a single spot so that it could easily be changed if needed but, more importantly, it could be implemented only once instead of requiring commands to rewrite the same code over and over, possibly with slight differences leading to subtle bugs and incoherent behavior.

Associated with concept abstractions, the Context would provide access to lazily instantiated concepts instead of providing direct access to local config, env or devfile files. Associated with cluster abstraction, the Context would provide simpler cluster access with a lazily instantiated, unified client instead of providing one client for each platform and letting commands figure out what to do based on the nullity of one or the other…

Related issues:

Properly use the Complete/Validate/Run pattern for commands

The Complete/Validate/Run pattern for commands is meant to simplify the maintenance of command implementations and make sure that execution steps are taken when needed, not before. This also makes it easier to onboard new developers since command implementations have a structure that is easily to follow. While it sometimes makes sense to deviate from this pattern (for example, when validation could be performed more efficiently in the Complete step), the pattern should be followed as much as possible. In particular, any computation steps should not be performed before the command is actually executed (i.e. Complete is called), and definitely not when the command is created. The Context should be created in the Complete step so that it's available for further computation.

Related issues:

Conclusion

odo has reached a state where evolving it becomes more and more difficult, slowing down the development of new features while maintaining existing ones without breaking them. With the 2.0 release out of the way, it's time to clean up the architecture to be able to move forward with greater confidence and velocity.


redhat-developer#4057


$upstream:4057$

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

No branches or pull requests

2 participants