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

RFD 58: Package Distribution #10746

Merged
merged 8 commits into from
Jun 3, 2022
Merged

Conversation

fheinecke
Copy link
Contributor

@fheinecke fheinecke commented Mar 2, 2022

@fheinecke fheinecke added the rfd Request for Discussion label Mar 2, 2022
@fheinecke fheinecke self-assigned this Mar 2, 2022
@github-actions github-actions bot requested review from r0mant and russjones March 2, 2022 17:43
@fheinecke fheinecke requested review from webvictim, wadells, zmb3 and tcsc March 2, 2022 17:54
wadells added a commit that referenced this pull request Mar 3, 2022
We do not publish pre-releases to apt repos, but we do publish them to
github.  That means we need to filter them out when considering if an
apt release should be published.  We don't want v8.3.3 to be blocked by
v9.0.0-dev.1, only by v9.0.0.

Honestly, this is a bit of a mess, but it only needs to hold out a bit
longer until #10746 lands.

Contributes to #10800
wadells added a commit that referenced this pull request Mar 3, 2022
We do not publish pre-releases to apt repos, but we do publish them to
github.  That means we need to filter them out when considering if an
apt release should be published.  We don't want v8.3.3 to be blocked by
v9.0.0-dev.1, only by v9.0.0.

Honestly, this is a bit of a mess, but it only needs to hold out a bit
longer until #10746 lands.

Contributes to #10800

(cherry picked from commit 08bc483)
wadells added a commit that referenced this pull request Mar 3, 2022
We do not publish pre-releases to apt repos, but we do publish them to
github.  That means we need to filter them out when considering if an
apt release should be published.  We don't want v8.3.3 to be blocked by
v9.0.0-dev.1, only by v9.0.0.

Honestly, this is a bit of a mess, but it only needs to hold out a bit
longer until #10746 lands.

Contributes to #10800

(cherry picked from commit 08bc483)
Copy link
Contributor

@wadells wadells left a comment

Choose a reason for hiding this comment

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

Thanks for all the research that went into this!

Some meta notes:

When you submit a PR, please provide a summary in the description field, including any related issues. While I may have that context, others who look at this PR (e.g. our open source community, or maintainers in two years time) won't have as much context implicitly available. Better to make that context explicitly available.

For RFDs, I also like to link to the rendered version in the description:

https://github.com/gravitational/teleport/blob/rfd/0058-package-distribution/rfd/0058-package-distribution.md

This is especially helpful for looking at e.g. the table, which is difficult to parse in the two column diff layout.

To maintain backwards compatibility with our current solution we can reroute requests for the current repos to the new ones. This can be accomplished by placing a reverse proxy in front of the current repos and then using it to rewrite or redirect the incoming request to the new repos.

### Future work
While a specific solution is outside of the scope of this RFD, it is pertinent to discuss a disaster scenario that is common to all solutions, including the current one. If the hosting solution that contains the repo (i.e. a S3 bucket or GCP Artifact Registry) is deleted then all artifacts must be rebuilt and published from scratch. It looks like the Drone pipeline for Teleport takes around 90 minutes to run. Depending on how many instances can be ran at once without conflicting with each other, it could take several hours to get the repository back online to it's previous state. This could be alleviated by backing up artifacts after they're built, or by backing up the entire hosting solution.
Copy link
Contributor

Choose a reason for hiding this comment

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

I've got some ideas here. DR backups are defiantly worth doing, with data replicated to a seperate region and versioning turned on.

Furthermore, I'd like to push towards a two phase publishing process:

  1. upon tag (or maybe even push to master/releases/*) push the bits to a internal, private, staging repo.
  2. upon promotion, re-sign with production keys, and push to the customer facing repo

Mostly, I want this so we can test most of the publishing without impacting customers. This has been a pain point for me, and many others running releases, as discussed recently at #10804 (review)

r0mant added a commit that referenced this pull request Mar 4, 2022
…10806)

We do not publish pre-releases to apt repos, but we do publish them to
github.  That means we need to filter them out when considering if an
apt release should be published.  We don't want v8.3.3 to be blocked by
v9.0.0-dev.1, only by v9.0.0.

Honestly, this is a bit of a mess, but it only needs to hold out a bit
longer until #10746 lands.

Contributes to #10800

(cherry picked from commit 08bc483)

Co-authored-by: Roman Tkachenko <roman@goteleport.com>
wadells added a commit that referenced this pull request Mar 4, 2022
…10804)

We do not publish pre-releases to apt repos, but we do publish them to
github.  That means we need to filter them out when considering if an
apt release should be published.  We don't want v8.3.3 to be blocked by
v9.0.0-dev.1, only by v9.0.0.

Honestly, this is a bit of a mess, but it only needs to hold out a bit
longer until #10746 lands.

Contributes to #10800
wadells added a commit that referenced this pull request Mar 4, 2022
…10805)

We do not publish pre-releases to apt repos, but we do publish them to
github.  That means we need to filter them out when considering if an
apt release should be published.  We don't want v8.3.3 to be blocked by
v9.0.0-dev.1, only by v9.0.0.

Honestly, this is a bit of a mess, but it only needs to hold out a bit
longer until #10746 lands.

Contributes to #10800

(cherry picked from commit 08bc483)
@wadells wadells requested a review from logand22 March 23, 2022 19:12
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

@fheinecke One thing I'm not sure I really understood is this:

https://github.com/gravitational/teleport/pull/10746/files#diff-642f76cdad949a89da3b92b729645c72d9fdca9f664afc093e98856d23106955R46

Does it download the entire content of S3 bucket with deb packages before sync? This is not going to be scalable, any way to do it without that? Unless I misunderstood of course.

@tcsc
Copy link
Contributor

tcsc commented Mar 29, 2022

In the past we have preferred using libraries rather than shelling out to command line apps. If we can, we should try using the aptly source as a library and interact with the repo via that. It would also solve the problem of getting the aptly binary on the buildbox image (i.e., we wouldn't - we'd just use go.mod to pull it in as needed)

Copy link
Contributor

@tcsc tcsc left a comment

Choose a reason for hiding this comment

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

Please delete the build-apt-repos binary

fheinecke added a commit that referenced this pull request Apr 11, 2022
@r0mant r0mant requested a review from tcsc April 12, 2022 01:06
@r0mant
Copy link
Collaborator

r0mant commented Apr 12, 2022

@tcsc Can you take another look?

@fheinecke fheinecke requested a review from klizhentas as a code owner April 25, 2022 18:49
@fheinecke fheinecke force-pushed the rfd/0058-package-distribution branch from 118ece8 to fe84b45 Compare May 25, 2022 22:31
@fheinecke fheinecke requested review from tcsc and wadells May 26, 2022 15:52
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

Nothing major on my side.

@tcsc @wadells Could you please take another look as you previously requested some changes on this?


kind: pipeline
type: kubernetes
name: migrate-apt-new-repos
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be a part of the drone pipeline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about this a lot while building this part out. Putting it in the pipeline is by far the easiest way to implement migrations for older versions. In 99.9% of cases this pipeline will never be ran, but when we need to add previous versions this will save significant time. This section along with this new dronegen function takes another migration process like I did last week and turns it into a 5m change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this particular pipeline is here, though. I get publish-apt-new-repos, but not this one. Is it just marking that you intend to implement the migration of legacy here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pipeline is the implementation of old artifact migration. It's just not enabled/ran unless dronegen is configured to migrate specific versions. When specific versions are added to this function then running make dronegen will add migration steps to this pipeline for Drone to run. When there are not any functions listed there, running make dronegen will replace the pipeline with a "NOP" pipeline to prevent repeated migrations.

}

// Instantiates a new apt repo tool instance and performs any required setup/config.
func NewAptRepoTool(config *Config, supportedOSs map[string][]string) (*AptRepoTool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not make supportedOSs a part of the Config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When writing this I thought that would be a great idea, but ultimately decided against it as it has several downsides:

  • The flags library does not support it, so implementing a map[string][]string input would likely add several hundred lines of parsing and code
  • Whatever format I went with would make reading the .drone.yml and related dronegen function more difficult, and would make adding/removing OS and OS versions more difficult, or would make the dronegen function more brittle

Not pulling in the supportedOSs map makes the code overall easier to read and maintain, and pushes the burden of validating the map to the compiler.

@fheinecke fheinecke force-pushed the rfd/0058-package-distribution branch from 3f642da to f2bb06b Compare June 1, 2022 16:49
Copy link
Contributor

@wadells wadells left a comment

Choose a reason for hiding this comment

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

Looks good to me. How did you test the new pipelines? Could you link a demo run or two?

@fheinecke fheinecke force-pushed the rfd/0058-package-distribution branch from f2bb06b to aa2663f Compare June 1, 2022 19:42
@fheinecke fheinecke force-pushed the rfd/0058-package-distribution branch from aa2663f to 5f0a933 Compare June 1, 2022 20:10
@fheinecke
Copy link
Contributor Author

@wadells here are some example runs:


kind: pipeline
type: kubernetes
name: migrate-apt-new-repos
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this particular pipeline is here, though. I get publish-apt-new-repos, but not this one. Is it just marking that you intend to implement the migration of legacy here?

@fheinecke fheinecke merged commit 6a693b9 into master Jun 3, 2022
fheinecke added a commit that referenced this pull request Jun 6, 2022
fheinecke added a commit that referenced this pull request Jun 6, 2022
fheinecke added a commit that referenced this pull request Jun 13, 2022
* Backport of #10746 to v8

* Added prerelease check to new APT promotion pipeline
fheinecke added a commit that referenced this pull request Jun 14, 2022
* Backport of #10746 to v9

* Added prerelease check to new APT promotion pipeline
@fheinecke fheinecke deleted the rfd/0058-package-distribution branch June 15, 2022 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfd Request for Discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants