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

Add CI script to deploy published releases #1837

Merged
merged 1 commit into from
May 29, 2024

Conversation

tnozicka
Copy link
Contributor

@tnozicka tnozicka commented Mar 15, 2024

Description of your changes:
This script will be used in periodics to test our published images. It essentially codifies what needs to be done to deploy a particular scylla-operator version. Some action could be simplified by restructuring manifest but it's out of the scope of this PR. Eventually, we should fix this and possibly have some smarter templating / publish manifests into releases with the replaced image. So take this PR as a start with what we have now, not where it should end.

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

Requires

@tnozicka tnozicka added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/machinery Categorizes issue or PR as related to Makefile, scripts or similar changes. labels Mar 15, 2024
@scylla-operator-bot scylla-operator-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 15, 2024
@scylla-operator-bot scylla-operator-bot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2024
@tnozicka
Copy link
Contributor Author

/retest

@tnozicka
Copy link
Contributor Author

/test images

@tnozicka
Copy link
Contributor Author

/retest

@rzetelskik
Copy link
Member

rzetelskik commented Mar 19, 2024

How is this going to be set up with CI? https://github.com/scylladb/scylla-operator/blob/master/hack/.ci/run-e2e-gke.sh uses ./hack/ci-deploy.sh. Are you planning to send a separate PR configuring it? Or is this going to be used by an entirely separate script?

@tnozicka
Copy link
Contributor Author

this can't be used by presubmits by definition as their merged git shas are not published - this is only for published releases

@rzetelskik
Copy link
Member

this can't be used by presubmits by definition as their merged git shas are not published - this is only for published releases

I know, but the question was about how is this going to be integrated with CI, pointing at whether this PR is potentially missing any adjustments.

@tnozicka
Copy link
Contributor Author

I know, but the question was about how is this going to be integrated with CI, pointing at whether this PR is potentially missing any adjustments.

This will be used only by new periodics based on already published images (we don't have these periodics yet).
(Eventually, we can add an on demand presubmit using this script against a latest release to smoke test it.)

@tnozicka
Copy link
Contributor Author

@zimnx
Copy link
Collaborator

zimnx commented Mar 27, 2024

/retest

@zimnx
Copy link
Collaborator

zimnx commented Mar 27, 2024

Looks like there's a missing bracket in CI test script.

Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

/lgtm

please fix CI script
/hold

@scylla-operator-bot scylla-operator-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2024
@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2024
@rzetelskik
Copy link
Member

https://github.com/scylladb/scylla-operator-release/pull/163

@tnozicka
Copy link
Contributor Author

/retest

@rzetelskik
Copy link
Member

time="2024-03-28T13:38:18Z" level=fatal msg="Error parsing image name \"docker://docker.io/scylladb/scylla-operator:latest@sha256:0c8896a666fdc0d273b92dbd1bd936140d1da34e6c25b9f7ac1260a7acc18f16\": Docker references with both a tag and digest are currently not supported"

needs a fix

@tnozicka
Copy link
Contributor Author

tnozicka commented Apr 2, 2024

@tnozicka
Copy link
Contributor Author

@tnozicka
Copy link
Contributor Author

cluster didn't spin up in time
/retest

@tnozicka
Copy link
Contributor Author

oh, I've tested this but seems like it depends on extglob which I had enabled and the image doesn't. need to fix that in the CI config

@tnozicka
Copy link
Contributor Author

@tnozicka
Copy link
Contributor Author

(this need more work on artifact collection and specifying nodeconfig on CI side)

@tnozicka tnozicka changed the title Add CI script to deploy published releases [WIP] Add CI script to deploy published releases Apr 19, 2024
@tnozicka tnozicka changed the title [WIP] Add CI script to deploy published releases Add CI script to deploy published releases May 16, 2024
@scylla-operator-bot 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 May 16, 2024
@tnozicka tnozicka requested a review from zimnx May 16, 2024 12:53
@tnozicka
Copy link
Contributor Author

(this need more work on artifact collection and specifying nodeconfig on CI side)

done and fixed the script + CI setup - ready for a second round of review, thx

# Wait for cert-manager crd and webhooks
kubectl wait --for condition=established --timeout=60s crd/certificates.cert-manager.io crd/issuers.cert-manager.io
for d in cert-manager{,-cainjector,-webhook}; do
kubectl -n=cert-manager rollout status --timeout=5m "deployment.apps/${d}"
Copy link
Member

Choose a reason for hiding this comment

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

missing wait-for-object-creation from ci-deploy:

wait-for-object-creation cert-manager deployment.apps/"${d}"
. Is it superfluous there or accidentally missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't be needed, I don't think kubectl hits the watch cache.

Copy link
Member

Choose a reason for hiding this comment

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

should it be removed from the other script then? (not in this pr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so

Copy link
Member

Choose a reason for hiding this comment

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

should we have a tracker for this? Even if it's in the backlog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tnozicka tnozicka changed the title Add CI script to deploy published releases [WIP] Add CI script to deploy published releases May 22, 2024
@scylla-operator-bot 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 May 22, 2024
@tnozicka tnozicka changed the title [WIP] Add CI script to deploy published releases Add CI script to deploy published releases May 22, 2024
@scylla-operator-bot 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 May 22, 2024
@tnozicka
Copy link
Contributor Author

@rzetelskik please have a look at the comment replies, I'd like to land this and unblock your arm64 PR

@rzetelskik
Copy link
Member

@rzetelskik please have a look at the comment replies, I'd like to land this and unblock your arm64 PR

done

@tnozicka tnozicka requested a review from rzetelskik May 27, 2024 13:23
Copy link
Member

@rzetelskik rzetelskik left a comment

Choose a reason for hiding this comment

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

lgtm
/assign zimnx
in case you want to have a second look at it, it changed since your review. Otherwise feel free to remove the hold.

Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

One nit. Feel free to cancel hold even without applying, i'm fine with having both options.

/lgtm
/hold

Comment on lines +101 to +102
wait-for-object-creation scylla-manager statefulset.apps/scylla-manager-cluster-manager-dc-manager-rack
kubectl -n=scylla-manager rollout status --timeout=5m statefulset.apps/scylla-manager-cluster-manager-dc-manager-rack
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: these could be replaced with waiting for our three aggregated conditions on ScyllaCluster without hardcoding names of Operator managed resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good point but it's in ci-deploy.sh as well, I'll send a followup.
/hold cancel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label May 27, 2024
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rzetelskik, tnozicka, zimnx

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@tnozicka tnozicka reopened this May 28, 2024
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 29, 2024
@scylla-operator-bot scylla-operator-bot bot merged commit 6c4fa9f into scylladb:master May 29, 2024
13 checks passed
@tnozicka tnozicka deleted the deploy-release branch May 29, 2024 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/machinery Categorizes issue or PR as related to Makefile, scripts or similar changes. lgtm Indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CI script to deploy published releases
3 participants