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

Currently, v0.23.0 image has wrong app version #736

Closed
eminaktas opened this issue Feb 23, 2022 · 15 comments
Closed

Currently, v0.23.0 image has wrong app version #736

eminaktas opened this issue Feb 23, 2022 · 15 comments
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@eminaktas
Copy link
Contributor

What version of descheduler are you using?

descheduler version: v0.23.0

crane digest k8s.gcr.io/descheduler/descheduler:v0.23.0
sha256:1a40adaa9b92624d09fee9e8611f0177949949c6f2fa633e43457eec96860c65

Does this issue reproduce with the latest release?

Yes.

Which descheduler CLI options are you using?

Default options.

Please provide a copy of your descheduler policy config file

Default policy yaml

What k8s version are you using (kubectl version)?

kubectl version Output
$ kubectl version
Client Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.4", GitCommit:"b695d79d4f967c403a96986f1750a35eb75e75f1", GitTreeState:"clean", BuildDate:"2021-11-17T15:48:33Z", GoVersion:"go1.16.10", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.7", GitCommit:"132a687512d7fb058d0f5890f07d4121b3f0a2e2", GitTreeState:"clean", BuildDate:"2021-05-12T12:32:49Z", GoVersion:"go1.15.12", Compiler:"gc", Platform:"linux/amd64"}

What did you do?

Nothing special.

What did you expect to see?

v0.23.0

What did you see instead?

v20220127-v0.22.0-79-gafb1d75ce

@eminaktas eminaktas added the kind/bug Categorizes issue or PR as related to a bug. label Feb 23, 2022
@eminaktas
Copy link
Contributor Author

#734

@JaneLiuL
Copy link
Member

/assign

@eminaktas
Copy link
Contributor Author

Kindly reminder:

Unfortunately, we still have the wrong Descheduler version with the latest image. We should consider publishing an image to fix the tag.

descheduler_build_info{DeschedulerVersion="v20220228-descheduler-helm-chart-0.23.2-2-gfd6c9a68a", GoVersion="go1.17.7",..

cc @damemi

@JaneLiuL
Copy link
Member

JaneLiuL commented Mar 9, 2022

@eminaktas do you use helm chart install or use which for install? i need to have a test, also have to reinstall recently?
since pr #747 just merge about 9 days ago, so you need to reinstall again for it

@eminaktas
Copy link
Contributor Author

@eminaktas do you use helm chart install or use which for install? i need to have a test, also have to reinstall recently? since pr #747 just merge about 9 days ago, so you need to reinstall again for it

I used the latest image k8s.gcr.io/descheduler/descheduler:v0.23.1. Whether you use the latest helm or this image, you should get the same results I've got.

@JaneLiuL
Copy link
Member

JaneLiuL commented Mar 9, 2022

thanks for your update~ i will have a try now :)

@JaneLiuL
Copy link
Member

JaneLiuL commented Mar 9, 2022

before which docker image you use? still k8s.gcr.io/descheduler/descheduler:v0.23.1 ?

@damemi
Copy link
Contributor

damemi commented Mar 9, 2022

I don't think this is going to be fixable in the current releases without publishing no-op changes and making new tags. Given the scope of this issue, I don't think it's worth the effort (but if others feel differently, we can still do it).

What we can do is sort out what order we do release steps for future releases to fix this. For v0.24, I have put what I think is the right order to address this in that release tracking issue (#735).

The reason you see descheduler-helm-chart... as the version is because of the automated Helm Chart Release action that runs when we merge any changes to a release-xx branch. That release action automatically creates a git tag to show the change in the Helm chart.

So, we need to re-think when we are cutting the release-xx branch relative to when we tag the actual release (v0.2x). If we can get the steps properly sorted out, it would be even better if we could automate the entire process.

@damemi
Copy link
Contributor

damemi commented May 20, 2022

This is a tricky issue, so I'm sorry for the long post. But I think everyone understanding the details of this is worthwhile.

To summarize the issue, we have two relevant components to each release which are conflicting here:

  • the gcr.io descheduler image
  • the official helm chart

GCR image summary:
We publish the gcr.io image by promoting the SHA of an automatically-built image to an official tag. Those auto-built images are created whenever a new PR is merged to any descheduler repo branch.

GCR image problem:
When we publish a release, we try to git tag the master/release branch (eg, with v0.24.0) at the time of publishing the release. And since we need an official gcr.io image to share for that release, there must already be a promoted image from one of the auto-built images above. This causes the problem where the most recent published image shows the previous version in its output: the automatically-built images come from the master branch that was most recently tagged with the previous release's version.

GCR image proposed solution:
One fix for this is to git tag the release on master before all final release PRs have merged. For example, things like README updates which don't affect the code. Those PRs would create a new auto-built image that can then be promoted to GCR with the right git tag, returning the right version.

However, this won't necessarily work with patch releases, as I'm realizing by reading #801

This is because patch releases are not built from master but from the release-X.XX branch corresponding to the patch. And when a commit for a patch is picked to that branch, the auto-built image gets built with the previous minor version. We then promote this minor version patch similarly. But the minor version image won't include the patch-version git tag unless we push some changes after creating that tag to trigger a new auto-built image.

Obviously, pushing changes to a release version after tagging that release is not ideal. Compounding this is the Helm-chart releaser action.

Helm-chart releaser:
This action runs automatically when the following occur:

  • A change is merged to any release-XX branch
  • The change contains updates to the helm chart (we update the version and appVersion with each release)

When this happens, the action updates the official helm chart and creates a new git tag like descheduler-helm-chart-0.23.2 on that branch (this is what you saw above in #736 (comment), the code that was promoted was still tagged with the previous helm chart update's tag).

Finally, the helm-chart releaser action won't run if it doesn't find any changes in the git diff between the current branch and the latest tag.

Based on all of the above, I propose that for all future releases we make Helm chart changes the final step in each release. I think decoupling the Chart from the code is acceptable, and this should allow us to make sure the right git tag is built into each promoted image following the processes below for major and patch releases:
Descheduler release process

@seanmalloy @ingvagabund @JaneLiuL does this flow look good to you? Want to sanity-check it so everyone's on the same page moving forward

@seanmalloy
Copy link
Member

Some things from my perspective ...

An automated container image is also built and pushed to the staging registry when a GitHub release is created. For example see below for the image that was created when the GitHub release was created for v0.23.1.

docker pull gcr.io/k8s-staging-descheduler/descheduler@sha256:e3515727be2e5976ff643aaa6354722aba1d83edc281acd932e7af7753618e7e
docker save gcr.io/k8s-staging-descheduler/descheduler@sha256:e3515727be2e5976ff643aaa6354722aba1d83edc281acd932e7af7753618e7e > image.tar
# do some tar  and cd commands
./descheduler version
Descheduler version {Major:0 Minor:23+ GitVersion:v20220301-v0.23.1 GitBranch: GitSha1: BuildDate:2022-03-01T17:06:46+0000 GoVersion:go1.17.7 Compiler:gc Platform:linux/amd64}

Also here is the screen shot of these images in the descheduler staging registry.
image

Note the BuildDate from the output of descheduler version, the upload date column from the staging registry, and the date from the GtiHub release all match(March 1st). This leads me to believe that the build automation is triggered when a GitHub release is created. Note the date the git tag was created for v0.23.1 was Feb 28th.

So I think we could instead just promote the container image build that is being triggered when the GitHub release is created.

I have not looked into the details of how the helm release GitHub Action works.

@damemi
Copy link
Contributor

damemi commented May 20, 2022

@seanmalloy I don't think creating the GitHub release triggers a build, only merges. The build date and staging upload date will always match, but the PR to promote that image merged ~30min before I published the release. So I think this was just coincidental quick turnaround in getting the promoted image released.

We also shouldn't publish the release without an image already available to point to in the release notes

@damemi
Copy link
Contributor

damemi commented May 23, 2022

I just un-held the v0.24.0 promoted images. So once those merge, I'll cut the release-1.24 branch and publish the release.

Following the flow above, cutting the release-1.24 branch will trigger the Helm chart release action for v0.24.0. Once that runs (ie, v0.24.0 is officially done), we can tag v0.24.1 off the release-1.24 branch (since the go1.18.2 update will already be "picked" when I cut the branch).

With the patch tag created, we need to push a non-Helm-related change to trigger a new auto-build with the v0.24.1 git tag. Once that image is built, we can promote it upstream.

Finally, we will finish the patch release by merging the Helm version updates to release-1.24. This will trigger the Helm release action, and it will create the new descheduler-helm-chart-... git tag.

If we follow the diagram I posted for future releases, this should go smoother. It's a little messy this time because I was still figuring it out.

Does that all sound good?

@ingvagabund
Copy link
Contributor

@damemi is Merge helm chart update to release-1.xx branch node on the "Patch release" path marked as a "code changes" (blue) as well? IIUC the whole idea of the workflow is to merge a code/readme/.. change after a new tag was created which will trigger yet another image auto-build. The new image will then have the right tag (a.k.a the version go code will have the right version injected).

Worth sharing the workflow graph in https://github.com/kubernetes-sigs/descheduler/blob/master/docs/release-guide.md as well. It looks amazing!!!

@damemi
Copy link
Contributor

damemi commented May 24, 2022

is Merge helm chart update to release-1.xx branch node on the "Patch release" path marked as a "code changes" (blue) as well?

Correct, it's technically both (a code change and a Helm release trigger) but I felt like it was more important to highlight that it was a release triggering action.

I'll definitely add this to the docs, I think from what we learned in this release we could add some stuff to the checklist to make sure everything gets caught.

At this point though, I think we've resolved this issue. So I'm going to close it. Thanks again for reporting this @eminaktas !
/close

@k8s-ci-robot
Copy link
Contributor

@damemi: Closing this issue.

In response to this:

is Merge helm chart update to release-1.xx branch node on the "Patch release" path marked as a "code changes" (blue) as well?

Correct, it's technically both (a code change and a Helm release trigger) but I felt like it was more important to highlight that it was a release triggering action.

I'll definitely add this to the docs, I think from what we learned in this release we could add some stuff to the checklist to make sure everything gets caught.

At this point though, I think we've resolved this issue. So I'm going to close it. Thanks again for reporting this @eminaktas !
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

6 participants