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

update subdomain tests #194

Merged

Conversation

vsoch
Copy link
Contributor

@vsoch vsoch commented Jun 16, 2023

Problem: there are duplicated attributes for subdomain/networksubdomain, and setting the subdomain on the result should be done by the JobSet.

Solution: remove the duplicate (subdomain) and manual setting.

This should hopefully help with #181 if the issue is related!

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 16, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @vsoch. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 16, 2023
@ahg-g
Copy link
Contributor

ahg-g commented Jun 16, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 16, 2023
test/e2e/e2e_test.go Outdated Show resolved Hide resolved
Problem: there are duplicated attributes for subdomain/networksubdomain,
and setting the subdomain on the result should be done by the JobSet.
Solution: remove the duplicate (subdomain) and manual setting.

Signed-off-by: vsoch <vsoch@users.noreply.github.com>
@vsoch
Copy link
Contributor Author

vsoch commented Jun 16, 2023

Looks like the gcr registry might be having a moment!
image

I suspect it's crying because of the impending loss of it's colleagues, Google Domains and Google Album Archive! 😭 A moment of silence for our friends.

@vsoch
Copy link
Contributor Author

vsoch commented Jun 16, 2023

/retest-required

@ahg-g
Copy link
Contributor

ahg-g commented Jun 17, 2023

/test pull-jobset-test-e2e-main-1-24

@danielvegamyhre
Copy link
Contributor

/test all

rerunning tests to check flakiness

@vsoch
Copy link
Contributor Author

vsoch commented Jun 17, 2023

That's weird it is based on the container build (and finding packages).

@danielvegamyhre
Copy link
Contributor

/retest

Yeah I'm hoping #191 will help resolve the unit test flakiness (for some reason apt update fails to update all package repositories somewhat frequently).

@vsoch
Copy link
Contributor Author

vsoch commented Jun 18, 2023

I don't know the details of the differences, but fwiw I usually do apt-get update always first, and I don't mix apt and apt-get. They are supposed to be mostly the same (and apt alone has extra UI niceties) but I seem to remember command line warnings sometimes that there may be issues in a headless environment. So my goto is always:

ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update && apt-get install -y <package>

Or I may just be an old crusty linux person, that is very likely too! 😆

@ahg-g
Copy link
Contributor

ahg-g commented Jun 18, 2023

/retest

@ahg-g
Copy link
Contributor

ahg-g commented Jun 18, 2023

/retest all

@k8s-ci-robot
Copy link
Contributor

@ahg-g: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test pull-jobset-test-e2e-main-1-24
  • /test pull-jobset-test-e2e-main-1-25
  • /test pull-jobset-test-e2e-main-1-26
  • /test pull-jobset-test-integration-main
  • /test pull-jobset-test-unit-main
  • /test pull-jobset-verify-main

Use /test all to run all jobs.

In response to this:

/retest all

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/test-infra repository.

@ahg-g
Copy link
Contributor

ahg-g commented Jun 18, 2023

/test all

@vsoch
Copy link
Contributor Author

vsoch commented Jun 18, 2023

I think we might try using apt-get in Dockerfile and scripts, e.g.,:

WARNING: apt does not have a stable CLI interface. Use with caution in scripts.

That's the warning I was mentioning earlier. I'm going to try running this manually to see if I can reproduce.

@vsoch
Copy link
Contributor Author

vsoch commented Jun 18, 2023

okay looks like you already did that! https://github.com/kubernetes-sigs/jobset/pull/192/files

@vsoch
Copy link
Contributor Author

vsoch commented Jun 18, 2023

What is / where is the base image that this testing setup called prow is using?

@tenzen-y
Copy link
Member

What is / where is the base image that this testing setup called prow is using?

here: https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes-sigs/jobset/jobset-presubmit.yaml

@vsoch
Copy link
Contributor Author

vsoch commented Jun 18, 2023

here: https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes-sigs/jobset/jobset-presubmit.yaml

Thank you - perfect! And I actually I saw (I think?) the same populated one here: https://prow.k8s.io/prowjob?prowjob=9c8ca3c0-37c3-4ba5-b35e-76b15bf25415 (albeit it's not as nice to look at!). Okay so if we are using the golang 1.20 base, I can reproduce:

root@0f2340563f52:/go# apt-get update && apt-get install -y openjdk-11-jdk
Get:1 http://deb.debian.org/debian bookworm InRelease [147 kB]
Get:2 http://deb.debian.org/debian bookworm-updates InRelease [52.1 kB]
Get:3 http://deb.debian.org/debian-security bookworm-security InRelease [48.0 kB]
Get:4 http://deb.debian.org/debian bookworm/main amd64 Packages [8904 kB]
Get:5 http://deb.debian.org/debian-security bookworm-security/main amd64 Packages [28.3 kB]
Fetched 9180 kB in 3s (3169 kB/s)                     
Reading package lists... Done
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
E: Unable to locate package openjdk-11-jdk

I tried explicitly adding the repository - it didn't seem to work (but here are the steps if anyone is interested)

apt-get update && apt-get install -y software-properties-common python3-launchpadlib 

To add the repository:

add-apt-repository ppa:openjdk-r/ppa

(DEBIAN_FRONTEND=nonteractive I think would be important for that because it usually asks you to press ENTER)
But it's still not found. So I stepped back and checked this page:

https://endoflife.date/java

And it seems opendjk-11-jdk is going EOL this year - it's a long term support version but seems to end in 3 months:

Perhaps we can use openjdk-17-jdk instead? That works for me in a fresh image:

apt-get install -y openjdk-17-jdk

I'm going to poke around and see if there are any notices for why the package isn't found. But I think we might do well to update to a version that isn't going EOL soon anyway.

@vsoch
Copy link
Contributor Author

vsoch commented Jun 18, 2023

okay more debugging - this shows a lot of results (for version 17)

$ apt search openjdk

This shows no results:

$ apt search openjdk-11
Sorting... Done
Full Text Search... Done

And I don't see the package under "bookworm" https://packages.debian.org/bookworm/openjdk-11-jdk.

It's definitely there for buster! https://packages.debian.org/buster/openjdk-11-jdk

And maybe this is the underlying issue - the golang base images for 19 and 20 were updated to have bookworm and not buster. docker-library/golang@db757a0 So I don't think we did anything here - we just need to update the libraries we used based on what is available. And note there are buster variants available: https://hub.docker.com/layers/library/golang/1.20-buster/images/sha256-f86220e98c9d858143a68158dde675a0178854549511d9fee469767edfefc0d7?context=explore if you absolutely cannot change, but based on the EOL of jdk 11 I think it's probably an OK time.

Hope that helps!

@vsoch
Copy link
Contributor Author

vsoch commented Jun 18, 2023

A suggestion from the Chainguard Dev CEO is to try their wolfi images: https://twitter.com/lorenc_dan/status/1670535938240389121?s=20. They are fairly minimalist, and not sure if it would greatly help with the EOL, but I thought I'd bring it up for discussion in case there is interest!

@ahg-g
Copy link
Contributor

ahg-g commented Jun 19, 2023

What I am confused about is how an error like "Unable to locate package openjdk-11-jdk" causes a flake? why would the test ever succeed if the package doesn't exist?

@vsoch
Copy link
Contributor Author

vsoch commented Jun 19, 2023

Because the golang images when they were buster did have the package. You likely were hitting a cache with the older images if it worked.

@ahg-g
Copy link
Contributor

ahg-g commented Jun 21, 2023

btw, in bookworm it is openjdk-17: https://packages.debian.org/bookworm/openjdk-17-jdk, so we just need to change the script at https://github.com/kubernetes-sigs/jobset/blob/main/hack/python-sdk/gen-sdk.sh#L48 to point to 17, right?

@vsoch
Copy link
Contributor Author

vsoch commented Jun 21, 2023

Yes! That's what I suggested we do here: #194 (comment)

Perhaps we can use openjdk-17-jdk instead? That works for me in a fresh image:
apt-get install -y openjdk-17-jdk

And the follow-up thread was just an exercise to understand why version 11 was not around!

@vsoch
Copy link
Contributor Author

vsoch commented Jun 21, 2023

Would you like me to put in a quick PR to do that update, or is that planned for https://github.com/kubernetes-sigs/jobset/pull/192/files?

@ahg-g
Copy link
Contributor

ahg-g commented Jun 21, 2023

yeah, to make it a bit more robust during this transition period, perhaps the script can try 11 first then if not successful try 17?

@vsoch
Copy link
Contributor Author

vsoch commented Jun 21, 2023

Sure, but I can almost guarantee since the images have been deployed (golang 19 and 20, specifically) to use bookworm, it will be unlikely. But I could see someone keeping a cache somewhere, so +1 to try 11 and fall back to 17.

@vsoch
Copy link
Contributor Author

vsoch commented Jun 21, 2023

👉 #201 👈

Can't wait to squash this bug! 🐛 🐞

@ahg-g
Copy link
Contributor

ahg-g commented Jun 21, 2023

/retest

@vsoch
Copy link
Contributor Author

vsoch commented Jun 21, 2023

I likely need to do a rebase here, but I can wait for the recently triggered tests to run.

@ahg-g
Copy link
Contributor

ahg-g commented Jun 21, 2023

I likely need to do a rebase here, but I can wait for the recently triggered tests to run.

it should automatically rebase

@ahg-g
Copy link
Contributor

ahg-g commented Jun 21, 2023

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, vsoch

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2023
@k8s-ci-robot k8s-ci-robot merged commit e7b7d5a into kubernetes-sigs:main Jun 21, 2023
8 checks passed
@vsoch vsoch deleted the tweaks-subdomain-testing branch June 21, 2023 17:34
@vsoch
Copy link
Contributor Author

vsoch commented Jun 21, 2023

Thank you!

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants