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

Do not test CNI in CI #21410

Merged
merged 2 commits into from Feb 6, 2024
Merged

Do not test CNI in CI #21410

merged 2 commits into from Feb 6, 2024

Conversation

ashley-cui
Copy link
Member

@ashley-cui ashley-cui commented Jan 29, 2024 •
edited

WIP.

5.0 build tags out CNI, let's drop those tests from our CI.

Probably needs many pushes to get it green, but I'll whack at this PR until then.

None

edsantiago
edsantiago reviewed Jan 29, 2024
View reviewed changes
Copy link
Collaborator

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

You might want to grep for NETWORK_BACKEND and excise those too.

LGTM at first pass. Really curious how the upgrade test will do...

contrib/cirrus/lib.sh Outdated
showrun install -v -D -m 644 $SCRIPT_BASE/99-do-not-use-google-subnets.conflist \
/etc/cni/net.d/
}

use_netavark() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is necessary any more

@edsantiago
Copy link
Collaborator

Root cause: f38 VM images do not include netavark.

Solution: containers/automation_images#325 in progress. Will take a few hours. I will report back.

edsantiago added a commit to edsantiago/libpod that referenced this pull request Jan 29, 2024
@edsantiago
Test new CI VMs
d12c62d 
From containers/automation_images#325

Major change: netavark and aardvark are now included in prior-fedora,
so CNI can be fully eliminated from CI (containers#21410)

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago
Copy link
Collaborator

c20240129t175101z-f39f38d13 is ready but you can't just swap them in without the rest of #21366

@ashley-cui
Copy link
Member Author

Thanks Ed! Not a huge deal, this can wait for that.

edsantiago added a commit to edsantiago/libpod that referenced this pull request Jan 30, 2024
@edsantiago
Test new CI VMs
9e04f0d 
From containers/automation_images#325

Major change: netavark and aardvark are now included in prior-fedora,
so CNI can be fully eliminated from CI (containers#21410)

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago
Copy link
Collaborator

Update: this PR is unlikely to merge this week. I'm sorry. Please feel free to follow along

@ashley-cui
Copy link
Member Author

No worries!

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2024
edsantiago added a commit to edsantiago/libpod that referenced this pull request Feb 1, 2024
@edsantiago
Test new CI VMs
1ab55de 
From containers/automation_images#325

Major change: netavark and aardvark are now included in prior-fedora,
so CNI can be fully eliminated from CI (containers#21410)

FIXME FIXME FIXME: skip two e2e tests, waiting for new netavark

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago mentioned this pull request Feb 1, 2024
Merged
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2024
@ashley-cui
Copy link
Member Author

ashley-cui commented Feb 1, 2024 •
edited

Thanks @edsantiago for the new VMs, lets see how this goes :)

@ashley-cui
Copy link
Member Author

Looks like everything except upgrade is passing, upgrade seems to be because v4.0.0 isn't in quay. Setting that it to v4.1.0 and re-running and seeing how that goes.

@edsantiago
Copy link
Collaborator

How frustrating. v4.0.0 doesn't exist, but v4.1.0 does. I see that you tested it. What was the failure with v4.1.0?

@ashley-cui
Copy link
Member Author

ashley-cui commented Feb 2, 2024 •
edited

@edsantiago Looks like it the network tests aren't passing, which I kinda expected. If you have any hunches off the top of your head, let me know, otherwise I'll dig a little deeper.

@edsantiago
Copy link
Collaborator

Just disable the upgrade tests. I have a TODO item to revamp those, I will make it a priority on Monday.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2024
@ashley-cui
Vendor c/common
ee92335 
@main

Signed-off-by: Ashley Cui <acui@redhat.com>
@ashley-cui ashley-cui changed the title [WIP] Do not test CNI in CI Do not test CNI in CI Feb 5, 2024
@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 5, 2024
@ashley-cui ashley-cui marked this pull request as ready for review February 5, 2024 14:51
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 5, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2024
@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@ashley-cui
Do not test CNI in CI
26cd01e 
CNI is deprecated and is build tagged out for 5.0. Don't test it in our CI.
This commit also disables upgrade tests for now - those need more work since the old version of Podman only uses CNI. Upgrade tests will be re-vamped in a later commit.
Signed-off-by: Ashley Cui <acui@redhat.com>
Luap99
Luap99 approved these changes Feb 5, 2024
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM , I would love to get upgrade tests back but for now I care more about unblocking the common vendor.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui, Luap99

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

@ashley-cui
Copy link
Member Author

Passing tests, ready for review/merge @containers/podman-maintainers

@mheon
Copy link
Member

mheon commented Feb 5, 2024

LGTM

@edsantiago
Copy link
Collaborator

LGTM. I'm working on upgrade tests, the fixes for boltdb/sqlite are much harder than I expected. I intend to submit my PR on or by Wednesday.

@Luap99
Copy link
Member

Luap99 commented Feb 6, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 6, 2024
@Luap99 Luap99 added breaking-change A change that will require a full version bump, i.e. 3.* to 4.* 5.0 labels Feb 6, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit a2f0a44 into containers:main Feb 6, 2024
88 of 89 checks passed
This was referenced Feb 6, 2024
Merged
Merged
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 7, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@edsantiago edsantiago edsantiago left review comments

@Luap99 Luap99 Luap99 approved these changes

Assignees

@Luap99 Luap99

Labels
5.0 approved Indicates a PR has been approved by an approver from all required OWNERS files. breaking-change A change that will require a full version bump, i.e. 3.* to 4.* lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ashley-cui @edsantiago @mheon @Luap99 @openshift-merge-robot