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

backendstorage: use fixed volume name #12261

Merged
merged 1 commit into from
Jul 3, 2024
Merged

backendstorage: use fixed volume name #12261

merged 1 commit into from
Jul 3, 2024
+1 −1

Conversation

fossedihelm
Copy link
Contributor

@fossedihelm fossedihelm commented Jul 2, 2024 •
edited
Loading

What this PR does

Currently, the backend storage state pod volume
name is generated from the vmi name.
Pod Volume names must respect RFC 1123 Label Names.
Using the vmi name is not correct since the latter must respect DNS Subdomain Names,
which is more relaxed compared with the first one.
This can cause an unschedulable pod in the following cases:

  • the vmi name contains dots(".")
  • the resulting volume name has a length > 63

Besides that, there is no reason to reference the vmi name inside the pod volume name,
since they must be unique just inside the container pod.

Let's just use a fixed name vm-state, as we already do, for example, for private-libvirt.

Before this PR:
Using tpm results in an unschedulable pod if the vmi name contains dots(".")

After this PR:
tpm can be used with vmis containing dots in their names.

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Release note

Fix: persistent tpm can be used with vmis containing dots in their name

@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. labels Jul 2, 2024
@fossedihelm
Copy link
Contributor Author

/cc @jean-edouard @mhenriks

@fossedihelm
Copy link
Contributor Author

/uncc @AlonaKaplan @0xFelix

@kubevirt-bot kubevirt-bot added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 2, 2024
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jean-edouard

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 Jul 2, 2024
@kubevirt-commenter-bot
Copy link

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator

@fossedihelm
backendstorage: use fixed volume name
c1f18a8 
Currently, the backend storage state pod volume
name is generated from the vmi name.
Pod Volume names must respect [RFC 1123 Label Names](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names).
Using the vmi name is not correct since the latter must respect [DNS Subdomain Names](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names),
which is more relaxed compared with the first one.
This can cause an unschedulable pod in the following cases:
- the vmi name contains dots(".")
- the resulting volume name has a length > 63

Besides that, there is no reason to reference the vmi name inside the pod volume name,
since they must be unique just inside the container pod.

Let's just use a fixed name `backendstorage-state`,
as we already do, for example, for `private-libvirt`.

Signed-off-by: fossedihelm <ffossemo@redhat.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2024
@jean-edouard
Copy link
Contributor

/unhold

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2024
@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2024
@kubevirt-commenter-bot
Copy link

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator

@fossedihelm
Copy link
Contributor Author

/cherrypick release-1.3
/cherrypick release-1.2
/cherrypick release-1.1
/cherrypick release-1.0

@kubevirt-bot
Copy link
Contributor

@fossedihelm: once the present PR merges, I will cherry-pick it on top of release-1.3 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.3
/cherrypick release-1.2
/cherrypick release-1.1
/cherrypick release-1.0

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.

@kubevirt-bot kubevirt-bot merged commit 31b146a into kubevirt:main Jul 3, 2024
41 checks passed
@kubevirt-bot kubevirt-bot mentioned this pull request Jul 3, 2024
Open
@kubevirt-bot
Copy link
Contributor

@fossedihelm: new pull request created: #12278

In response to this:

/cherrypick release-1.3
/cherrypick release-1.2
/cherrypick release-1.1
/cherrypick release-1.0

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.

@kubevirt-bot
Copy link
Contributor

@fossedihelm: #12261 failed to apply on top of branch "release-1.0":

Applying: backendstorage: use fixed volume name
Using index info to reconstruct a base tree...
M	pkg/virt-controller/services/rendervolumes.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-controller/services/rendervolumes.go
CONFLICT (content): Merge conflict in pkg/virt-controller/services/rendervolumes.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 backendstorage: use fixed volume name
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-1.3
/cherrypick release-1.2
/cherrypick release-1.1
/cherrypick release-1.0

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.

@kubevirt-bot
Copy link
Contributor

@fossedihelm: new pull request created: #12279

In response to this:

/cherrypick release-1.3
/cherrypick release-1.2
/cherrypick release-1.1
/cherrypick release-1.0

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.

@kubevirt-bot kubevirt-bot mentioned this pull request Jul 3, 2024
Open
@kubevirt-bot
Copy link
Contributor

@fossedihelm: new pull request created: #12280

In response to this:

/cherrypick release-1.3
/cherrypick release-1.2
/cherrypick release-1.1
/cherrypick release-1.0

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.

@kubevirt-bot kubevirt-bot mentioned this pull request Jul 3, 2024
Open
@fossedihelm fossedihelm mentioned this pull request Jul 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabiand fabiand fabiand left review comments

@xpivarc xpivarc xpivarc approved these changes

@jean-edouard jean-edouard Awaiting requested review from jean-edouard

@mhenriks mhenriks Awaiting requested review from mhenriks

Assignees

@xpivarc xpivarc

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@fossedihelm @kubevirt-bot @kubevirt-commenter-bot @jean-edouard @fabiand @xpivarc