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

Use centos:stream9 as a base image, cleanup unused code #1983

Merged
merged 16 commits into from Apr 20, 2022

Conversation

maya-r
Copy link
Contributor

@maya-r maya-r commented Oct 11, 2021

What this PR does / why we need it:
KubeVirt has done similar with centos stream8.
Fedora has some downsides: frequent releases, and shorter end of life.
CentOS stream will mean fewer base image changes, and it means we use a base image which has vulnerabilities reported to it, as requested on kubevirt-dev.

Special notes for your reviewer:
Blocked on #1982 which changes the builder image.
Biggest change was switching away from what might be a self-made Fedora RPM for golang-govmomi to downloading the releases and untarring with the entrypoint.sh file.
VDI test now uses a pre-generated tinyCore.vdi as the qemu-img vdi support is read-only.

Release note:

Use centos:stream9 as a base image for our containers.

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XXL labels Oct 11, 2021
@brybacki
Copy link
Contributor

You say "Removes VDI importer code"
Does it mean we lose part of functionality? Is it used by some migration tools or end users?

@maya-r
Copy link
Contributor Author

maya-r commented Oct 12, 2021

Looking again, in the original PR by @visheshtanksale (#1512) he added VDI support, so it might be used.
I wonder if I can request to change the RPM in centos.

@maya-r
Copy link
Contributor Author

maya-r commented Oct 12, 2021

@visheshtanksale
Copy link
Contributor

Looking again, in the original PR by @visheshtanksale (#1512) he added VDI support, so it might be used.
I wonder if I can request to change the RPM in centos.

Yes some of our key functionality will be lost if we merge this without support for VDI.

@maya-r
Copy link
Contributor Author

maya-r commented Oct 13, 2021

/hold
waiting for VDI support in centos stream packages

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2021
@jsquyres
Copy link
Contributor

Per https://groups.google.com/g/kubevirt-dev/c/Vu89kytp2mQ, I'm interested in having both Kubevirt and CDI move away from Fedora. Is there a time estimate on when Centos 8 stream will gain VDI support so that this PR can progress?

@jsquyres
Copy link
Contributor

Gentle ping. I see in #1982, you cited https://bugzilla.redhat.com/show_bug.cgi?id=2013331. It doesn't look like there has been any activity on that bug yet. Do you have any kind of estimate on if/when that will be acted on, which would unblock both #1982 and this PR?

@maya-r
Copy link
Contributor Author

maya-r commented Oct 26, 2021

@jsquyres No idea about timelines. The bug report requesting VDI support seems to get a positive response but unfortunately all the replies are set to private.

@jsquyres
Copy link
Contributor

@maya-r That's somewhat of a bummer. Is it a sensitive topic?

I notice that the issue is currently marked as RHEL9.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2021
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2021
@maya-r maya-r changed the title Use centos:stream8 as a base image, cleanup unused code Use centos:stream9 as a base image, cleanup unused code Dec 5, 2021
"@nbdkit-server-aarch64//file",
"@nbdkit-basic-filters-aarch64//file",
#"@nbdkit-vddk-plugin-aarch64//file",
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need the vddk plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plugin doesn't seem to be built for aarch64 - I'm not sure why that is the case.
(I didn't notice I'm the one introducing this change, I thought it's an existing one)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Worth noting that vmware aarch64 support is in tech preview and not really likely to be in heavy use

@maya-r
Copy link
Contributor Author

maya-r commented Dec 7, 2021

/retest

@maya-r
Copy link
Contributor Author

maya-r commented Dec 7, 2021

looks like the push builder job is a periodic that hasn't run yet

@maya-r
Copy link
Contributor Author

maya-r commented Dec 8, 2021

Oops, I'm going to need kubevirt/project-infra#1796 to update the builder.

@maya-r
Copy link
Contributor Author

maya-r commented Dec 15, 2021

Blocked as the builder push job is having connectivity issues. Daniel Hiller tried re-running it but it failed again.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2022
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2022
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 21, 2022
@jsquyres
Copy link
Contributor

jsquyres commented Mar 3, 2022

Is there any progress on this PR, perchance (and #2087)? It would be really, really great to ditch Fedora and switch to Centos stream...

They expire faster than we can update checksums, this is unfortunate
but perhaps they will soon publish images at a lower rate allowing
us to keep up.

Signed-off-by: Maya Rashish <mrashish@redhat.com>
Signed-off-by: Maya Rashish <mrashish@redhat.com>
Signed-off-by: Maya Rashish <mrashish@redhat.com>
Signed-off-by: Maya Rashish <mrashish@redhat.com>
Signed-off-by: Maya Rashish <mrashish@redhat.com>
Noticed due to: duplicated checksum but no problem in testsuite,
lack of aarch64 equivalent.

Signed-off-by: Maya Rashish <mrashish@redhat.com>
Signed-off-by: Maya Rashish <mrashish@redhat.com>
Signed-off-by: Maya Rashish <mrashish@redhat.com>
Needed after kubevirt#2174

Signed-off-by: Maya Rashish <mrashish@redhat.com>
The previous version we were using can't be fetched any more

Signed-off-by: Maya Rashish <mrashish@redhat.com>
Now updating the dependencies can be done by running `make rpm-deps`
and committing the change, like kubevirt.

This creates a small complication that we need to run update-ca-trust
to trust root CAs. Do this on the pod, using the entrypoint to do so.

Use a single image with all the dependencies for the test tools, we
don't benefit from making them minimal and it saved some trouble in
the conversion.

Signed-off-by: Maya Rashish <mrashish@redhat.com>
@awels
Copy link
Member

awels commented Apr 12, 2022

Looks like all the image io tests are failing on something.

Run update-ca-trust and update-crypto-policies before running
ovirt-imageio, to stop error messages.

Signed-off-by: Maya Rashish <mrashish@redhat.com>
@maya-r
Copy link
Contributor Author

maya-r commented Apr 14, 2022

/retest-required

@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Apr 14, 2022

@maya-r: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-containerized-data-importer-e2e-k8s-1.19-hpp 4eca286 link true /test pull-containerized-data-importer-e2e-k8s-1.19-hpp
pull-containerized-data-importer-e2e-k8s-1.20-hpp-destructive 4eca286 link true /test pull-containerized-data-importer-e2e-k8s-1.20-hpp-destructive
pull-containerized-data-importer-e2e-k8s-1.20-hpp-istio 4eca286 link true /test pull-containerized-data-importer-e2e-k8s-1.20-hpp-istio
pull-containerized-data-importer-e2e-k8s-1.20-upg 4eca286 link true /test pull-containerized-data-importer-e2e-k8s-1.20-upg
pull-containerized-data-importer-e2e-k8s-1.20-hpp 4eca286 link true /test pull-containerized-data-importer-e2e-k8s-1.20-hpp
pull-containerized-data-importer-e2e-k8s-1.20-nfs 4eca286 link true /test pull-containerized-data-importer-e2e-k8s-1.20-nfs
pull-containerized-data-importer-e2e-k8s-1.19-ceph 4eca286 link true /test pull-containerized-data-importer-e2e-k8s-1.19-ceph

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. I understand the commands that are listed here.

@mhenriks
Copy link
Member

/retest

@maya-r
Copy link
Contributor Author

maya-r commented Apr 18, 2022

all green @mhenriks

Copy link
Member

@awels awels left a comment

Choose a reason for hiding this comment

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

Looks good just a question about the arm base image not being set in a few places

"@io_bazel_rules_go//go/platform:linux_arm64": "@fedora-aarch64//image",
"//conditions:default": "@fedora//image",
}),
base = "//:centos_base",
Copy link
Member

Choose a reason for hiding this comment

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

Don't we also need the arm version here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

centos_base is a different image if the platform is arm64.
the cross build works, I pushed the binaries and when inspecting one by hand it seems to be made up of arm64 binaries and executables.

Copy link
Member

Choose a reason for hiding this comment

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

okay

"@io_bazel_rules_go//go/platform:linux_arm64": "@fedora-aarch64//image",
"//conditions:default": "@fedora//image",
}),
base = "//:centos_base",
Copy link
Member

Choose a reason for hiding this comment

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

Same we need arm version here.

"@io_bazel_rules_go//go/platform:linux_arm64": "@fedora-aarch64//image",
"//conditions:default": "@fedora//image",
}),
base = "//:centos_base",
Copy link
Member

Choose a reason for hiding this comment

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

arm version?

@awels
Copy link
Member

awels commented Apr 20, 2022

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2022
@awels
Copy link
Member

awels commented Apr 20, 2022

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2022
@kubevirt-bot kubevirt-bot merged commit 02e70a5 into kubevirt:main Apr 20, 2022
maya-r added a commit to maya-r/containerized-data-importer that referenced this pull request May 14, 2022
Forgotten in kubevirt#1983, needed to run `make rpm-deps`

Signed-off-by: Maya Rashish <mrashish@redhat.com>
maya-r added a commit to maya-r/containerized-data-importer that referenced this pull request May 14, 2022
Forgotten in kubevirt#1983, needed to run `make rpm-deps`

Signed-off-by: Maya Rashish <mrashish@redhat.com>
kubevirt-bot pushed a commit that referenced this pull request May 15, 2022
Forgotten in #1983, needed to run `make rpm-deps`

Signed-off-by: Maya Rashish <mrashish@redhat.com>
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. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants