RFC: Redesign storage api

Inspired by Rust's RFC process, and a desire to have decisions made more openly. I would like to follow their lead with using the Request For Comment (RFC) process to discuss large changes to the project before we start seeing PRs. We will allow a period for feedback and provide some design decision documentation.

Summary

The storage API, database schema, and potentially all of the protocol buffers should be re-implemented. The gRPC storage API will be written in Rust, and it will involve a complete reimplementation of the database schema. Consequentially, we will need to revisit all or our protobuf messages to align better with the database.

The existing Go code will be refactored remove the storage package (at least the major functionality), and the rest of the codebase will communicate with the storage layer (i.e. Postgres) only through gRPC calls.

Background

It's been an honest struggle to understand the codebase. Part of the reason is because I haven't warmed up to the Go language and tooling ecosystem. Another reason is that I was unprepared for the codebase organization to be in the state it currently is in considering the vast amount of different functionality we have.

Go's failing is that we were able to easily create such wide namespaces/packages to the point where circular dependency is always around the corner in our codebase. The compiler and I have a very adversarial relationship, and I'm pretty turned off of the language because of it.

To be clear: What we have is not throwaway code, and I don't intend to do so any time soon.

Since we are using gRPC, I propose that integration will be less risky if we are more strict about using gRPC calls to pass stateful data across domains.

How this relates to the backlog

A number of issues in the backlog point at both the database and proto message schemas being overloaded in functionality, used in untype-safe ways with the database, or just being un-ergonomic in the codebase.

And others require updates in the data domains to add functionality

In attendance: @eccramer @mariannefeng @shankj3 @tjtelan

Summary of in-person meeting:

There's an atmosphere of caution, but a productive conversation. The expected next steps is some database + proto message design, and some example test workflows.

@mariannefeng and @shankj3 raised caution flags. There's risk about this RFC due to the perception of lack of movement in the project (measured by commits landing in the master branch), and an observing our accelerating negative burn rate in the backlog.

Questions about current holistic approach of maintenance were asked, and we split on priorities. (Why not address the smaller client-facing bugs vs introduce unknown risk from RFC?)

Our (@tjtelan + @eccramer ) response is that there are many frequently traveled, but inflexible areas in the codebase without a clear design pattern to follow. Attempting to modify code has lead to build and/or test issues that we could not move forward into PR, which lead to the creation of this RFC.

The primary focus of Our attention is on Ocelot's data model. We are seeking to use an ORM and structuring storage level CRUD operations interfaced exclusively through dedicated grpc calls so we can isolate serialization/deserialization for protos and storage.

There is concern for including another language within the same project, as it may be an unconventionally high barrier to entry for contributors. But we are all in agreement that considering another language is pretty distracting, and unproductive right now. Therefore, this RFC should step back on choosing language and focus more on providing this Storage api through designs first.

Lastly, we reviewed creds.proto very quickly and informally, and ended on discussion for what even the concept of a "scalable" / "better" proto form would look like. A round of designing and critiquing the database/protos schema should be the next step.

Here's a first pass at the schema. I'll be following up with some user stories that we possibly want to be able to support.

Ocelot proposed schema

Looking at the current schema migrations, I noticed that I am missing the table for tracking build stages: build_stage_details. This is used for within output of ocelot status and Slack notifications. Possibly also for printing the stages in ocelot logs?

Anyway, it may appear to have an important relationship with build_output, but in the current (master branch) schema, it isn't clear how these two columns are supposed to coexist.

build_output column: output https://github.com/level11consulting/ocelot/blob/master/deploy/sql/V1__create_initial_tables.sql#L17

build_stage_details column: messages https://github.com/level11consulting/ocelot/blob/master/deploy/sql/V1__create_initial_tables.sql#L30

build_output contains all of the stdout/(stderr too?) from the docker container, w/ addition of a column we prepend to each line that marks what stage we're in.

This will be an internal stage, like setup, or prestart or a user provided stage from ocelot.yml.

build_stage_details contains some internal statuses, like details of creating containers, initialization of the build environment, or running integrations. It reads like a checklist, where either everything is checked, or all but the last will be checked and the last item is a failure.

There is some value to keeping track of state changes like this, for performance monitoring or root cause analysis.

We'll need to play around with this idea, but build_output possibly might need to keep output related to stage instead of all in a single row.

Ocelot proposed schema 20190401T142902

Made small changes to formatting, and field types.
Rename: notification_integration -> notify_integration
Merged the idea of build_stage_details and build_output into a single table: build_stage
Added a active_state to account, so we can possibly disable an account
Added a job_trigger type, so we can possibly add configurable trigger conditions in ocelot.yml

Ocelot proposed schema 20190401T170548

Updated account table to org
Changed repo_id FK to a longer form to be better self-documenting
Removing FK on registered_repo and build_summary
Changed cardinality on org.id to be one and only one
Minor rearranging rows, types and arrows for clarity

@eccramer @shankj3 @mariannefeng

I think this is going to be my last change without feedback before moving onto the API.

Ocelot proposed schema (1)

build_stage.status is now a jsonb
- I'm thinking we can work with this json, and keep it chronologically ordered easier
polled_repo.cron_expression is also a jsonb
- I'm thinking we should interpret the cron string into a json expression so we can validate input
Keeping track of build numbers per-repo with build_target.build_index and registered_repo.next_build_index
Removed registered_repo.deploy_active_state
- This was from an older idea that I wasn't committing to.
Adding active_state to registered_repo and polled_repo

User stories: https://gist.github.com/tjtelan/6ffc399159f897f25e6b893eb787c97a

I'm trying to loosely follow RFC procedures that the Rust-lang devs have developed. Part of that being a request for a final review. It'll have a time constrained period (default 10 calendar days). If no one has any objections I was hoping it could take place in the comments, in addition to any in-person meeting requests so we can have any history about this process documented (other than my own comments)

https://github.com/rust-lang/rfcs#what-the-process-is

We'll probably have 2 of these requests because my changes only deal with the database design, and we haven't yet dealt with the api.

Hey! Just an FYI this week has been really heads down at 10/ trying to get work done for a client by our meeting with them tomorrow. I will look look over the proposed designs and give my feedback starting Friday

I had time today, so I took a quick look. It's been so long since I've been on the project I don't know if I have any productive input. I am curious though - what do you imagine an active_state field would mean for secrets or vcs_creds? I'm just trying to think of a reasonable use case where I would add github information or secret info like a pem key but for some reason it'd be disabled. Although...I guess if somebody wants to "delete" a credential you could set the active_state field to be "disabled"? But then you'd have to somehow differentiate from disabled meaning a deleted entry vs. an actual temporary disable (like of a notification or an account).

Once again, it's been a while since I dug around on the code, so this is just me following a random thought I had, feel free to ignore if it's not relevant! Also, @tjtelan, your user stories gist is hard to read....I think formatting in markdown and taking advantage of headers might help with readability.

@mariannefeng Good question!

Here are 2 examples off the top of my head describing what we could do by changing the active state for secrets or for vcs_creds.

Secrets:

Disable private artifact repo (effectively, by disallowing the ability to login, since the secrets are disabled)

VCS Creds:

Disable all builds from private Github organization (by disabling the vcs of the registered repos we want to disable)

An org can have secrets, vcs, or notifications. There is no relationship between a vcs and a secret. Disabling a secret, or disabling a vcs are both independent of one another.

Disabled and deleted are shades of grey that we don't support now. Most of the mindset was to prevent a chain reaction of expected failures from growing. Premptively silencing expected, but temporary noise. Examples of causes for expected, but temporary noise: Major refactoring, test failures because of new functionality, attempting deployment during a known outage.

I'm just trying to think of a reasonable use case where I would add github information or secret info like a pem key but for some reason it'd be disabled.

The only reason for anything to be disabled is because a user in the org disabled it. That's not just an arbitrary state you may fall into. (And w/o an authentication system to provide accountability, we can only assume that there's no malicious intent inside our private network. More likely poor communication would explain anything happening "for some reason").

Although...I guess if somebody wants to "delete" a credential you could set the active_state field to be "disabled"? But then ...

I'll just stop you right there. Shadow deletes aren't what I had in mind. We don't have a compelling use-case to support keeping track of history for secrets with the intention of undoing deletion, because users aren't trying to undelete secrets. They would be trying to re-add.

But maybe I'm reading into another concern? How are we supposed to interpret adding any resources into an org when they are certain to be immediately unusable? For example, if we were adding secrets that would only be used by a disabled repo.

One example for doing this would be setting up an org/vcs/registered repo before they are used by developers.

The closest equivilent workflow today would be to commit to the ocelot.yml with stages or relevant lines commented out. This is a noisy Ocelot-specific commit that I want to discourage by providing enough flexibility outside of the user's git history.

I've been thinking about how the L11 org has been managed over time. I couldn't think of combinations of active_state scenarios that were broken in a way that we couldn't prevent through the cli, so perhaps this could be an overcorrection? Negative examples would be very useful in convincing me that allowing all of this combination of active_state is a thing to avoid in the database.

Before I forget, I've updated the formatting for the user stories. I always forget I have to rename the file with a markdown suffix for it to render properly

Hi all!

Just a reminder: We're 8 days in. At day 10, if there are no unresolved objections, I'm going to continue forward with API design with this database schema and with my use-cases.

Hmm with the active state I would think that both of those use cases could be achieved by deleting the VCS credential as well, unless you think a different behavior in how that stores would be valuable for the edge case. On an org level, I can see disabling everything when... say... Kubernetes goes down.

Could you elaborate more on your use of next_build_index? Also on the value of cron_expression as jsonb.

Adding output to build stage results has a significant impact on how the werker code stores, as currently it saves the entire output at the end of the build. Is there a reason for this change? I don't currently see the value in being able to get output from a specific stage.

I see the distinctions you are trying to create with the different secret types, but they have all the same columns, save the type field. It seems repetitive. Especially since now since all the additional fields are gone (I assume to put it all in vault?). Is this a normal pattern, to have 3 tables that all have the same 4/5 columns that mean the same thing? Maybe I'm also hung up on the naming of them -- they all are different parts of a build lifecycle and that's what's really differentiating the tables, one is for setup and starting builds, one if for during builds, and one is for after builds. secret_integration, and specifically secret_type, could apply to any of those stages.

@shankj3 Thanks for taking the time to respond! I'll give my response inline with your questions/comments so I can align how I made my design considerations.

Hmm with the active state I would think that both of those use cases could be achieved by deleting the VCS credential as well, unless you think a different behavior in how that stores would be valuable for the edge case. On an org level, I can see disabling everything when... say... Kubernetes goes down.

I was thinking that an org may want to delgate areas of responsibility, and therefore would want some ability to manage its own space with some autonomy. Like the difference in scope between K8s's cluster vs namespace.

I don't consider disabling of any user resources to be edge-case behavior. What if we only want to temporarily stop builds from Partner repos (example reason: We want to stop building forked code from our Partner's vcs)? If we delete the Partner bitbucket, then when we want to re-enable builds, we have to re-add repos. I don't think that is a practical solution for every case.

From experience w/ other build systems, being able to see all of the VCS that are available (that is, we can still commit to them) is preferable to deletion, even if the build system has it disabled by a user. It is more or less used as a shared accounting system for Devs and Ops.

For L11, this workflow is highlighted when we are committing into other partner's Bitbucket. In my proposed model, the data would look like this:

L11 (org):
    L11 bitbucket (vcs):
        L11 repos:
    Partner bitbucket (vcs)
        Partner repos:

Active state is tracked everywhere because I could not think of general reasons to only track at specific areas in the model. (This thinking will need to be revisited if we ever support RBAC style permissions.) The granularity is particularly important in this case. If we disable the org, then all of L11's repos get disabled, which is undesired.

Could you elaborate more on your use of next_build_index?

Sure!

next_build_index is to help achieve relative build numbers for a repo instead of our current usage of global build numbers.

Jenkins or Shippable etc. don't exclusively use global build numbers. I think it is that weird that we do.

A developer shouldn't need to look at their build summary list to know that their repo only had one build between build numbers 1000 and 1010.

Also, a creeping feature is the ability to set the next build number. Jenkins has this feature. If we track relative build numbers, then this is a simple feature to offer.

Also on the value of cron_expression as jsonb.

cron_expression I honestly have less strong feelings about change here at the moment. It certainly can stay the way it is as a raw cron string but possibly w/ some validation, since we currently take in anything as the cron expression, and jam it into cron. My immediate goal is to allow this to be configurable in the ocelot.yml (#153).

Jsonb is looking at the future implementation I am imagining, since I'm already proposing changes in this space. I hoped this would make a natural language parser easier to implement. Something similar to how Slack does recurring reminders.

I get that we could possibly use less space to continue to express what will ultimately be a unix-like cron expression, but if we wanted to print out an english translation of */3 * * * * to "every three minutes", then currently we would have to provide that translation all the time in the frontend.

For our usage, only cron cares about the cron expression, so I propose we would convert to that form when polling starts. To the user, it is slightly extra friction to document for those who aren't already familiar (to be clear: it isn't a huge hurdle). In the code and to the user, we'd like to be able to be fuzzy about time since users really just want to poll as often as necessary for new commits to get built.

Adding output to build stage results has a significant impact on how the werker code stores, as currently it saves the entire output at the end of the build. Is there a reason for this change? I don't currently see the value in being able to get output from a specific stage.

I think I see your point. What I'm trying to achieve is having lighter row inserts. We currently have several rows that are corrupted in our table. A common characteristic is that those builds typically had very large logs.

Completing a build stage means we are holding onto build logs that don't really need to be kept in memory since the stage isn't going to change anymore.

I also don't really like for how we mark output for stages. We prepend stage names per line. This means we repeat the same string over possibly thousands of lines, and that's a lot of unnecessary work (not to mention wasted space). The frontend only needs to know the stage name once, and it can take care of printing stage names to the user on its own.

Live build output streaming would likely need to make new considerations to manage printing what stages have completed, and what is currently producing logs. There's a relation between build summary and build stages, so it should not be a problem to collect these stages for presentation.

But no value for this feature? There really aren't any constraints on what a user does during their jobs. Though I would probably want to see all the output though in most cases. I would not personally say that no useful reason exists to pick out a specific stage out of any of a build output. Example: Picking out a failed stage, like a deployment. Especially after a very long previous stage that succeeded.

Additionally this would create structure for reporting stage info in chronologically correct order (issue #133).

The creeping feature would be the ability to report on the time-performance for a repo build vs the average time. It would beg the question about what stages we spend time in with detail. We get this for free if we commit logs on build stage transitions.

I see the distinctions you are trying to create with the different secret types, but they have all the same columns, save the type field. It seems repetitive. Especially since now since all the additional fields are gone (I assume to put it all in vault?). Is this a normal pattern, to have 3 tables that all have the same 4/5 columns that mean the same thing? Maybe I'm also hung up on the naming of them -- they all are different parts of a build lifecycle and that's what's really differentiating the tables, one is for setup and starting builds, one if for during builds, and one is for after builds. secret_integration, and specifically secret_type, could apply to any of those stages.

This could certainly be an overcorrection from the single table model we have right now. But I need to hear an offer of what to do instead given my non-functional requirements. Otherwise I don't know if this is an actionable objection.

What I want is a typed approach in the database for managing the different kind of resources we allow users to add. No more jamming protobuf enum indexes into the db. I want to look at the db w/o having to have the protobuf file open to translate types. (A secret_type env_var is more descriptive than a sub_cred_type of 11) Our database schema going forward will not have so much tight-coupling with our protobuf representation.

I don't claim to know my database design very well, but I would say it is normal to group similar things together in a table so that it can be queried. I guess I'm not getting the point of the repetitive comment? Yes, it is repetitive, but it is also going to be much easier to build queries, and grow the schema.

Suggestions for the naming? I think the gist of the names are really concise and to the point, tbh. The split I chose was all on functional domain, and you already largely identified my methodology for the split.

I landed on the separate tables to illustrate the relationships w/ respect to an organization. which looks roughly like this in my mind:

org:
    notify_integration:
    secret_integration:
    vcs_creds:
        registered_repo:
            polled_repo:
            build_target:
                build_summary:
                    build_stage:

I split the existing creds domain into 3 according to the contexts they are used in.

vcs_creds are managed as a component of an org. These are not directly used during a build. We couldn't even have builds w/o these being valid.
notify_integration is also part of an org. It'll primarily be used during a build for communicating out during/after stages.
secret_integration is unsurprisingly part of an org. It'll cover pretty much everything else. It is only used within a build.

Updated schema to match the current protobuf definition. This is what I'm aiming for at the moment.

Orbital - Orbital proposed schema

Renamed a lot of tables to be more specific, and have consistency with the protos. Timestamps + duration rather than ending timestamps. But overall relationships haven't changed.

I got rid of the vcs_creds table altogether along with the usage of vcs to mean the git provider. The foreseeable future is probably going to be git and all the different kind of git provider support.
I want to think of secrets as a very flat hierarchy with repos + notifications having specific uses, so I have a new relationship noted between those two tables and the secret table. This separates the logic between handling management of secrets internally, and how they are organized in vault.
Anything about cron was removed in repo_poll_history because (at least for now) polling for changes will be the default behavior so I don't want to bother the user with these details.

The only thing I've lost some context for purpose is build_stage.status

orbitalci / orbital