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

ROX-22474: push Konflux images to quay.io/rhacs-eng #1674

Merged
merged 2 commits into from
May 21, 2024

Conversation

tommartensen
Copy link
Contributor

@tommartensen tommartensen commented May 15, 2024

Description

Switch to push images to quay.io/rhacs-eng.
Creates images like:

  • quay.io/rhacs-eng/collector:3.18.x-231-g2c316751d5-fast-latest
  • quay.io/rhacs-eng/collector:3.18.x-231-g2c316751d5-fast-slim

Checklist

  • Investigated and inspected CI test results
  • Updated documentation accordingly

Automated testing

  • Added unit tests
  • Added integration tests
  • Added regression tests

If any of these don't apply, please comment below.

Testing Performed

Konflux passing with correct image names is sufficient.

@tommartensen tommartensen self-assigned this May 15, 2024
@tommartensen tommartensen requested a review from a team as a code owner May 15, 2024 11:42
@@ -27,7 +27,7 @@ spec:
- name: image-expires-after
value: '13w'
- name: output-image-repo
value: quay.io/redhat-user-workloads/rh-acs-tenant/acs/collector-slim
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remind me, will main, roxctl and operator images think it is a release build in the sense of this code block here https://github.com/stackrox/stackrox/blob/896452897dbc54bb60183782731dda95766e3a20/pkg/images/defaults/flavor.go#L103-L112?
If yes, the repo is fine. If no, -slim should be part of the tag, not repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All three images have GOTAGS=release set, like https://github.com/stackrox/stackrox/blob/master/image/rhel/konflux.Dockerfile#L85.

If I understand this correctly, https://github.com/stackrox/stackrox/blob/896452897dbc54bb60183782731dda95766e3a20/pkg/images/defaults/flavor.go#L107 is true.

version.GetAllVersionsUnified would set the collector & scanner versions to the same as main (make tag of stackrox/stackrox instead of {COLLECTOR, SCANNER}_VERSION (https://github.com/stackrox/stackrox/blob/master/pkg/version/version.go#L72-L77)), BUT this is overriden in L109, L111, L128.

https://github.com/stackrox/stackrox/blob/master/pkg/version/version.go#L72-L77

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TLDR: I think it's fine and the assumption that SCANNER_VERSION and COLLECTOR_VERSION tags will be generated should also hold.

Copy link
Contributor

Choose a reason for hiding this comment

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

BUT this is overriden in L109, L111, L128

Overridden to what? GetAllVersionsUnified will return MainVersion there.

With GOTAGS=release:

  • quay.io/rhacs-eng/collector:<MainVersion>
  • quay.io/rhacs-eng/collector-slim:<MainVersion>

Without:

  • quay.io/rhacs-eng/collector:<CollectorVersion>-latest
  • quay.io/rhacs-eng/collector:<CollectorVersion>-slim

"<CollectorVersion>" is what we get by running make tag in stackrox/collector repo.
"<MainVersion>" is make tag result in stackrox/stackrox, i.e. a retagged image. https://github.com/stackrox/stackrox/blob/7690ea6ed07fe68b85aac8810b32c9d7b640e8fb/.github/workflows/build.yaml#L400-L408

We don't have a way to retag in Konflux unless we write a custom task which I at the moment doubt is the right choice.

-slim and -latest in suffix were introduced as a compatibility thing to not break old workflows. Maybe this is no longer necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think, the simplest for MVP^2 may be to switch the release off, push to -latest+-slim, and plan sorting out release in GA scope.

Copy link
Contributor Author

@tommartensen tommartensen May 15, 2024

Choose a reason for hiding this comment

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

I think, the simplest for MVP^2 may be to switch the release off, push to -latest+-slim, and plan sorting out release in GA scope.

IMO we can merge stackrox/stackrox#10824, (this can already be done without merging)

We can deploy a Central cluster and see what it would generate for collector(-slim). It might just work.

Copy link
Contributor

@msugakov msugakov May 15, 2024

Choose a reason for hiding this comment

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

I realized yesterday that we might have to -fast in that file

We should not add suffixes to COLLECTOR_VERSION or SCANNER_VERSION because it can break builds outside of Konflux and/or product built outside of Konflux. We should look for other solution (overriding BUILD_TAG was my first candidate) - https://issues.redhat.com/browse/ROX-19958

So there are actually no images like quay.io/rhacs-eng/collector-slim:<CollectorVersion>.

Correct! And I think it proves my point, doesn't it?

Is the development_build flavor && releaseBuild path unused?

I believe it executes for nightly builds and any other tagged builds.

We can deploy a Central cluster and see what it would generate for collector(-slim).

There's a quicker way to check:

$ cd tmp
$ podman run --rm -it --entrypoint=/bin/bash -u 0:0 -v ./:/tmp quay.io/redhat-user-workloads/rh-acs-tenant/acs/roxctl:4.4.x-685-g7690ea6ed0-fast
$ roxctl helm output secured-cluster-services --output-dir /tmp/chart-konflux --remove --image-defaults=development_build
INFO:	Written Helm chart secured-cluster-services to directory "/tmp/chart-konflux"
$ exit
$ /assets/downloads/cli/roxctl-linux helm output secured-cluster-services --output-dir /tmp/chart-gha --remove --image-defaults=development_build
INFO:	Written Helm chart secured-cluster-services to directory "/tmp/chart-gha"
$ exit

(I used some successful build on master)

Then, study internal/defaults/50-images.yaml in both dirs.

/tmp/chart-konflux/internal/defaults/50-images.yaml has:

image:
  registry: quay.io/rhacs-eng
  main:
    name: main
    pullPolicy: IfNotPresent
  collector:
    name: collector
    slimName: collector-slim
# ...
image:
  collector:
    {{- if ._rox.collector.slimMode }}
    tag: "4.4.x-685-g7690ea6ed0"
    pullPolicy: IfNotPresent
    {{- else }}
    tag: "4.4.x-685-g7690ea6ed0"
    pullPolicy: Always
    {{- end }}

/tmp/chart-gha/internal/defaults/50-images.yaml:

image:
  registry: quay.io/rhacs-eng
  main:
    name: main
    pullPolicy: IfNotPresent
  collector:
    name: collector
    slimName: collector
# ...
image:
  collector:
    {{- if ._rox.collector.slimMode }}
    tag: "3.18.x-221-g580b954a9a-slim"
    pullPolicy: IfNotPresent
    {{- else }}
    tag: "3.18.x-221-g580b954a9a-latest"
    pullPolicy: Always
    {{- end }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you notice that scanner.tag is also the <MainVersion>, not <ScannerVersion>?

  scanner:
    name: scanner-slim
    tag: 4.4.x-685-g7690ea6ed0

I have to think about the other points..

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, that's the "unified versions" case of release builds, I expected it (we came up with this code in the Maple team while working on that image flavor stuff that occurred after onboarding all containers to CPaaS).

Copy link
Contributor Author

@tommartensen tommartensen May 16, 2024

Choose a reason for hiding this comment

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

From discussion in Weekly:

@tommartensen tommartensen requested a review from msugakov May 15, 2024 12:25
@tommartensen
Copy link
Contributor Author

/retest

1 similar comment
@tommartensen
Copy link
Contributor Author

/retest

@tommartensen tommartensen merged commit c5e8688 into master May 21, 2024
52 of 55 checks passed
@tommartensen tommartensen deleted the tm/ROX-22474-konflux-push-to-quay-io-rhacs-eng branch May 21, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants