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

Helm Repo Release Streams #419

Merged
merged 4 commits into from
Mar 9, 2021
Merged

Helm Repo Release Streams #419

merged 4 commits into from
Mar 9, 2021

Conversation

bradfordcp
Copy link
Member

@bradfordcp bradfordcp commented Feb 24, 2021

What this PR does:

  • Replaces existing Helm publishing automation targeting helm.k8ssandra.io. Instead, depending on the action charts are either published to helm.k8ssandra.io/stable or helm.k8ssandra.io/next.
  • Removes .tgz files from under charts/*/charts/*.tgz
  • Adds script to pull / regenerate dependencies based on contents of Chart.yaml (wraps calls to helm dep update in a for loop)

This removes the binary dependencies from the repo and gitignores anything locally. K8ssandra developers will be able to regenerate these files with the helper script.

Which issue(s) this PR fixes:
Fixes #439, #203

Checklist

  • Changes manually tested - client
    • Next stream works (Helm client test)
    • Stable stream works (Helm client test)
  • Changes manually tested - GH actions
    • Next stream publishes when a PR is merged (and anything under charts/ has changed)
    • Stable stream publishes on tag creation
  • Migrate existing charts to Next stream
  • Initialize empty chart repo for Stable stream
  • Chart versions updated (if necessary)
  • Automated Tests added/updated
  • CHANGELOG.md updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA
  • Validate tests still behave as expected
  • Update release automation to run the helper script prior to packaging

@bradfordcp bradfordcp changed the title Enhancements/release streams Helm Repo Release Streams Feb 24, 2021
@bradfordcp bradfordcp force-pushed the enhancements/release-streams branch 3 times, most recently from 519c55c to 34fb897 Compare February 24, 2021 16:27
@jdonenine jdonenine mentioned this pull request Feb 24, 2021
4 tasks
@bradfordcp bradfordcp marked this pull request as ready for review February 24, 2021 18:50
@bradfordcp
Copy link
Member Author

To test:

helm repo add k8ssandra-stable https://helm.k8ssandra.io/stable/
helm repo update
helm install demo k8ssandra-stable/k8ssandra

CHANGELOG.md Outdated

## main / unreleased

* [FEATURE] #409 Add support for `configOverride`
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these issue numbers or PR numbers? When adding a new PR, should we create an entry at the top or the bottom of the unreleased section?

Here's an entry for my in-flight PR:

Suggested change
* [FEATURE] #409 Add support for `configOverride`
* [ENHANCEMENT] #436 Upgrade Stargate to 1.0.11 and add a `preStop` lifecycle hook to improve behavior when reducing the number of Stargate replicas in the presence of live traffic
* [FEATURE] #409 Add support for `configOverride`

Copy link
Member Author

Choose a reason for hiding this comment

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

It is currently the PR number. The order outside of the category seems less important. We could try and do it by when it was merged in to main, or based on the PR number. What are your thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

For your inflight PR do you want to rebase off of main and update the CHANGELOG there (in the PR)?

charts/cass-operator/crds/customresourcedefinition.yaml Outdated Show resolved Hide resolved
@jakerobb
Copy link
Contributor

Looks good! I added a changelog entry for a PR I have in flight in the form of a "suggestion", and have a couple non-blocking questions.

@bradfordcp
Copy link
Member Author

Note for developers, the dependencies are not stored in-tree and are explicitly .gitignored. If you want to pull the latest dependencies run the scripts/update-helm-deps.sh script. Note the GH actions will run this command before executing tests. This may lead to a difference in behavior locally vs out on CI.

@jsanda
Copy link
Contributor

jsanda commented Mar 1, 2021

@bradfordcp I understand that this PR removes the dependencies from git, but are the charts still using local, filesystem dependencies?

My hope was to altogether do away with local dependencies.

Copy link
Contributor

@jsanda jsanda left a comment

Choose a reason for hiding this comment

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

I have done a first pass through the changes. I see that we are still using local dependencies, e.g.,

dependencies:
  - name: cass-operator
    version: 0.29.0
    repository: file://../cass-operator
    condition: cass-operator.enabled

I want to altogether avoid local dependencies. I think it is error-prone. I think the charts should be pointing to published versions.

Here is the sort of workflow I am hoping for:

  • I update cass-operator chart with new cass-operator image
  • I update cass-operator chart version
  • I open an PR with cass-operator chart changes
  • When the PR is merged, GH Actions publishes the new version of cass-operator chart
  • Note that at this point k8ssandra still points at the previous version of cass-operator chart
  • I open another PR that updates the k8ssandra chart to point to the new version of cass-operator chart

@bradfordcp
Copy link
Member Author

I'm open to the workflow where we do not support local dependencies at all, but it has a major drawback. That is we can never have changes across chart boundaries within a single PR.

In the workflow above I would have to wait for the cass-operator PR to merge before moving on with my changes instead of bundling them into a common PR.

Additionally, the release process would have to change a little. We would have to update the dependencies to reference the stable stream when we cut a tag. It's not difficult, but there is extra work / complexity.

@bradfordcp
Copy link
Member Author

What do you think of a hybrid approach? IE local dependency on the main branch with part of the release process updating these to use the published dependencies?

@jsanda
Copy link
Contributor

jsanda commented Mar 1, 2021

I'm open to the workflow where we do not support local dependencies at all, but it has a major drawback. That is we can never have changes across chart boundaries within a single PR.

I am aware of this. I am not convinced though that it is a drawback though.

The versions of k8ssandra's dependencies correspond to what is in main. For example, we k8ssandra depends on version 0.29.0 of cass-operator. If we look in charts/cass-operator/Chart.yaml we should find that the version is 0.29.0. This makes me nervous because we recently had a situation where that dependency did not actually match what was in main despite the versions being the same.

Here is the problem I see in this PR. GH Actions runs scripts/update-helm-deps.sh which runs helm dep update across all charts. We could be pulling in uncommitted changes, and that will go unnoticed. Let's say I update the cass-operator chart which means I will bump the version to 0.30.0. I also update the k8ssandra dependency. Suppose I also have some changes in the reaper-operator and k8ssandra-common charts. Helm does not care about the version numbers. Updates will get applied and now the k8ssandra chart in the chart repo has dependencies that may not match either of what is in the chart repo or in git for the reaper-operator and k8ssandra-common charts. I don't think this will be a problem if we use published dependencies.

@burmanm
Copy link
Contributor

burmanm commented Mar 1, 2021

Here is the sort of workflow I am hoping for:

I'm not a fan of that process. Here's what I had in mind:

  • reaper-operator gets updated by pushing to reaper-operator master
  • automated process updates the k8ssandra master with new image tag version
    • same process, updates the version in reaper-operator chart
    • still same process, updates the dependency in k8ssandra chart with the new reaper-operator chart version
    • updates the k8ssandra chart version

I don't think we should have another step at all when controlling operator <-> k8ssandra master. What happens with stable releases is different, but otherwise I hope we would always ship the newest operator in the k8ssandra's next channel.

The github actions can modify the files automatically in the repository, there's no need for PR step. It means the updated versions can be automatically pushed to the helm streams also to prevent local storage of those files.

Obviously, the two exceptions are k8ssandra-common and cass-operator since we don't directly control these at least at the moment. We could automatically poll them through github actions if we wanted, but maybe that's the manual step we should keep with the current project setup (unless of course cass-operator moves under k8ssandra in which case we can automate it also).

@jdonenine jdonenine linked an issue Mar 2, 2021 that may be closed by this pull request
@bradfordcp
Copy link
Member Author

We could be pulling in uncommitted changes, and that will go unnoticed. Let's say I update the cass-operator chart which means I will bump the version to 0.30.0. I also update the k8ssandra dependency. Suppose I also have some changes in the reaper-operator and k8ssandra-common charts.

This could definitely happen in a local environment. Fortunately, with this change the compressed dependencies will NOT be committed. As you commit files and push them to the branch PR checks will run. This will regenerate the compressed dependencies with only the files that are committed. The test suite is then run again with the committed files. This could lead to tests failing in the CI environment where they "pass" locally. IMHO that's a win as we would catch the issue before being merged to main.

@bradfordcp
Copy link
Member Author

I'm open to removing the logic that rebuilds the dependencies and keeping the helper script if that helps us get the release automation in place. That feels like a mistake as we could end up in a situation where the committed tarballs are out of date (this has already happened).

What do you think we should do to move forward?

@burmanm
Copy link
Contributor

burmanm commented Mar 8, 2021

Just to clarify to @jsanda's approach, in my ideology this would not require multiple PRs. I'll try to go through some workflows to explain why. Even when using remote-repository by default, it does not mean that the local installation must use (for testing purposes) the remote repository. It's a very simple script to change this to local for testing.

However, for development, I think we should automate every possible part. Lets say you're developing changes to reaper-operator and want to test them.

  1. Commit lands to reaper-operator repository
  2. The GHA in reaper-operator builds the new Dockerimage and other relevant parts
  3. It then locally fetches the k8ssandra repository and makes modifications to the reaper-operator chart (updates CRD, the Dockerimage and updates the version). It automatically pushes them to the k8ssandra repo.
  4. A GHA in k8ssandra is triggered on push to the master. It notices changes in the version of reaper-operator chart (or we make it notice the change in reaper-operator elsewhere and it updates the version - does not matter) and packages this change and pushes the modifications to the remote repo that's used.
  5. A remote-repo change is detected (this could be continuation of previous GHA run). GHA modifies the k8ssandra's dependencies to include this newer version of reaper-operator and pushes the modifications to the k8ssandra-repo.
  6. The above process triggers an event - the k8ssandra chart has been modified. This causes the chart to be built and a new next release is pushed.

This all should be automated, there's no need for manual PRs to update any of these (and I'd say we shouldn't even allow that). It guarantees that all the dependencies are automatically managed and kept on the correct versions. This is for the development versions obviously - I'd like to keep the stable updates as separate thoughtprocess for now. For stable releases, we can automatically create all those .tar.gz packages that include everything for offline installation also if we want, so remote repositories are not an issue.

The above process is not black magic nor even difficult to implement. Neither is the yaml modification script to take local version to use - this is simple in Python.

jsanda and others added 4 commits March 8, 2021 17:04
Removed binary dependencies from repo
Added helper script to pull chart dependencies
Started CHANGELOG
Added helper scripts to install yq and helm

Signed-off-by: Christopher Bradford <christopher.bradford@datastax.com>
@bradfordcp bradfordcp force-pushed the enhancements/release-streams branch from fb0e03e to b53e74d Compare March 8, 2021 22:55
@jsanda
Copy link
Contributor

jsanda commented Mar 9, 2021

@burmanm Thanks for the detailed write up. I am very interested to learn more about the process you are describing. I think it would be best to pursue that in a separate ticket though.

The changes in this PR are definitely a big improvement from where we currently are, so I think it makes sense to go ahead and merge. We can consider additional changes/improvements in follow up tickets.

Thanks @bradfordcp!

@jsanda jsanda merged commit a304b72 into main Mar 9, 2021
@jsanda jsanda deleted the enhancements/release-streams branch March 9, 2021 01:10
jeffreyscarpenter pushed a commit to jeffreyscarpenter/k8ssandra that referenced this pull request Mar 10, 2021
* update chart version

* Updated workflows for new release streams

Removed binary dependencies from repo
Added helper script to pull chart dependencies
Started CHANGELOG
Added helper scripts to install yq and helm

Signed-off-by: Christopher Bradford <christopher.bradford@datastax.com>

* Bringing in CHANGELOG entry from @jakerobb

* Removing binary chart dependencies, again

Co-authored-by: John Sanda <john.sanda@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate management of stable/next repo channels Set up a stable Helm chart repo
5 participants