Fix initializations and terminations of http servers #13723

davidhadas · 2023-02-21T11:12:21Z

Fixes #13720

Bug fixes for server initialization and termination.

Adds timer for waiting for services to shutdown
Fixes drainer for TLS
Remove admin service when tlsAdmin is used
Shutdown all servers before we flush and exit

Signed-off-by: David Hadas <[email protected]>

codecov · 2023-02-21T12:05:50Z

Codecov Report

Patch coverage has no change and project coverage change: -0.09 ⚠️

Comparison is base (8d8c39e) 86.10% compared to head (8ef6bac) 86.02%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13723      +/-   ##
==========================================
- Coverage   86.10%   86.02%   -0.09%     
==========================================
  Files         199      199              
  Lines       14796    14815      +19     
==========================================
+ Hits        12740    12744       +4     
- Misses       1752     1768      +16     
+ Partials      304      303       -1

Impacted Files	Coverage Δ
pkg/queue/sharedmain/main.go	`0.87% <0.00%> (+0.02%)`	⬆️
pkg/queue/sharedmain/servers.go	`0.00% <0.00%> (ø)`

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

davidhadas · 2023-02-21T12:47:43Z

Tests fail. Is this due to admin being removed when tlsAdmin was used? Do we require both?

Signed-off-by: David Hadas <[email protected]>

davidhadas · 2023-02-21T20:20:25Z

@nak3, any thoughts on why the tests are failing following g the change?

pkg/queue/sharedmain/main.go

skonto · 2023-02-23T10:18:56Z

pkg/queue/sharedmain/main.go

- for name, server := range httpServers {
- go func(name string, s *http.Server) {
+ for _, s := range srvs {
+ go func(s service) {


Should we use errGroup to fail here if any of the server fails?

errGroup waits for all to terminate (and present the first error). We seem to not have a reason to use it here.
We will still need the errChan to signal when any one of them had an error.

During shutdown, we use synchronous Shutdowns (one by one).
in both cases seems we do not need to wait for all to terminate asynchronously

skonto · 2023-02-23T10:25:24Z

stream.go:305: E 18:03:48.425 activator-544b695d96-85k9p [activator] [serving-tests/should-have-stdin-e-o-f-wrakflxj-00001] Failed to probe clusterIP 10.96.112.83:80 err=error roundtripping [https://10.96.112.83:80/healthz:](https://10.96.112.83/healthz:) dial tcp 10.96.112.83:80: connect: connection refused

pkg/queue/sharedmain/main.go

davidhadas · 2023-02-23T14:34:12Z

What do we need adminTls for?

Looking into Drainer, my understanding is that the admin's sole purpose is to serve the "/wait-for-drain" sent as a preStop hook. This afaik will always be HTTP and never TLS. Can we drop the adminTls then and always use HTTP based admin?

Another way to put this is that admin is always an HTTP service - never a TLS one.

…ers, moved to struct service being returned from build* functions Signed-off-by: David Hadas <[email protected]>

Signed-off-by: David Hadas <[email protected]>

davidhadas · 2023-02-23T15:42:21Z

/assign @davidhadas

davidhadas · 2023-02-23T16:17:20Z

Moved to a single admin service (no mainTls needed) - the admin now drain both the drainer from main and the one from mainTls (if not nil).

build* functions are now returning a service struct

We now have a simpler logic where we just add services as needed to our array depending on the different flags.

Signed-off-by: David Hadas <[email protected]>

davidhadas · 2023-02-23T17:44:58Z

/retest

davidhadas · 2023-02-23T17:56:18Z

/retest

davidhadas · 2023-02-23T18:36:54Z

/test istio-latest-no-mesh

davidhadas · 2023-03-16T13:02:26Z

/retest

davidhadas · 2023-03-24T18:32:50Z

/hold
@dprotaso is planning to refactor the main and mainTls handlers - moving from 2 separate handlers to one.
Once this refactor is done, we can suffice with a single drainer.
We will then continue working on this PR to solve #13720 - it will naturally simplify this PR as there will be only a single drainer.

dprotaso · 2023-03-28T15:30:14Z

The handler refactor merged - #13815

You should be able to rebase your changes. There might be some overlap - one that's still unique to this PR is introduce a deadline for shutdown.

knative-prow · 2023-03-29T04:53:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidhadas, nak3, ReToCode

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~~ [nak3]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow · 2023-03-29T04:53:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidhadas, nak3, ReToCode

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~~ [nak3]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

davidhadas · 2023-03-29T05:47:43Z

@dprotaso rebased

dprotaso

I still think that it's much easier to reason which servers support TLS by having it declarative vs. changing the builders and having an opaque list.

I'm prioritizing readibility over repitition in this instance.

Can we just scope this PR to introducing a shutdown time limit?

davidhadas · 2023-03-29T15:27:40Z

@dprotaso
This PR came to fix a different issue and not the shutdown time limit...
It is here to fix a bug in the termination that resulted in connections being successfully created even after flush().
See #13720 (comment),

Was this bug fixed and tested by #13815 ?

If it did, then we can do without this PR.
If it was not, then some PR is still needed to fix it.
This PR offers to solve this bug by ensuring that we have an orderly list of open services and ensuring that we carefully shutdown this list - this has been tested to fix the bug.

dprotaso · 2023-03-29T15:38:50Z

Was this bug fixed and tested by #13815 ?

Yup - main should be shutting down properly now

The fix was here
https://github.com/knative/serving/pull/13815/files#diff-c89ab95fa1d2a52207142b5664c30bfce82bc2abea59bf620d248dd43f30bd56L309-L310

This line should have been removed in an earlier refactor.

davidhadas · 2023-03-29T17:16:10Z

Closing this PR as #13720 (comment) was solved

Fix initializations and terminations of http servers

cb1ad3b

Signed-off-by: David Hadas <[email protected]>

knative-prow bot added the area/autoscale label Feb 21, 2023

knative-prow bot requested review from izabelacg and skonto February 21, 2023 11:12

knative-prow bot added area/networking size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 21, 2023

davidhadas added 4 commits February 21, 2023 16:31

Adding admin even when tlsAdmin is added

f68fa70

Signed-off-by: David Hadas <[email protected]>

Try out with shutdown to see if e2e kurier-tls pass

f7b1cb1

Signed-off-by: David Hadas <[email protected]>

Seperate Drainers trying to get kurier tls e2e to pass

2b03c1b

Signed-off-by: David Hadas <[email protected]>

Trying to wait longer at Shutdown

7534c4e

Signed-off-by: David Hadas <[email protected]>

skonto reviewed Feb 23, 2023

View reviewed changes

pkg/queue/sharedmain/main.go Show resolved Hide resolved

skonto reviewed Feb 23, 2023

View reviewed changes

pkg/queue/sharedmain/main.go Outdated Show resolved Hide resolved

skonto reviewed Feb 23, 2023

View reviewed changes

pkg/queue/sharedmain/main.go Outdated Show resolved Hide resolved

added main in tls mode, unified admin and adminTls, unified the drain…

3db3349

…ers, moved to struct service being returned from build* functions Signed-off-by: David Hadas <[email protected]>

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 23, 2023

nits

9a25aea

Signed-off-by: David Hadas <[email protected]>

knative-prow bot assigned davidhadas Feb 23, 2023

nits

2ec4a90

Signed-off-by: David Hadas <[email protected]>

davidhadas requested review from dprotaso and removed request for psschwei March 16, 2023 12:14

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2023

davidhadas closed this Mar 29, 2023

davidhadas force-pushed the queueServiceInitAndShutdown branch from 9f070f6 to 8d8c39e Compare March 29, 2023 03:52

knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 29, 2023

rebase

8ef6bac

davidhadas reopened this Mar 29, 2023

knative-prow-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 29, 2023

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 29, 2023

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 29, 2023

dprotaso requested changes Mar 29, 2023

View reviewed changes

davidhadas closed this Mar 29, 2023

dprotaso mentioned this pull request Mar 29, 2023

Http and tls services are not initialized and terminated correctly #13720

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix initializations and terminations of http servers #13723

Fix initializations and terminations of http servers #13723

davidhadas commented Feb 21, 2023

codecov bot commented Feb 21, 2023 •

edited

Loading

davidhadas commented Feb 21, 2023

davidhadas commented Feb 21, 2023

skonto Feb 23, 2023

davidhadas Feb 23, 2023

skonto commented Feb 23, 2023

davidhadas commented Feb 23, 2023 •

edited

Loading

davidhadas commented Feb 23, 2023

davidhadas commented Feb 23, 2023

davidhadas commented Feb 23, 2023

davidhadas commented Feb 23, 2023

davidhadas commented Feb 23, 2023

davidhadas commented Mar 16, 2023

davidhadas commented Mar 24, 2023 •

edited

Loading

dprotaso commented Mar 28, 2023

knative-prow bot commented Mar 29, 2023

knative-prow bot commented Mar 29, 2023

davidhadas commented Mar 29, 2023

dprotaso left a comment

davidhadas commented Mar 29, 2023 •

edited

Loading

dprotaso commented Mar 29, 2023

davidhadas commented Mar 29, 2023

Fix initializations and terminations of http servers #13723

Fix initializations and terminations of http servers #13723

Conversation

davidhadas commented Feb 21, 2023

codecov bot commented Feb 21, 2023 • edited Loading

Codecov Report

davidhadas commented Feb 21, 2023

davidhadas commented Feb 21, 2023

skonto Feb 23, 2023

Choose a reason for hiding this comment

davidhadas Feb 23, 2023

Choose a reason for hiding this comment

skonto commented Feb 23, 2023

davidhadas commented Feb 23, 2023 • edited Loading

davidhadas commented Feb 23, 2023

davidhadas commented Feb 23, 2023

davidhadas commented Feb 23, 2023

davidhadas commented Feb 23, 2023

davidhadas commented Feb 23, 2023

davidhadas commented Mar 16, 2023

davidhadas commented Mar 24, 2023 • edited Loading

dprotaso commented Mar 28, 2023

knative-prow bot commented Mar 29, 2023

knative-prow bot commented Mar 29, 2023

davidhadas commented Mar 29, 2023

dprotaso left a comment

Choose a reason for hiding this comment

davidhadas commented Mar 29, 2023 • edited Loading

dprotaso commented Mar 29, 2023

davidhadas commented Mar 29, 2023

codecov bot commented Feb 21, 2023 •

edited

Loading

davidhadas commented Feb 23, 2023 •

edited

Loading

davidhadas commented Mar 24, 2023 •

edited

Loading

davidhadas commented Mar 29, 2023 •

edited

Loading