-
Notifications
You must be signed in to change notification settings - Fork 258
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
Change clone strategy and cron sources format for LVMS #3196
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Following some investigation we are now confident this tuning is benefical for LVMS: - CSI clone is effectively implemented the same as snapshotting because both already use lvm2 thinly provisioned snapshots under the hood - Snapshots being COW, it makes total sense to store cron sources in the snapshot format instead of PVC: ``` A snapshot of a thin logical volume also creates a thin logical volume. This consumes no data space until a COW operation is required, or until the snapshot itself is written. ``` Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
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/XS
labels
Apr 16, 2024
/lgtm |
[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 |
kubevirt-bot
added
the
approved
Indicates a PR has been approved by an approver from all required OWNERS files.
label
Apr 16, 2024
akalenyu
added a commit
to akalenyu/containerized-data-importer
that referenced
this pull request
May 1, 2024
Following some investigation we are now confident this tuning is benefical for LVMS: - CSI clone is effectively implemented the same as snapshotting because both already use lvm2 thinly provisioned snapshots under the hood - Snapshots being COW, it makes total sense to store cron sources in the snapshot format instead of PVC: ``` A snapshot of a thin logical volume also creates a thin logical volume. This consumes no data space until a COW operation is required, or until the snapshot itself is written. ``` Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
kubevirt-bot
added a commit
that referenced
this pull request
May 3, 2024
…intenance (#3227) * Double number of e2e parallel nodes from 3 to 6 (#3156) We have some feedback from different platform testing that this can work tests (not resource) wise. Let's give it a try here as well Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Support IPv6 in controller GetMetricsURL (#3165) Joining hostname+port with string concatenation leads to wrong URLs when the hostame is an IPv6: HOST PORT Sprintf CORRECT ::1 8000 ::1:8000 [::1]:8000 To avoid this issue, it's best to use net.JoinHostPort. I added a test that verifies this fix. This type of issue can be discovered running the following command: golangci-lint run ./... --enable nosprintfhostport Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Enable misspell linter and fix spelling errors (#3164) * Add misspell to list of linters Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Fix spelling errors Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> --------- Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Remove openshift dep on api (#3172) Less deps the better! Could be problematic for projects importing CDI and openshift. Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * Enable unconvert linter (#3171) * Enable unconvert linter This linter's doc describes it as: The unconvert program analyzes Go packages to identify unnecessary type conversions; i.e., expressions T(x) where x already has type T. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Unrestrict the number of linter warnings It is best to show all warnings at once than to reveal them piece-meal, particularly in CI where the feedback loop can be a bit slow. By default, linters may only print the same message three times (https://golangci-lint.run/usage/configuration/#issues-configuration) The unconvert linter always prints the same message, so it specially affected by this setting. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Remove redundant type conversions Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> --------- Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Enable linters checking error handling (#3177) * Enable errorlint linter Per the readme: > go-errorlint is a source code linter for Go software that can be used to find code that will cause problems with the error wrapping scheme introduced in Go 1.13. https://github.com/polyfloyd/go-errorlint Essentially, it checks that you properly use errors.Is and errors.As It also forces you to use %w, but that is a bit too much. Using %v is fine when you don't want to leak the internal error types of the package. This is why I disabled this setting. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Replace error comparisons with errors.Is Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Replace error type assertions with errors.As Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Enable errname linter Per the readme: > Checks that sentinel errors are prefixed with the Err and error > types are suffixed with the Error. https://github.com/Antonboom/errname Not a big deal, but helps keep the naming consistent. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Fix errname linter warnings Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> --------- Signed-off-by: Edu Gómez Escandell <egomez@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 (#3199) Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com> * CVE 2024-24786 fix: Bump google.golang.org/protobuf from 1.31.0 to 1.33.0 (#3195) * Upgrade google.golang.org/protobuf This solves CVE-2024-24786 https://www.cve.org/CVERecord?id=CVE-2024-24786 Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Update checksum and vendoring Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> --------- Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Enable durationcheck linter (#3176) * Enable durationcheck linter > Check for two durations multiplied together. https://github.com/charithe/durationcheck Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Fix misuse of time.Duration I also had to rename the variable because go-statickcheck complains about a time.Duration having the suffix 'Sec'. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> --------- Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Enable nakedret linter (#3178) * Enable nakedret linter > Checks that functions with naked returns are not longer than a maximum > size (can be zero). https://github.com/alexkohler/nakedret Relevant from the GO style guide: > Naked returns are okay if the function is a handful of lines. Once > it’s a medium sized function, be explicit with your return values. https://go.dev/wiki/CodeReviewComments#named-result-parameters I think a naked return is never easier to read than a regular return, so I set the linter to always complain about it. Disagreements are welcome. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Refactor functions with naked returns Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> --------- Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Clean up leftover snapshot sources from DataImportCron tests (#3123) * Clean up leftover snapshot sources from DataImportCron tests The dataimportcron tests may affect existing crons in the environment (in addition to the ones deployed by testing) so we are better off cleaning up after ourselves. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Add watch for volumesnapshot delete Although we don't support seamless transition from volumesnapshot->pvc sources (we hope you stay on snapshots if they scale better for your storage) this will still be needed in most cases when someone switches and manually deletes snapshots. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --------- Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Enable autoformatting linters (#3179) * Enable gofmt linter From the docs: > Gofmt checks whether code was gofmt-ed. By default this tool runs with > -s option to check for code simplification. https://golangci-lint.run/usage/linters/#gofmt Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Run gomft on the project Ran this command after adding the gofmt linter: golangci-lint run ./... --fix Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Enable whitespace linter From the docs: > Whitespace is a linter that checks for unnecessary newlines at the > start and end of functions, if, for, etc. https://golangci-lint.run/usage/linters/#whitespace Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Run whitespace on the project Ran this command after adding the whitespace linter: golangci-lint run ./... --fix Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Enable GCI linter Per the docs: > Gci controls Go package import order and makes it always deterministic. https://golangci-lint.run/usage/linters/#gci NOTE: I noticed that many files separate their imports in a particular way, so I set the linter to enforce this standard. Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Run GCI on the project Ran this command after adding the GCI linter: golangci-lint run ./... --fix Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> --------- Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Enable dupword linter (#3175) * Enable dupword linter Per the readme: > A linter that checks for duplicate words in the source code (usually miswritten) https://github.com/Abirdcfly/dupword https://golangci-lint.run/usage/linters/#dupword Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Remove duplicate words in comments and strings Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> --------- Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> * Change clone strategy and cron sources format for LVMS (#3196) Following some investigation we are now confident this tuning is benefical for LVMS: - CSI clone is effectively implemented the same as snapshotting because both already use lvm2 thinly provisioned snapshots under the hood - Snapshots being COW, it makes total sense to store cron sources in the snapshot format instead of PVC: ``` A snapshot of a thin logical volume also creates a thin logical volume. This consumes no data space until a COW operation is required, or until the snapshot itself is written. ``` Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Set default clone strategy & cron format for trident to snapshot (#3209) There's a bug in the trident CSI driver that causes a snapshot to be left over following each CSI clone: NetApp/trident#901 This is becoming a problem rapidly once one hits the snapshot limit per volume (golden image): https://kb.netapp.com/onprem/ontap/os/Maximum_number_of_snapshots_supported_by_ONTAP Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --------- Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> Signed-off-by: Edu Gómez Escandell <egomez@redhat.com> Signed-off-by: Michael Henriksen <mhenriks@redhat.com> Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com> Co-authored-by: Edu Gómez Escandell <edu1997xyz@gmail.com> Co-authored-by: Edu Gómez Escandell <egomez@redhat.com> Co-authored-by: Michael Henriksen <mhenriks@redhat.com> Co-authored-by: kubevirt-bot <kubevirtbot@redhat.com>
akalenyu
added a commit
to akalenyu/containerized-data-importer
that referenced
this pull request
Aug 4, 2024
Recently we've had a back and forth about the appropriate source format for LVMS: kubevirt#3196 kubevirt#3303 Which presented the need to support a seamless transition between snapshot->PVC sources which was never implemented in hopes that one would not go through such a transition. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
kubevirt-bot
pushed a commit
that referenced
this pull request
Aug 7, 2024
Recently we've had a back and forth about the appropriate source format for LVMS: #3196 #3303 Which presented the need to support a seamless transition between snapshot->PVC sources which was never implemented in hopes that one would not go through such a transition. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
kubevirt-bot
pushed a commit
to kubevirt-bot/containerized-data-importer
that referenced
this pull request
Aug 7, 2024
Recently we've had a back and forth about the appropriate source format for LVMS: kubevirt#3196 kubevirt#3303 Which presented the need to support a seamless transition between snapshot->PVC sources which was never implemented in hopes that one would not go through such a transition. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
kubevirt-bot
pushed a commit
to kubevirt-bot/containerized-data-importer
that referenced
this pull request
Aug 7, 2024
Recently we've had a back and forth about the appropriate source format for LVMS: kubevirt#3196 kubevirt#3303 Which presented the need to support a seamless transition between snapshot->PVC sources which was never implemented in hopes that one would not go through such a transition. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
kubevirt-bot
added a commit
that referenced
this pull request
Aug 7, 2024
Recently we've had a back and forth about the appropriate source format for LVMS: #3196 #3303 Which presented the need to support a seamless transition between snapshot->PVC sources which was never implemented in hopes that one would not go through such a transition. 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
Aug 8, 2024
Recently we've had a back and forth about the appropriate source format for LVMS: #3196 #3303 Which presented the need to support a seamless transition between snapshot->PVC sources which was never implemented in hopes that one would not go through such a transition. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> Co-authored-by: Alex Kalenyuk <akalenyu@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/XS
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Following some investigation we are now confident this tuning is benefical for LVMS:
CSI clone is effectively implemented the same as snapshotting because both already use lvm2 thinly provisioned snapshots under the hood
Snapshots being COW, it makes total sense to store cron sources in the snapshot format instead of PVC:
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/6/html/logical_volume_manager_administration/thinly-provisioned_snapshot_volumes
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:
Release note: