Skip to content

[WIP] kubetest2-kops: use ephemeral discovery store for AWS #17520

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[WIP] kubetest2-kops: use ephemeral discovery store for AWS #17520

wants to merge 1 commit into from
+26 −4

Conversation

xmudrii
Copy link
Member

@xmudrii xmudrii commented Jul 28, 2025

This PR is a follow up on kubernetes/k8s.io#5127 to use an ephemeral discovery store/S3 bucket instead of a static one (s3://k8s-kops-ci-prow).

Related to kubernetes/k8s.io#5127

cc @ameukam

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign olemarkus for approval. For more information see the 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:

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 28, 2025
@ameukam
Copy link
Member

ameukam commented Jul 28, 2025

cc @hakman @rifelpet
They know more about me for the discovery store.

ameukam
ameukam reviewed Jul 28, 2025
View reviewed changes
tests/e2e/kubetest2-kops/deployer/up.go Outdated
@@ -70,6 +70,9 @@ func (d *deployer) Up() error {
if err := d.aws.EnsureS3Bucket(ctx, d.stateStore(), false); err != nil {
return err
}
if err := d.aws.EnsureS3Bucket(ctx, d.discoveryStore(), false); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to make them public ? cc @rifelpet

Suggested change
if err := d.aws.EnsureS3Bucket(ctx, d.discoveryStore(), false); err != nil {
if err := d.aws.EnsureS3Bucket(ctx, d.discoveryStore(), true); err != nil {

Copy link
Member Author

Choose a reason for hiding this comment

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

The existing bucket (s3://k8s-kops-ci-prow) is public, but I'm not sure if that's because it's required by tests or because we want boskos accounts to be able to read from this bucket. Also, do we need to migrate existing data from there?

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, It's a requirement for the tests and I don't think we need to migrate any data.

Copy link
Member

Choose a reason for hiding this comment

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

I confirm, data is irrelevant. If this is the discovery bucket, we need it to be public, or put CloudFront in front of it.

@ameukam
Copy link
Member

ameukam commented Jul 28, 2025

/test pull-kops-e2e-k8s-aws-cilium-1-33

@ameukam
Copy link
Member

ameukam commented Jul 28, 2025

/test pull-kops-aws-distro-al2023

@hakman
Copy link
Member

hakman commented Jul 28, 2025

@xmudrii @ameukam tests are temporarily broken, so you may have to wait a few days with this.

@ameukam
Copy link
Member

ameukam commented Jul 28, 2025 •
edited
Loading

@xmudrii @ameukam tests are temporarily broken, so you may have to wait a few days with this.

@hakman The current state of CI don't prevent us to validate some aspects of the PR: :-P

I0728 12:16:03.905852   16831 s3.go:127] Bucket k8s-infra-kops-state-4d6a-20250728120558 created successfully
I0728 12:16:04.427119   16831 s3.go:127] Bucket k8s-infra-kops-discovery-4d6a-20250728121603 created successfully
Deleted cluster: "presubmit.test-cncf-aws.k8s.io"
I0728 12:19:33.040823   16831 s3.go:173] Bucket k8s-infra-kops-state-4d6a-20250728120558 deleted
I0728 12:19:33.294451   16831 s3.go:173] Bucket k8s-infra-kops-discovery-4d6a-20250728121603 deleted

@xmudrii
Copy link
Member Author

xmudrii commented Jul 28, 2025

@hakman @rifelpet Please let us know once the tests are fixed. As @ameukam pointed out, this already looks promising, I'll do some more tests once they are working

@ameukam
Copy link
Member

ameukam commented Jul 29, 2025

@xmudrii Mind rebase your PR to include #17523 ?

@kubernetes kubernetes deleted a comment from k8s-ci-robot Jul 29, 2025
@hakman
Copy link
Member

hakman commented Jul 29, 2025

/test all

@xmudrii xmudrii mentioned this pull request Jul 31, 2025
Merged
@xmudrii xmudrii marked this pull request as draft August 1, 2025 14:41
@xmudrii
Copy link
Member Author

xmudrii commented Aug 1, 2025

/test pull-kops-aws-distro-al2023

@xmudrii
Copy link
Member Author

xmudrii commented Aug 5, 2025

/test presubmit-kops-aws-boskos

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 5, 2025 •
edited
Loading

@xmudrii: 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
presubmit-kops-aws-boskos 76657d3 link true /test presubmit-kops-aws-boskos

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 5, 2025
@xmudrii
Copy link
Member Author

xmudrii commented Aug 5, 2025

/test pull-kops-aws-distro-al2023

@xmudrii xmudrii mentioned this pull request Aug 5, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hakman hakman hakman left review comments

@johngmyers johngmyers Awaiting requested review from johngmyers

@olemarkus olemarkus Awaiting requested review from olemarkus

+1 more reviewer

@ameukam ameukam ameukam left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xmudrii @k8s-ci-robot @ameukam @hakman