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

forklift: introduce forklift controller #2983

Merged
merged 27 commits into from
May 14, 2024

Conversation

bennyz
Copy link
Contributor

@bennyz bennyz commented Nov 13, 2023

What this PR does / why we need it:
A controller to run the forklift populators, it will handle the population for the OvirtVolumePopulator and OpenstackVolumePopulator kinds

Example DVs:

apiVersion: cdi.kubevirt.io/v1beta1
kind: DataVolume
metadata:
  name: my-populator-dv
  annotations:
spec:
  storage:
    dataSourceRef:
        apiGroup: forklift.konveyor.io
        kind: OvirtVolumePopulator
        name: test
    resources:
      requests:
        storage: 1G
    accessMode:
      - ReadWriteOnce
    storageClassName: nfs-csi
apiVersion: forklift.konveyor.io/v1beta1
kind: OvirtVolumePopulator
metadata:
  name: test
  namespace: default
spec:
  engineUrl: https://ovirt.fqdn
  engineSecretName: ovirt-secret
  diskId: 134db723-7ba2-47c4-b19a-975383ff1491

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
depends on #2947
Release note:

Introduce a controller to handle forklift's volume populators (ovirt, openstack)

TODO:

  • Make sure all annotations are properly set
  • Handle pod cleanup
  • Complete tests

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 13, 2023
@kubevirt-bot
Copy link
Contributor

Hi @bennyz. Thanks for your PR.

I'm waiting for a kubevirt 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.

@kubevirt-bot kubevirt-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL labels Nov 13, 2023
@bennyz bennyz changed the title [WIP] forklift: add forklift populators to cdi-importer [WIP] forklift: introduce forklift controller Nov 13, 2023
@bennyz bennyz force-pushed the forklift-controller branch 2 times, most recently from cfbaa09 to b349944 Compare November 16, 2023 14:58
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2023
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2023
@bennyz bennyz force-pushed the forklift-controller branch 2 times, most recently from c0397fe to 692a25f Compare November 27, 2023 13:00
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2023
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 24, 2024
@bennyz bennyz force-pushed the forklift-controller branch 2 times, most recently from b478523 to f4e7b96 Compare January 29, 2024 15:01
@bennyz bennyz force-pushed the forklift-controller branch 2 times, most recently from 509fadc to 00150db Compare February 4, 2024 13:20
@bennyz
Copy link
Contributor Author

bennyz commented Feb 4, 2024

@akalenyu could you have a preliminary look?

@akalenyu
Copy link
Collaborator

akalenyu commented Feb 4, 2024

@akalenyu could you have a preliminary look?

So this is fine for review, regardless of the ovirt populator still not being in?

@bennyz
Copy link
Contributor Author

bennyz commented Feb 4, 2024

@akalenyu could you have a preliminary look?

So this is fine for review, regardless of the ovirt populator still not being in?

This fetches the ovirt populator from our repo, as this is seems the direction we'll be going for, and even if not, switching back to the original approach will not be difficult and won't affect the core controller code too much, just the boilerplate code around

@@ -61,6 +61,9 @@ QUAY_REPOSITORY=${QUAY_REPOSITORY:-cdi-operatorhub}
QUAY_NAMESPACE=${QUAY_NAMESPACE:-kubevirt}
CDI_LOGO_PATH=${CDI_LOGO_PATH:-"assets/cdi_logo.png"}

# oVirt populator image from forklift
OVIRT_POPULATOR_IMAGE_NAME=${OVIRT_POPULATOR_IMAGE_NAME:-"quay.io/kubev2v/ovirt-populator:release-v2.5.4"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

cc @alromeros @mhenriks
my worry is productizing this in downstream

Copy link
Collaborator

Choose a reason for hiding this comment

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

WDYT about including & using the sources under CDI? at least in our scripts
On other distributions (downstream) the env var will be set to something else and that will get tested instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not implemented yet, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not yet, just to clarify, we want to build it in CDI?
something like was done here: #2895 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, any drawbacks? lets us stay completely in charge upstream if anything breaks down

Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure why you linked the closed PR, the one that merged also had the ovirt populator sources in it:
#2947

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the closed one ovirt-populator is built separately which is what I assume we want here, while in #2947 it's part of cdi-importer

Copy link
Collaborator

@akalenyu akalenyu Apr 1, 2024

Choose a reason for hiding this comment

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

ah, I see. maybe for consistency let's keep it non separate (part of importer)? wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

published, only did a basic sanity check but looks ok

bennyz and others added 13 commits May 13, 2024 10:35
- remove accidental copy-paste
- change type of NAD reference to string
- use NAD in the controller

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
… images with tls

Possibly a bug, there were some segfault fixes in 1.25.4 release:
https://nginx.org/en/CHANGES

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
- Fix const
- Reverse conditional
- Pass context to reconcile
- Improve cross namespace check
- Add populator pod watcher

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
- Drop unnecessary comments
- Add dataSourceRef check
- Add node to spec

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
- fix race condition leaving Running phase in annotation
- fix metrics UT

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
- Change optional fields to a pointer
- Fix Pod watch
- Add NotFound check to avoid an extra reconile

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
@awels
Copy link
Member

awels commented May 13, 2024

/test all

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2024
@awels
Copy link
Member

awels commented May 13, 2024

/test pull-cdi-goveralls

@awels
Copy link
Member

awels commented May 13, 2024

/test pull-cdi-generate-verify

@coveralls
Copy link

Coverage Status

coverage: 60.687% (-0.8%) from 61.503%
when pulling 84dc967 on bennyz:forklift-controller
into 6f7809e on kubevirt:main.

@awels
Copy link
Member

awels commented May 13, 2024

/test pull-containerized-data-importer-fossa

@awels
Copy link
Member

awels commented May 13, 2024

/test pull-containerized-data-importer-e2e-nfs

@awels
Copy link
Member

awels commented May 13, 2024

/test pull-containerized-data-importer-e2e-ceph

@awels
Copy link
Member

awels commented May 14, 2024

/retest-required

@kubevirt-bot kubevirt-bot merged commit 46f0f76 into kubevirt:main May 14, 2024
19 checks passed
@akalenyu
Copy link
Collaborator

/cherrypick release-v1.59

@kubevirt-bot
Copy link
Contributor

@akalenyu: #2983 failed to apply on top of branch "release-v1.59":

Applying: forklift: add types for forklift populators
Applying: forklift: introduce forklift controller
Applying: forklift: add CRD missing CRD suffix
Applying: forklift: fix tests
Applying: forklift: start support for mutated PVC
Applying: forklift: fix linter issues
Applying: forklift: update vendor and generated code
Applying: forklift: remove unnecessary argument
Applying: forklift: add ovirt-populator to cdi-importer
Using index info to reconstruct a base tree...
M	WORKSPACE
M	hack/build/rpm-deps.sh
Falling back to patching base and 3-way merge...
Auto-merging hack/build/rpm-deps.sh
Auto-merging WORKSPACE
CONFLICT (content): Merge conflict in WORKSPACE
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0009 forklift: add ovirt-populator to cdi-importer
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-v1.59

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-sigs/prow repository.

akalenyu added a commit to akalenyu/containerized-data-importer that referenced this pull request May 15, 2024
* forklift: add types for forklift populators

Introduce the OvirtVolumePopulator and OpenstackVolumePopulator types

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: introduce forklift controller

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: add CRD missing CRD suffix

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix tests

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: start support for mutated PVC

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix linter issues

and skip bound PVCs

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: update vendor and generated code

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: remove unnecessary argument

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: add ovirt-populator to cdi-importer

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix linter issues

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix contrller_test

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: regenerate swagger

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: normalize ovirt image name

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: restore OVIRT_POPULATOR_IMAGE_NAME

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: address comments

- remove accidental copy-paste
- change type of NAD reference to string
- use NAD in the controller

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: add doc with examples for the forklift populators

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: rename secret reference

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix linter warning

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: update deps

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix linter issues

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: use ginkgo for openstack populator test

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* Roll back nginx from 1.24.0 to 1.22.1 to avoid segfaults when pulling…
… images with tls

Possibly a bug, there were some segfault fixes in 1.25.4 release:
https://nginx.org/en/CHANGES

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* forklift: address comments

- Fix const
- Reverse conditional
- Pass context to reconcile
- Improve cross namespace check
- Add populator pod watcher

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: address comments

- Drop unnecessary comments
- Add dataSourceRef check
- Add node to spec

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: add restartCount annotation

- fix race condition leaving Running phase in annotation
- fix metrics UT

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: address comments

- Change optional fields to a pointer
- Fix Pod watch
- Add NotFound check to avoid an extra reconile

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: regenerate

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

---------

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
Co-authored-by: Alex Kalenyuk <akalenyu@redhat.com>
kubevirt-bot added a commit that referenced this pull request May 15, 2024
* Run bazelisk run //robots/cmd/uploader:uploader -- -workspace /home/prow/go/src/github.com/kubevirt/project-infra/../containerized-data-importer/WORKSPACE -dry-run=false (#3247)

Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com>

* Improve usability of Golangci-lint (#3193)

* Run 'make format && make generate'

Seems like this hadn't been run in a long while.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Run 'golangci-lint --fix' when using 'make format'

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Update golangci-lint version

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Remove gofmt

Golangci-lint already runs gofmt internally (see ./.golangci.yml)

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Remove golint pass

This linter has been deprecated since May 9, 2021. Golangci-lint already
covers its functionality

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Remove goimports

Golangci-lint already runs goimports internally (see ./.golangci.yml)

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Move govet into golangci-lint

This way we don't parse the project twice.

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Update README

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* Run formaters and generators again

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

---------

Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>

* forklift: introduce forklift controller (#2983)

* forklift: add types for forklift populators

Introduce the OvirtVolumePopulator and OpenstackVolumePopulator types

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: introduce forklift controller

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: add CRD missing CRD suffix

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix tests

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: start support for mutated PVC

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix linter issues

and skip bound PVCs

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: update vendor and generated code

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: remove unnecessary argument

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: add ovirt-populator to cdi-importer

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix linter issues

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix contrller_test

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: regenerate swagger

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: normalize ovirt image name

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: restore OVIRT_POPULATOR_IMAGE_NAME

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: address comments

- remove accidental copy-paste
- change type of NAD reference to string
- use NAD in the controller

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: add doc with examples for the forklift populators

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: rename secret reference

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix linter warning

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: update deps

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: fix linter issues

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: use ginkgo for openstack populator test

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* Roll back nginx from 1.24.0 to 1.22.1 to avoid segfaults when pulling…
… images with tls

Possibly a bug, there were some segfault fixes in 1.25.4 release:
https://nginx.org/en/CHANGES

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

* forklift: address comments

- Fix const
- Reverse conditional
- Pass context to reconcile
- Improve cross namespace check
- Add populator pod watcher

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: address comments

- Drop unnecessary comments
- Add dataSourceRef check
- Add node to spec

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: add restartCount annotation

- fix race condition leaving Running phase in annotation
- fix metrics UT

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: address comments

- Change optional fields to a pointer
- Fix Pod watch
- Add NotFound check to avoid an extra reconile

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

* forklift: regenerate

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>

---------

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
Co-authored-by: Alex Kalenyuk <akalenyu@redhat.com>

* Run bazelisk run //robots/cmd/uploader:uploader -- -workspace /home/prow/go/src/github.com/kubevirt/project-infra/../containerized-data-importer/WORKSPACE -dry-run=false

Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com>

---------

Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com>
Signed-off-by: Edu Gómez Escandell <egomez@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
Co-authored-by: kubevirt-bot <kubevirtbot@redhat.com>
Co-authored-by: Edu Gómez Escandell <egomez@redhat.com>
Co-authored-by: Benny Zlotnik <2139890+bennyz@users.noreply.github.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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. 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

6 participants