search

kubevirt / containerized-data-importer

Data Import Service for kubernetes, designed with kubevirt in mind.
Apache License 2.0
428 stars 269 forks source link

cdi-uploadserver: healthcheck and application use same port #3536

Closed mhenriks closed 4 days ago

mhenriks commented 5 days ago

healthcheck used to be on a dedicated port for "reasons" and by that I mean I was too lazy to properly set up the tls configuration and check authz in upload handlers

What this PR does / why we need it:

This one is for @akalenyu and @0xFelix: I know I told you this wasn't possible but here you go.

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

Special notes for your reviewer:

Release note:

BugFix: cdi-uploadserver reporting readiness more honestly resulting in concurrent image uploads failing less
coveralls commented 5 days ago

Coverage Status

coverage: 59.291% (-0.04%) from 59.326% when pulling 9c74cd3c57ed747371cd21abfcbe3b175123ebea on mhenriks:uploadserver-health into ed0b77938155c1049d0bec03da6786f8aaef325e on kubevirt:main.

akalenyu commented 4 days ago

/test pull-containerized-data-importer-e2e-ceph tests didn't even run

mhenriks commented 4 days ago

This is great! maybe rework the release note a bit and attach the related issue? #3450

How can the release note be better? Feel free to edit yourself

akalenyu commented 4 days ago

This is great! maybe rework the release note a bit and attach the related issue? #3450

How can the release note be better? Feel free to edit yourself

made the change. wdyt

akalenyu commented 4 days ago

/approve

kubevirt-bot commented 4 days ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akalenyu

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: - ~~[OWNERS](https://github.com/kubevirt/containerized-data-importer/blob/main/OWNERS)~~ [akalenyu] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
0xFelix commented 8 hours ago

@mhenriks @akalenyu Does this mean we can drop the retry from virtctl imageupload again? Does this also mean we can make the functests for it run in parallel again?

mhenriks commented 3 hours ago

@mhenriks @akalenyu Does this mean we can drop the retry from virtctl imageupload again? Does this also mean we can make the functests for it run in parallel again?

@0xFelix I think this change will have minimal to no effect on that issue but you are welcome to try. There is a big delta (much bigger than can be accounted by this change) between when a pod says it is ready and k8s creates endpoints for a service and iptables are updated/etc. And of course that all takes longer when the system is under load (more functests in parallel). cdi-uploadproxy does not take any of that into account just pod readiness