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.

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

Add CI runners matrix (ubuntu 20.04, 22.04) #3409

Closed
wants to merge 1 commit into from
Closed

Add CI runners matrix (ubuntu 20.04, 22.04) #3409

wants to merge 1 commit into from

Conversation

yankay
Copy link
Member

@yankay yankay commented Nov 1, 2023 •
edited
Loading

Upgrade the CI environment by adding the ubuntu 22.04 to the CI.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 1, 2023
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 1, 2023
@yankay yankay mentioned this pull request Nov 1, 2023
Closed
@yankay
Copy link
Member Author

yankay commented Nov 1, 2023

/retest

neolit123
neolit123 reviewed Nov 1, 2023
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/lgtm

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

/lgtm
/retest

Failing tests are for the cgroupv2 workflow in setting up the Fedora Vagrant box. So shouldn't have anything to do with this change.

AkihiroSuda
AkihiroSuda reviewed Nov 1, 2023
View reviewed changes
.github/workflows/docker.yaml Outdated
@@ -14,7 +14,7 @@ permissions:
jobs:
docker:
name: Docker
runs-on: ubuntu-20.04
runs-on: ubuntu-22.04
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to keep 20.04 too to test cgroup v1

Copy link
Contributor

Choose a reason for hiding this comment

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

that is a good suggestion, is prow using cgroupsv2 @BenTheElder ?

Copy link
Member

Choose a reason for hiding this comment

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

that is a good suggestion, is prow using cgroupsv2 @BenTheElder ?

AFAIK the GKE clusters are not yet, though it's possible to do that. kubernetes/k8s.io#5276
I'm not sure for the EKS cluster.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should depend on which cgroups the prowjobs are on.

Copy link
Member

Choose a reason for hiding this comment

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

ping @yankay

Copy link
Member

Choose a reason for hiding this comment

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

Too early to drop cgroup v1

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @AkihiroSuda

The code has been changed to keep Ubuntu 20.04 and add Ubuntu 22.04 to "docker CI."

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the final state? Ubuntu 20.04 equal to.cgroupsv1 testing and 22.04 to cgroupsv2 ?

If that's the case can we make it more evident?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, nevermind, I'll figure it out later

Copy link
Contributor

Choose a reason for hiding this comment

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

But this makes.the ci to.run 14 jobs on presubmit, seems excessive

@AkihiroSuda AkihiroSuda mentioned this pull request Jan 21, 2024
Merged
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 22, 2024
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 22, 2024
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 22, 2024
@yankay yankay changed the title upgrade ci runners to ubuntu 22.04 Add CI runners matrix (ubuntu 20.04, 22.04) Jan 22, 2024
AkihiroSuda
AkihiroSuda approved these changes Jan 22, 2024
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AkihiroSuda, yankay
Once this PR has been reviewed and has the lgtm label, please assign aojea 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:

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

@aojea
Copy link
Contributor

aojea commented Apr 1, 2024

I´m going to close these as this will make the presubmits too expensive to maintain (in terms of support and time to run) for any PR

@aojea aojea closed this Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenTheElder BenTheElder BenTheElder left review comments

@neolit123 neolit123 neolit123 left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@aojea aojea Awaiting requested review from aojea

Assignees

@neolit123 neolit123

@stmcginnis stmcginnis

@AkihiroSuda AkihiroSuda

Labels
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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yankay @stmcginnis @k8s-ci-robot @aojea @neolit123 @BenTheElder @AkihiroSuda