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

Run rh-sign-image-cosign signing in parallel [CLOUDDST-25034] #701

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

midnightercz
Copy link
Contributor

rh-sign-image-cosign now checks for existing signatures and sign only when signature for given identity and digest is not found.
Also whole procedure now runs in parallel.

Copy link

openshift-ci bot commented Nov 20, 2024

Hi @midnightercz. Thanks for your PR.

I'm waiting for a konflux-ci member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@midnightercz
Copy link
Contributor Author

I still need to apply some retry mechanism

@midnightercz midnightercz marked this pull request as draft November 20, 2024 17:10
@midnightercz midnightercz force-pushed the parallel-cosign-signing branch from d2207ec to 2772ff1 Compare November 21, 2024 12:02
@midnightercz midnightercz force-pushed the parallel-cosign-signing branch 3 times, most recently from 804a0c8 to df53da2 Compare November 21, 2024 12:21
@midnightercz midnightercz marked this pull request as ready for review November 21, 2024 12:23
@johnbieren
Copy link
Collaborator

rh-sign-image-cosign now checks for existing signatures and sign only when signature for given identity and digest is not found. Also whole procedure now runs in parallel.

So this makes the task idempotent too? Sounds like it may resolve https://issues.redhat.com/browse/RELEASE-1251

Copy link
Collaborator

@johnbieren johnbieren left a comment

Choose a reason for hiding this comment

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

All the tests have concurrentLimit: 1. It would be nice to have one testing the parallel functionality since this PR introduces it

@midnightercz
Copy link
Contributor Author

rh-sign-image-cosign now checks for existing signatures and sign only when signature for given identity and digest is not found. Also whole procedure now runs in parallel.

So this makes the task idempotent too? Sounds like it may resolve https://issues.redhat.com/browse/RELEASE-1251

Well, it was idempotent even before - cosign checks before pushing signature to the server, point is that now it doesn't even build the signature layer.

All the tests have concurrentLimit: 1. It would be nice to have one testing the parallel functionality since this PR introduces it
Yes, I thought for testing it's fine. With concurrencyLimit set to >1 I need to change way how expected results are checked. But it's possible.

@johnbieren
Copy link
Collaborator

rh-sign-image-cosign now checks for existing signatures and sign only when signature for given identity and digest is not found. Also whole procedure now runs in parallel.

So this makes the task idempotent too? Sounds like it may resolve https://issues.redhat.com/browse/RELEASE-1251

Well, it was idempotent even before - cosign checks before pushing signature to the server, point is that now it doesn't even build the signature layer.

Okay, I am going to assign the ticket to you and put it in review since you did all the work here. Thank you!

All the tests have concurrentLimit: 1. It would be nice to have one testing the parallel functionality since this PR introduces it
Yes, I thought for testing it's fine. With concurrencyLimit set to >1 I need to change way how expected results are checked. But it's possible.

If it wouldn't take too long to add, I think it would be nice to have

@johnbieren
Copy link
Collaborator

Can you squash it down to one commit and make it pass gitlint (and rebase)? Then I will trigger the e2e

Copy link
Collaborator

@johnbieren johnbieren left a comment

Choose a reason for hiding this comment

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

One outstanding nit pick about the rm in the test cleanups, but otherwise lgtm. Can you fix the checkton issues, rebase and squash it down to one commit that fits gitlint standards?

@openshift-ci openshift-ci bot added the lgtm label Dec 2, 2024
@johnbieren johnbieren force-pushed the parallel-cosign-signing branch from ab6fafb to 94e8c27 Compare December 2, 2024 14:47
@openshift-ci openshift-ci bot removed the lgtm label Dec 2, 2024
@johnbieren
Copy link
Collaborator

/ok-to-test

@johnbieren
Copy link
Collaborator

@midnightercz okay to merge it if we get an e2e pass?

@johnbieren johnbieren force-pushed the parallel-cosign-signing branch from 94e8c27 to cf3c9c7 Compare December 3, 2024 01:53
@openshift-ci openshift-ci bot removed the lgtm label Dec 3, 2024
Copy link

openshift-ci bot commented Dec 3, 2024

New changes are detected. LGTM label has been removed.

@johnbieren
Copy link
Collaborator

/ok-to-test

@johnbieren johnbieren force-pushed the parallel-cosign-signing branch from cf3c9c7 to 4e081ba Compare December 3, 2024 12:44
@johnbieren
Copy link
Collaborator

/ok-to-test

1 similar comment
@johnbieren
Copy link
Collaborator

/ok-to-test

rh-sign-image-cosign now checks for existing signatures and
sign only when signature for given identity and digest is not
found.
Also whole procedure now runs in parallel.

Signed-off-by: Jindrich Luza <[email protected]>
@johnbieren johnbieren force-pushed the parallel-cosign-signing branch from 347c423 to c80d219 Compare December 5, 2024 13:41
@johnbieren
Copy link
Collaborator

/ok-to-test

@konflux-ci-qe-bot
Copy link

@midnightercz: The following test has Failed, say /retest to rerun failed tests.

PipelineRun Name Status Rerun command Build Log Test Log
konflux-e2e-tests-catalog-tlx6g Failed /retest View Pipeline Log View Test Logs

Inspecting Test Artifacts

To inspect your test artifacts, follow these steps:

  1. Install ORAS (see the ORAS installation guide).
  2. Download artifacts with the following commands:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/konflux-test-storage/konflux-team/release-service-catalog:konflux-e2e-tests-catalog-tlx6g

@johnbieren
Copy link
Collaborator

container "step-sign-image" in pod "managed-vlnqf-rh-sign-image-cosign-pod" is waiting to start: CreateContainerConfigError I am guessing the secret is missing PUBLIC_KEY but I will have to confirm after meetings

@johnbieren
Copy link
Collaborator

container "step-sign-image" in pod "managed-vlnqf-rh-sign-image-cosign-pod" is waiting to start: CreateContainerConfigError I am guessing the secret is missing PUBLIC_KEY but I will have to confirm after meetings

Yes, that is the issue. @midnightercz how do we derive the proper PUBLIC_KEY values to put in the secret?

@johnbieren
Copy link
Collaborator

@midnightercz ping

@openshift-merge-robot
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants