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

Fix propagation of EFFECTIVE_VERSION during builds in pipeline #62

Merged
merged 3 commits into from
Sep 21, 2022

Conversation

vpnachev
Copy link
Member

@vpnachev vpnachev commented Sep 20, 2022

What this PR does / why we need it:
Fix propagation of EFFECTIVE_VERSION during builds in pipeline.
Also splits docker builder stage to speed-up image builds.

Which issue(s) this PR fixes:
Fixes #47

Special notes for your reviewer:
/invite @ialidzhikov

Release note:

A bug that the extension was using wrong version of the installation image when no image vector overwrite is configured is fixed.

@vpnachev vpnachev requested a review from a team as a code owner September 20, 2022 12:37
@gardener-robot gardener-robot added needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Sep 20, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 20, 2022
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

Are you sure that it is that trivial? In the past you did a similar change in #36 and then you reverted in #39.

@vpnachev
Copy link
Member Author

Good catch.
/hold

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Sep 20, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 20, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 20, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 20, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 20, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 20, 2022
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 20, 2022
@ialidzhikov ialidzhikov added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 20, 2022
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 20, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 21, 2022
@vpnachev
Copy link
Member Author

After gardener/cc-utils@2348bce, the EFFECTIVE_VERSION is set as build arg and the GitVersion is properly set.

$ docker run  --rm eu.gcr.io/gardener-project/gardener/extensions/runtime-gvisor:v0.7.0-dev-c94906ece86738c32a4558d2dba378aaa289837f-linux-amd64 --version=raw | grep -Po 'GitVersion:".*?"'
GitVersion:"v0.7.0-dev-c94906ece86738c32a4558d2dba378aaa289837f"

/title Fix propagation of EFFECTIVE_VERSION during builds in pipeline
/unhold

@gardener-robot gardener-robot changed the title Enable effective version injection during pipeline builds Fix propagation of EFFECTIVE_VERSION during builds in pipeline Sep 21, 2022
@gardener-robot gardener-robot removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Sep 21, 2022
@gardener-robot
Copy link

@gardener-robot Only code owners and the author may add reviewers. Commenters may only invite themselves.

@gardener-robot gardener-robot added size/s Size of pull request is small (see gardener-robot robot/bots/size.py) and removed size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Sep 21, 2022
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 21, 2022
@vpnachev
Copy link
Member Author

This arm64 build seems to take forever most of the time. It should be because no mater of the target, always the golang binaries are built or the gVisor binaries are pulled. With fdb8413, I've split the builder stage to avoid unnecessary build steps. Let's see if it will really improve.

@vpnachev
Copy link
Member Author

vpnachev commented Sep 21, 2022

Let's see if it will really improve.

For the amd64 builds there is already an improvement:

  • extension image build time is reduced from 90 to 60 seconds (no significant improvement for the arm64 so far)
  • installation image build time reduced from 90 to 14 seconds (320 to 30 seconds for arm64)

Unfortunately, it seems the arm64 builds are still prone to stuck.

Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

Minor suggestion, otherwise lgtm.

I can confirm that the effective version is correctly injected in the controller image/binary. It also works for images build with make docker-images.

Makefile Outdated Show resolved Hide resolved
vpnachev and others added 3 commits September 21, 2022 18:21
Co-authored-by: ialidzhikov <i.alidjikov@gmail.com>
This way each target builds/installs only what it actually needs:
- extension image does not pull the gVisor binaries
- installation image does not build extension binaries

This should speed up multiarch builds

Signed-off-by: Vladimir Nachev <vladimir.nachev@sap.com>
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 21, 2022
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks for picking this ad-hoc item!

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Sep 21, 2022
@ialidzhikov ialidzhikov merged commit f1c01fb into gardener:master Sep 21, 2022
@ialidzhikov
Copy link
Member

ialidzhikov commented Sep 21, 2022

I edited the release note as follow:

+A bug that the extension was using wrong version of the installation image when no image vector overwrite is configured is fixed.
-A bug that non-released version of the extension was using wrong version of the installation image when no image vector overwrite is configured is fixed.

as it also affect released versions. We don't hit it on our landscapes because we always use imageVectorOverwrite there.

@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Sep 21, 2022
@vpnachev vpnachev deleted the ci/inject-effective-version branch September 21, 2022 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) reviewed/lgtm Has approval for merging size/s Size of pull request is small (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Installation image is using wrong tag by default
6 participants