Skip to content

use headless service for upload server#4052

Merged
kubevirt-bot merged 1 commit into
kubevirt:mainfrom
akalenyu:headless-upload-svc
Mar 16, 2026
Merged

use headless service for upload server#4052
kubevirt-bot merged 1 commit into
kubevirt:mainfrom
akalenyu:headless-upload-svc

Conversation

@akalenyu
Copy link
Copy Markdown
Collaborator

@akalenyu akalenyu commented Mar 9, 2026

What this PR does / why we need it:
apparently, there's no k8s API that says
"kube-proxy has synced iptables rules for this service on all nodes"
so, pod readiness/healthz (even endpointslices) could all report success,
but the client would still face a connection refused
(this is causing unnecessary restarts in the k8s 1.35 bump PR)

we faced something similar in virtctl too
#3545

arguably, a headless service was the right fit for us anyway
and cuts the above overhead out of the equation.

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 #3851

Special notes for your reviewer:
Not sure if there's a backwards compatibility consideration in here,
since upload pods are not usually carried over an upgrade

Release note:

Enhancement: use headless service for upload server

apparently there's no k8s API that says
"kube-proxy has synced iptables rules for this service on all nodes"
so pod readiness/healthz (even endpointslices) could all report
success but the client would still face a connection refused.

we faced something similar in virtctl
kubevirt#3545

arguably, a headless service was the right fit for us anyway
and cuts the above overhead out of the equation.

Signed-off-by: Alex Kalenyuk <[email protected]>
@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 Mar 9, 2026
@akalenyu
Copy link
Copy Markdown
Collaborator Author

akalenyu commented Mar 9, 2026

/cc @Acedus @mhenriks
would especially appreciate thoughts on

Special notes for your reviewer:
Not sure if there's a backwards compatibility consideration in here,
since upload pods are not usually carried over an upgrade

@kubevirt-bot kubevirt-bot requested review from Acedus and mhenriks March 9, 2026 16:45
@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 49.29% (-0.01%) from 49.3%
when pulling b9f9073 on akalenyu:headless-upload-svc
into 2acfe66 on kubevirt:main.

IntVal: 8443,
},
Protocol: corev1.ProtocolTCP,
Port: 8443,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we change the exposed port to 8443 instead of 443?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is a no op for headless, there will be no remapping

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right, no port translation... I wonder if some users rely on this somehow (although I doubt it due to the proxy).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yeah definitely hyrums law territory, thought about this too. if anyone did, they can still use their own services

@Acedus
Copy link
Copy Markdown
Contributor

Acedus commented Mar 9, 2026

/cc @Acedus @mhenriks would especially appreciate thoughts on

Special notes for your reviewer:
Not sure if there's a backwards compatibility consideration in here,
since upload pods are not usually carried over an upgrade

I'd argue that it shouldn't be problematic since the upload-server Pod is transient and bound to the lifetime of an upload.

Copy link
Copy Markdown
Contributor

@Acedus Acedus left a comment

Choose a reason for hiding this comment

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

I think this is good, thanks!

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2026
@akalenyu
Copy link
Copy Markdown
Collaborator Author

/cc @0xFelix
Hopefully this is the time where we can finally make the virtctl image upload tests parallel lol

@kubevirt-bot kubevirt-bot requested a review from 0xFelix March 10, 2026 16:09
@0xFelix
Copy link
Copy Markdown
Member

0xFelix commented Mar 12, 2026

@akalenyu Let's hope so! 😁

@mhenriks
Copy link
Copy Markdown
Member

Fine with me. Getting rid of the cluster IP is fine for this case but endpointslices still have to be created and that may not happen right away either

@akalenyu
Copy link
Copy Markdown
Collaborator Author

Fine with me. Getting rid of the cluster IP is fine for this case but endpointslices still have to be created and that may not happen right away either

Yeah good point. I haven't seen that play out but I agree it's completely possible.
I think this PR stands regardless of that though

@mhenriks
Copy link
Copy Markdown
Member

/approve

@kubevirt-bot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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 Mar 16, 2026
@kubevirt-bot kubevirt-bot merged commit 2924a9b into kubevirt:main Mar 16, 2026
21 checks passed
akalenyu added a commit to akalenyu/kubevirt that referenced this pull request Apr 12, 2026
latest CDI release includes kubevirt/containerized-data-importer#4052
which finally allows us to trust the service/pod readiness
(more info in the linked PR)

Signed-off-by: Alex Kalenyuk <[email protected]>
@akalenyu
Copy link
Copy Markdown
Collaborator Author

/cherrypick release-v1.64 release-v1.63 release-v1.62 release-v1.61

@kubevirt-bot
Copy link
Copy Markdown
Contributor

@akalenyu: new pull request created: #4122

Details

In response to this:

/cherrypick release-v1.64 release-v1.63 release-v1.62 release-v1.61

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.

RamLavi added a commit to RamLavi/containerized-data-importer that referenced this pull request May 12, 2026
Since the upload service became headless (kubevirt#4052), DNS resolves directly
to the upload server pod IP. In OVN-Kubernetes clusters where the target
namespace uses network segmentation, the pod's default network interface
is locked down. The k8s.ovn.org/open-default-ports annotation opens port
8443 on the default interface, allowing the upload proxy to connect.

The annotation is harmless on non-OVN clusters where it is simply
ignored.

Assisted-by: Claude Opus 4.6 <[email protected]>
Signed-off-by: Ram Lavi <[email protected]>
RamLavi added a commit to RamLavi/containerized-data-importer that referenced this pull request May 12, 2026
Since the upload service became headless (kubevirt#4052), DNS resolves directly
to the upload server pod IP. In OVN-Kubernetes clusters where the target
namespace uses network segmentation, the pod's default network interface
is locked down. The k8s.ovn.org/open-default-ports annotation opens port
8443 on the default interface, allowing the upload proxy to connect.

The annotation is harmless on non-OVN clusters where it is simply
ignored.

Assisted-by: Claude Opus 4.6 <[email protected]>
Signed-off-by: Ram Lavi <[email protected]>
lyarwood pushed a commit to lyarwood/kubevirt that referenced this pull request May 12, 2026
latest CDI release includes kubevirt/containerized-data-importer#4052
which finally allows us to trust the service/pod readiness
(more info in the linked PR)

Signed-off-by: Alex Kalenyuk <[email protected]>
RamLavi added a commit to RamLavi/containerized-data-importer that referenced this pull request May 12, 2026
Since the upload service became headless (kubevirt#4052), DNS resolves directly
to the upload server pod IP. In OVN-Kubernetes clusters where the target
namespace uses network segmentation, the pod's default network interface
is locked down. The k8s.ovn.org/open-default-ports annotation opens port
8443 on the default interface, allowing the upload proxy to connect.

The annotation is harmless on non-OVN clusters where it is simply
ignored.

Assisted-by: Claude Opus 4.6 <[email protected]>
Signed-off-by: Ram Lavi <[email protected]>
@RamLavi
Copy link
Copy Markdown
Contributor

RamLavi commented May 12, 2026

/cherry-pick release-v1.64

@akalenyu FYI following our conversation

@kubevirt-bot
Copy link
Copy Markdown
Contributor

@RamLavi: new pull request could not be created: failed to create pull request against kubevirt/containerized-data-importer#release-v1.64 from head kubevirt-bot:cherry-pick-4052-to-release-v1.64: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for kubevirt-bot:cherry-pick-4052-to-release-v1.64."}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#create-a-pull-request","status":"422"}

Details

In response to this:

/cherry-pick release-v1.64

@akalenyu FYI following our conversation

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.

@RamLavi
Copy link
Copy Markdown
Contributor

RamLavi commented May 12, 2026

/cherry-pick release-v1.64

@akalenyu FYI following our conversation

oh sorry was not aware you already cherry-picked. Thanks!!

kubevirt-bot pushed a commit that referenced this pull request May 14, 2026
* Extract upload server port into a common constant

Replace hardcoded 8443 references in the upload controller with
common.UploadServerPort to prevent silent breakage if the port
ever changes.

Assisted-by: Claude Opus 4.6 <[email protected]>
Signed-off-by: Ram Lavi <[email protected]>

* Add open-default-ports annotation to upload server pod

Since the upload service became headless (#4052), DNS resolves directly
to the upload server pod IP. In OVN-Kubernetes clusters where the target
namespace uses network segmentation, the pod's default network interface
is locked down. The k8s.ovn.org/open-default-ports annotation opens port
8443 on the default interface, allowing the upload proxy to connect.

The annotation is harmless on non-OVN clusters where it is simply
ignored.

Assisted-by: Claude Opus 4.6 <[email protected]>
Signed-off-by: Ram Lavi <[email protected]>

---------

Signed-off-by: Ram Lavi <[email protected]>
kubevirt-bot added a commit that referenced this pull request May 14, 2026
#4135)

* Extract upload server port into a common constant

Replace hardcoded 8443 references in the upload controller with
common.UploadServerPort to prevent silent breakage if the port
ever changes.

Assisted-by: Claude Opus 4.6 <[email protected]>
Signed-off-by: Ram Lavi <[email protected]>

* Add open-default-ports annotation to upload server pod

Since the upload service became headless (#4052), DNS resolves directly
to the upload server pod IP. In OVN-Kubernetes clusters where the target
namespace uses network segmentation, the pod's default network interface
is locked down. The k8s.ovn.org/open-default-ports annotation opens port
8443 on the default interface, allowing the upload proxy to connect.

The annotation is harmless on non-OVN clusters where it is simply
ignored.

Assisted-by: Claude Opus 4.6 <[email protected]>
Signed-off-by: Ram Lavi <[email protected]>

---------

Signed-off-by: Ram Lavi <[email protected]>
Co-authored-by: Ram Lavi <[email protected]>
kubevirt-bot added a commit that referenced this pull request May 17, 2026
#4136)

* Extract upload server port into a common constant

Replace hardcoded 8443 references in the upload controller with
common.UploadServerPort to prevent silent breakage if the port
ever changes.

Assisted-by: Claude Opus 4.6 <[email protected]>
Signed-off-by: Ram Lavi <[email protected]>

* Add open-default-ports annotation to upload server pod

Since the upload service became headless (#4052), DNS resolves directly
to the upload server pod IP. In OVN-Kubernetes clusters where the target
namespace uses network segmentation, the pod's default network interface
is locked down. The k8s.ovn.org/open-default-ports annotation opens port
8443 on the default interface, allowing the upload proxy to connect.

The annotation is harmless on non-OVN clusters where it is simply
ignored.

Assisted-by: Claude Opus 4.6 <[email protected]>
Signed-off-by: Ram Lavi <[email protected]>

---------

Signed-off-by: Ram Lavi <[email protected]>
Co-authored-by: Ram Lavi <[email protected]>
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/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable Headless Service for CDI UploadProxy to Improve Image Upload Performance

7 participants