Add support of named and digest scylladb image reference with google cr #2491

grzywin · 2025-02-20T11:28:54Z

Description of your changes:
Add support of named and digest scylladb image reference with google cr. Supported syntax: <tag><digest>, <digest>, <tag> (also tags like "latest")

Which issue is resolved by this Pull Request:
Resolves #2455

scylla-operator-bot · 2025-02-20T11:28:57Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

scylla-operator-bot · 2025-02-20T11:28:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: grzywin
Once this PR has been reviewed and has the lgtm label, please assign rzetelskik for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ake update command

grzywin · 2025-02-25T10:11:51Z

/retest

zimnx

first batch

zimnx · 2025-02-25T11:30:50Z

assets/config/config.yaml

@@ -13,6 +13,6 @@ operator:
  prometheusVersion: "v2.54.1"
 operatorTests:
  scyllaDBVersions:
-    updateFrom: "6.2.0-rc2"
-    upgradeFrom: "6.1.2"
+    updateFrom: "sha256:cf00b8bfcce4d27e8527cad913e83db89af842e6cfb64ff4736bce83594b33d1"


It's unclear what it's being tested, use tag+sha format

zimnx · 2025-02-25T11:32:41Z

assets/config/config.yaml

@@ -1,5 +1,5 @@
 operator:
-  scyllaDBVersion: "6.2.0"
+  scyllaDBVersion: "sha256:5b53a7c60d9f9555bb87791ff29b2e633c6f472aec00de7afaf4db1addc6d594"


It's unclear what version it is. Use tag+sha format

zimnx · 2025-02-25T11:33:23Z

Makefile

@@ -463,7 +468,7 @@ endef
 # $1 - values.yaml
 define update-scylla-helm-versions
 	$(YQ) eval-all -i -P '\
-	select(fi==0).scyllaImage.tag = ( select(fi==1) | .operator.scyllaDBVersion ) | \
+	select(fi==0).scyllaImage.tag = ( select(fi==1) | "$(SCYLLADB_SEM_VER)" ) | \


Would you mind elaborating why this is changed?

When we execute make update command it is updating the manifests. Without this change the ScyllaDB version is taken directly from config.yaml file which at this moment I changed to digest. So in manifest we will something like below which won't work.

spec: version: sha256:5b53a7c60d9f9555bb87791ff29b2e633c6f472aec00de7afaf4db1addc6d594

Note: Just to clarify. My solution is not changing almost any logic directly in operator. I just added logic to handle digest and convert it to 'supported' version before it reaches the operator/manifests.

Note: Just to clarify. My solution is not changing almost any logic directly in operator. I just added logic to handle digest and convert it to 'supported' version before it reaches the operator/manifests.

Lets do other way around. Having manipulations in tests and scripts doesn't bring any value to the user. If Operator would support all valid formats of image references, then any transformation in test/scripts wouldn't be needed, and users could use all variants of image references.

I think as part of this feature, we could also restructure config.yaml to accept full Scylla image ref instead just version. Places where split between repository and other part is required would be handled by the scripts around assets.

Ok, I thought the goal of this change was to allow us to use the digest/tag/'latest', so that we have more control over the exact version we're testing, rather than to please the users.

zimnx · 2025-02-25T11:37:11Z

pkg/semver/version.go

@@ -47,3 +66,91 @@ func (sv ScyllaVersion) SupportFeatureUnsafe(featureVersion semver.Version) bool
 func (sv ScyllaVersion) SupportFeatureSafe(featureVersion semver.Version) bool {
 	return !sv.unknown && sv.version.GTE(featureVersion)
 }
+
+func GetImageVersionAndDigest(imageName, version string) (string, string, error) {


Based on the name it's unclear what is returned from this function.

Please also add unit tests for it.

zimnx · 2025-02-25T11:38:50Z

pkg/semver/version.go

+		return semVersion, version, nil
+	}
+
+	imageReference := fmt.Sprintf("scylladb/%s", imageName)


Lets not hardcode anything. ScyllaCluster spec accepts also repository which might not contain scylladb substring at all.

zimnx · 2025-02-25T11:39:38Z

pkg/semver/version.go

+func computeSemVersion(fullVersion string) (string, error) {
+	reSemVersion := regexp.MustCompile(`\d+\.\d+\.\d+`)
+	semVersion := reSemVersion.FindString(fullVersion)
+
+	if semVersion == "" {
+		return "", fmt.Errorf("could not extract semantic version from: %s", fullVersion)
+	}
+
+	reSuffix := regexp.MustCompile(`[~-]([a-zA-Z]+\d*)`)
+	suffixMatch := reSuffix.FindStringSubmatch(fullVersion)
+	if len(suffixMatch) > 1 {
+		semVersion = fmt.Sprintf("%s-%s", semVersion, suffixMatch[1])
+	}
+	return semVersion, nil
+}


https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
Why not using semver dependancy we already have?

I will take another look on that but semver dependency was not working for me here as for RCs ScyllaDB version label has a bit unusual format:
6.2.0~rc2-0.20241002.93700ff5d1ce@sha256:92dfd26d64c6d076c5a641b3526c63fd827c777bc8c0fbce3bfddc6c9913e9b6
As far as I had a problem with tilde sign.

zimnx · 2025-02-25T11:39:53Z

test/e2e/set/scyllacluster/scyllacluster_replace.go

@@ -172,7 +171,7 @@ var _ = g.Describe("ScyllaCluster", func() {
 	},
 		g.Entry(describeEntry, &entry{
 			scyllaImageRepository: scyllaOSImageRepository,
-			scyllaVersion:         configassests.Project.Operator.ScyllaDBVersion,
+			scyllaVersion:         framework.TestContext.ScyllaDBVersion,


Why this is changed?

With my previous commit b2d7fb1 I added flags support.
This line is actually an oversight which should be changed/added in the commit above.

zimnx · 2025-02-25T12:25:56Z

pkg/cmd/tests/options.go

+	if _, err := semver.Parse(o.ScyllaDBVersion); err != nil {
+		semScyllaDBVersion, fullScyllaDBVersion, err := scyllasemver.GetImageVersionAndDigest("scylla", o.ScyllaDBVersion)
+		if err != nil {
+			return fmt.Errorf("failed to resolve ScyllaDB version: %w", err)
+		}
+		fmt.Println("Extracted", semScyllaDBVersion, "from ScyllaDB version:", fullScyllaDBVersion)
+
+		o.ScyllaDBVersion = semScyllaDBVersion
+	}
+
+	if !tagOrDigestRegexp.MatchString(o.ScyllaDBUpdateFrom) {


Shouldn't all of this be part of Operator not the tests?
We shouldn't manipulate the input parameters there to transform it to format accepted by the Operator, but Operator should accept any valid image reference and support it.

scylla-operator-bot · 2025-02-27T02:25:22Z

PR needs rebase.

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-sigs/prow repository.

scylla-operator-bot · 2025-02-28T15:37:50Z

@grzywin: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gke-multi-datacenter-parallel	`2a7a7ae`	link	true	`/test e2e-gke-multi-datacenter-parallel`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

scylla-operator-bot · 2025-03-31T11:17:18Z

The Scylla Operator project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 30d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out

/lifecycle stale

scylla-operator-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2025

scylla-operator-bot bot requested review from mflendrich and rzetelskik February 20, 2025 11:28

scylla-operator-bot bot added the area/dependency Issues or PRs related to dependency changes label Feb 20, 2025

scylla-operator-bot bot requested a review from zimnx February 20, 2025 11:28

scylla-operator-bot bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 20, 2025

grzywin force-pushed the add-support-of-named-and-digest-scylladb-image-reference-google-cr branch 5 times, most recently from cad74fd to 3209648 Compare February 21, 2025 11:57

scylla-operator-bot bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Feb 21, 2025

grzywin force-pushed the add-support-of-named-and-digest-scylladb-image-reference-google-cr branch from eeeda12 to 43394ad Compare February 21, 2025 11:58

scylla-operator-bot bot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Feb 21, 2025

grzywin marked this pull request as ready for review February 21, 2025 12:14

scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 21, 2025

grzywin force-pushed the add-support-of-named-and-digest-scylladb-image-reference-google-cr branch 2 times, most recently from da639b8 to 1d114b8 Compare February 22, 2025 08:30

grzywin added the kind/machinery Categorizes issue or PR as related to Makefile, scripts or similar changes. label Feb 22, 2025

scylla-operator-bot bot removed the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Feb 22, 2025

grzywin added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Feb 22, 2025

scylla-operator-bot bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 22, 2025

grzywin marked this pull request as draft February 25, 2025 08:52

scylla-operator-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2025

grzywin force-pushed the add-support-of-named-and-digest-scylladb-image-reference-google-cr branch from 1d114b8 to 21a0f03 Compare February 25, 2025 08:54

scylla-operator-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 25, 2025

grzywin force-pushed the add-support-of-named-and-digest-scylladb-image-reference-google-cr branch 2 times, most recently from 989ab65 to 57cf6e7 Compare February 25, 2025 08:56

scylla-operator-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 25, 2025

grzywin force-pushed the add-support-of-named-and-digest-scylladb-image-reference-google-cr branch from 3fd1dcb to c7414b0 Compare February 25, 2025 08:58

grzywin added 3 commits February 25, 2025 10:05

Add support of names and digest ScyllaDB image reference

65ca648

Update makefile to handle digest in config.yaml during execution of m…

b47d85c

…ake update command

Update dependencies

2a7a7ae

grzywin force-pushed the add-support-of-named-and-digest-scylladb-image-reference-google-cr branch from 4b0ff85 to 2a7a7ae Compare February 25, 2025 09:06

grzywin marked this pull request as ready for review February 25, 2025 09:06

scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2025

zimnx reviewed Feb 25, 2025

View reviewed changes

scylla-operator-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2025

scylla-operator-bot bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support of named and digest scylladb image reference with google cr #2491

Add support of named and digest scylladb image reference with google cr #2491

grzywin commented Feb 20, 2025 •

edited

Loading

scylla-operator-bot bot commented Feb 20, 2025

scylla-operator-bot bot commented Feb 20, 2025

grzywin commented Feb 25, 2025

zimnx left a comment

zimnx Feb 25, 2025

zimnx Feb 25, 2025

zimnx Feb 25, 2025

grzywin Feb 25, 2025 •

edited

Loading

zimnx Feb 25, 2025 •

edited

Loading

zimnx Feb 25, 2025 •

edited

Loading

grzywin Feb 25, 2025

zimnx Feb 25, 2025

zimnx Feb 25, 2025

zimnx Feb 25, 2025

zimnx Feb 25, 2025

grzywin Feb 25, 2025 •

edited

Loading

zimnx Feb 25, 2025

grzywin Feb 25, 2025

zimnx Feb 25, 2025

scylla-operator-bot bot commented Feb 27, 2025

scylla-operator-bot bot commented Feb 28, 2025

scylla-operator-bot bot commented Mar 31, 2025

Add support of named and digest scylladb image reference with google cr #2491

Are you sure you want to change the base?

Add support of named and digest scylladb image reference with google cr #2491

Conversation

grzywin commented Feb 20, 2025 • edited Loading

scylla-operator-bot bot commented Feb 20, 2025

scylla-operator-bot bot commented Feb 20, 2025

grzywin commented Feb 25, 2025

zimnx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzywin Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

zimnx Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

zimnx Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grzywin Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scylla-operator-bot bot commented Feb 27, 2025

scylla-operator-bot bot commented Feb 28, 2025

scylla-operator-bot bot commented Mar 31, 2025

grzywin commented Feb 20, 2025 •

edited

Loading

grzywin Feb 25, 2025 •

edited

Loading

zimnx Feb 25, 2025 •

edited

Loading

zimnx Feb 25, 2025 •

edited

Loading

grzywin Feb 25, 2025 •

edited

Loading