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 to F36 CI VM Images + Testing netavark/aardvark-dns #13376

Merged
merged 6 commits into from
Apr 27, 2022

Conversation

cevich
Copy link
Member

@cevich cevich commented Feb 28, 2022

Update to F36 CI VM Images + Testing netavark/aardvark-dns on F36+

Depends on:

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2022
@cevich cevich changed the title [WIP] [WIP] Update to F36 CI VM Images + Testing netavark/aardvark-dns on F36+ Feb 28, 2022
@cevich cevich changed the title [WIP] Update to F36 CI VM Images + Testing netavark/aardvark-dns on F36+ [WIP] Update to F36 CI VM Images + Testing netavark/aardvark-dns Feb 28, 2022
@cevich
Copy link
Member Author

cevich commented Mar 1, 2022

@rhatdan @mheon @vrothberg The F36 VM is failing in validate. Is this a golang-1.18 thing? Is there a compatibility-ish[1] flag or some way to disable the eleven-billion gofmt changes?

************************************************************
Runner executing validate podman-tests as root on fedora-36(fedora-36)
Current environment VM image: fedora-c4789105149083648
************************************************************
find . -name '*.go' -type f \
	-not \( \
		-name '.golangci.yml' -o \
		-name 'Makefile' -o \
		-path './vendor/*' -prune -o \
		-path './contrib/*' -prune \
	\) -exec gofmt -d -e -s -w {} \+
diff -u ./cmd/podman/early_init_unsupported.go.orig ./cmd/podman/early_init_unsupported.go
--- ./cmd/podman/early_init_unsupported.go.orig	2022-02-28 14:13:04.931221238 -0600
+++ ./cmd/podman/early_init_unsupported.go	2022-02-28 14:13:04.931221238 -0600
@@ -1,3 +1,4 @@
+//go:build !linux
 // +build !linux
 package main
...cut 10.9 billion more...

[1] Note: At a higher-level, this command is coming from the Makefile, so we need a solution/fix that's also supported for developers in other environments (down to golang 1.16 IIUC).

@vrothberg
Copy link
Member

It's a Go thing, yes. In order for it to pass, we need to apply all the changes suggested by the linter.

@cevich
Copy link
Member Author

cevich commented Mar 1, 2022

apply all the changes suggested by the linter

I'm fine with that. Can you confirm that won't impact folks running validate on older golangs we support?

@vrothberg
Copy link
Member

I'm fine with that. Can you confirm that won't impact folks running validate on older golangs we support?

Yes, we're already using that in c/image and probably more parts of the stack.

@cevich
Copy link
Member Author

cevich commented Mar 1, 2022

Great, thanks. I'll add a dedicated commit for it.

@cevich cevich force-pushed the f36_update branch 2 times, most recently from ee40752 to 3f8e16c Compare March 1, 2022 20:37
@cevich
Copy link
Member Author

cevich commented Mar 1, 2022

@vrothberg have you seen these golang 1.18 problems WRT golangci-lint at all (some are posted upstream)? If I use 1.36.0 (same as on main) I get these failures. If I update to 1.44.2 (the latest), I get these failures 😕

@cevich
Copy link
Member Author

cevich commented Mar 1, 2022

Update: I tried 1.42.1 (manually), it doesn't throw errors per say, but reports several screen-fulls of lint violations before ERRO Timeout exceeded... 😖

@cevich
Copy link
Member Author

cevich commented Mar 3, 2022

foirce-push: Temporarily bypass validate requirement for other tasks to run

@cevich
Copy link
Member Author

cevich commented Mar 3, 2022

Well int podman fedora-36 root host passed, so that's good. int podman fedora-36 root container is a complete mess (log). Hey @mheon @baude, do I need to ensure $NETWORK_BACKEND=netavark inside the container? I think I might be missing that.

All the F35 failures make me worried - it's currently setup for runc + cni. Is it time we drop testing of runc in Fedora entirely, and just rely on Ubutnu to catch any issues?

@rhatdan
Copy link
Member

rhatdan commented Mar 3, 2022

I am fine with dropping testing for runc on Fedora as long as we are testing it somewhere.

@vrothberg
Copy link
Member

@cevich, I fear the only way forward is to disable make lint on F36 until Go 1.18 has been released and is supported by the linter tool chain.

I feel comfortable skipping it in F36 since we're still running the checks on F35.

@cevich
Copy link
Member Author

cevich commented Mar 4, 2022

I feel comfortable skipping it in F36 since we're still running the checks on F35.

I can temporarily switch the Validate task to use the F35 image, then we can just revert that commit later. Presumably the Fedora people think 1.18 will release before F36 releases?

@cevich
Copy link
Member Author

cevich commented Mar 4, 2022

Force-push: Added Cirrus: Temporarily validate in F35

@cevich cevich force-pushed the f36_update branch 2 times, most recently from 342f937 to 92ccebf Compare March 10, 2022 16:55
@cevich
Copy link
Member Author

cevich commented Mar 10, 2022

Force-push: Enable $NETWORK_BACKEND=netavar in containerized e2e and rootless

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2022
@openshift-ci openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 16, 2022
@TomSweeneyRedHat
Copy link
Member

One question that could be addressed later. Otherwise LGTM. @edsantiago if you could take a deep dive, I'd appreciate it.

cevich added 6 commits April 27, 2022 12:13
This reverts commit 7b55ab4.

Signed-off-by: Chris Evich <[email protected]>
Now that netavark and aardvark are packaged and default in F36, support
CNI-based testing in F35 and Ubuntu.

* Remove the temporary/special `$TEST_ENVIRON=host-netavark` construct.
* Remove dedicated/special integration and system testing tasks.
* Update test-config setup to properly handle CNI vs netavark/aardvark
  environments.
* Update package-version logging to operate based on installed packages
  (along with some other minor script cleanups).
* Update global environment setup to force `$NETWORK_BACKEND=netavark`
  in F36 and later.  Except when `upgrade_test` task runs.
* Discontinue installing netavark and aardvark-dns binaries from
  upstream build artifacts.
* Drop CGV1-vs-2 policy check.  Ubuntu VMs now exclusively test CGv1,
  Fedora VMs test CGv2, with F35 testing CNI and F36 testing Netavark.

Signed-off-by: Chris Evich <[email protected]>
Normally installing/updating packages at test runtime is highly
discouraged for reliability and efficiency reasons.  However, in this
specific case, development work of these packages is still fairly hot.
As a compromise to support podman test development, temporarily update
these two specific packages at runtime.  At a future date, when updates
are less frequent, this commit can/should be safely reverted.  At that
point, the versions installed at VM image build time will persist.

Signed-off-by: Chris Evich <[email protected]>
Newer versions of git are much more pedantic about who owns the
repository files.  When setting up to run rootless, prior to this
commit, the repo. ownership was changed from root.  This causes
all subsequent git-operations as root to fail:

    ```
    fatal: unsafe repository ('<$GOSRC>' is owned by someone else)
    ```

Fix this by re-ordering operations, such that the change in ownership is
done immediately before executing as a user.  Also disable the
git-ownership check on the source repository assuming the CI environment
is disposable.

Signed-off-by: Chris Evich <[email protected]>
@cevich
Copy link
Member Author

cevich commented Apr 27, 2022

force-push: Rebased, and tweaked two comments.

@edsantiago
Copy link
Member

@cevich is this ready for review?

@cevich
Copy link
Member Author

cevich commented Apr 27, 2022

@cevich is this ready for review?

Yes please.

Edit: If you think it will make reviewing easier, I can probably split off the Cirrus: Fix ownership of repos. to keep git happy commit into a separate PR. The other 5 commits need to stay here though.

# TODO: Remove this when netavark/aardvark-dns development slows down
warn "Updating netavark/aardvark-dns to avoid frequent VM image rebuilds"
# N/B: This is coming from updates-testing repo in F36
lilto dnf update -y netavark aardvark-dns
Copy link
Member

Choose a reason for hiding this comment

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

Should these be installed from updates-testing?

Copy link
Member

Choose a reason for hiding this comment

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

The comment suggests that they are, but the string updates.test does not appear anywhere else (relevantly) in this repo, nor in containers-automation, nor in containers-image, so I have this question too

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

So updates-testing repo is enabled.

@rhatdan
Copy link
Member

rhatdan commented Apr 27, 2022

LGTM with one question
@baude @edsantiago PTAL

@rhatdan
Copy link
Member

rhatdan commented Apr 27, 2022

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 27, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, rhatdan

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 27, 2022
@edsantiago
Copy link
Member

I have quibbles, but nothing that warrants yet another re-push on this two-month saga. Thanks for your hard work on this @cevich

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 27, 2022
@openshift-merge-robot openshift-merge-robot merged commit 578ffd7 into containers:main Apr 27, 2022
@rhatdan
Copy link
Member

rhatdan commented Apr 27, 2022

Should probably start working on f37 now. :^(

@edsantiago
Copy link
Member

New Cirrus task map now on wiki

@cevich
Copy link
Member Author

cevich commented Apr 28, 2022

Should probably start working on f37 now. :^(

Thanks for the help reviewing and fixing all the issues this raised along the way. This one was especially difficult due to the netavark/aardvark switchup. Though, now we can all sleep better at night, knowing the test-coverage of the new networking stack has been vastly expanded.

Next up is actually Ubuntu 22.04 😢

@cevich cevich deleted the f36_update branch April 18, 2023 14:45
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 31, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants