Skip to content

Remove legacy session service#15155

Merged
xacrimon merged 8 commits intomasterfrom
joel/remove-legacy-sess
Aug 12, 2022
Merged

Remove legacy session service#15155
xacrimon merged 8 commits intomasterfrom
joel/remove-legacy-sess

Conversation

@xacrimon
Copy link
Copy Markdown
Contributor

@xacrimon xacrimon commented Aug 3, 2022

Paired with https://github.com/gravitational/teleport.e/pull/504

Removes the legacy session service since Moderated Sessions trackers are now used instead. All dependent usage on this service was phased out in v10 so I believe should be clear to do this.

@xacrimon xacrimon marked this pull request as ready for review August 3, 2022 14:38
@rosstimothy rosstimothy requested a review from espadolini August 3, 2022 15:43
Copy link
Copy Markdown
Contributor

@nklaassen nklaassen left a comment

Choose a reason for hiding this comment

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

Looks okay but TestIntegrations/SessionRecordingModes/StrictMode/BeforeStartFailure is failing

Comment thread lib/session/session.go
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Aug 4, 2022

Looks like audit on is flaky:

--- FAIL: TestIntegrations (52.44s)
    --- FAIL: TestIntegrations/AuditOn (52.43s)
        --- FAIL: TestIntegrations/AuditOn/recording_proxy (14.18s)
            integration_test.go:379: Skipping mismatching session ed8d74c8-eec4-459f-ae7f-1fbd11f21a08, expecting upload of 63fd3d91-7e47-4db5-abe3-67a6a4130dec.
            integration_test.go:379: Skipping mismatching session ed8d74c8-eec4-459f-ae7f-1fbd11f21a08, expecting upload of 63fd3d91-7e47-4db5-abe3-67a6a4130dec.
            integration_test.go:385: recording proxy: Timeout waiting for upload of session 63fd3d91-7e47-4db5-abe3-67a6a4130dec to complete to
        --- FAIL: TestIntegrations/AuditOn/recording_proxy,_sync_recording (14.15s)
            integration_test.go:379: Skipping mismatching session eae5b3a8-20c3-40d8-8c76-a0c0afa3955c, expecting upload of c89f61af-5399-4709-b126-ff3aa17ed65b.
            integration_test.go:385: recording proxy, sync recording: Timeout waiting for upload of session c89f61af-5399-4709-b126-ff3aa17ed65b to complete to
FAIL
FAIL	github.com/gravitational/teleport/integration	53.607s
FAIL

@xacrimon
Copy link
Copy Markdown
Contributor Author

xacrimon commented Aug 5, 2022

@nklaassen That was flaky, I took a look and it doesn't look like my code change is related to anything in that test.

@xacrimon
Copy link
Copy Markdown
Contributor Author

xacrimon commented Aug 5, 2022

Looks like audit on is flaky:

--- FAIL: TestIntegrations (52.44s)
    --- FAIL: TestIntegrations/AuditOn (52.43s)
        --- FAIL: TestIntegrations/AuditOn/recording_proxy (14.18s)
            integration_test.go:379: Skipping mismatching session ed8d74c8-eec4-459f-ae7f-1fbd11f21a08, expecting upload of 63fd3d91-7e47-4db5-abe3-67a6a4130dec.
            integration_test.go:379: Skipping mismatching session ed8d74c8-eec4-459f-ae7f-1fbd11f21a08, expecting upload of 63fd3d91-7e47-4db5-abe3-67a6a4130dec.
            integration_test.go:385: recording proxy: Timeout waiting for upload of session 63fd3d91-7e47-4db5-abe3-67a6a4130dec to complete to
        --- FAIL: TestIntegrations/AuditOn/recording_proxy,_sync_recording (14.15s)
            integration_test.go:379: Skipping mismatching session eae5b3a8-20c3-40d8-8c76-a0c0afa3955c, expecting upload of c89f61af-5399-4709-b126-ff3aa17ed65b.
            integration_test.go:385: recording proxy, sync recording: Timeout waiting for upload of session c89f61af-5399-4709-b126-ff3aa17ed65b to complete to
FAIL
FAIL	github.com/gravitational/teleport/integration	53.607s
FAIL

@zmb3 It indeed does, but it looks to be related to the upload completer being late and not any of the timing-related fetching code changed in this PR as it errors on a seperate timeout that is only started after the changes made here execute. So I'd deem this out of scope.

			// wait for the upload of the right session to complete
			timeoutC := time.After(10 * time.Second)
		loop:
			for {
				select {
				case event := <-teleport.UploadEventsC:
					if event.SessionID != tracker.GetSessionID() {
						t.Logf("Skipping mismatching session %v, expecting upload of %v.", event.SessionID, tracker.GetSessionID())
						continue
					}
					break loop
				case <-timeoutC:
					dumpGoroutineProfile()
					t.Fatalf("%s: Timeout waiting for upload of session %v to complete to %v",
						tt.comment, tracker.GetSessionID(), tt.auditSessionsURI)
				}
			}
			```

@xacrimon xacrimon requested a review from nklaassen August 5, 2022 14:18
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Aug 5, 2022

it looks to be related to the upload completer being late and not any of the timing-related fetching code changed in this PR as it errors on a seperate timeout that is only started after the changes made here execute. So I'd deem this out of scope.

Sure, but the upload completer is based on the session tracker system and this PR fully cuts us over to the session tracker system, so it seems related enough to fix here unless it's a major effort.

@xacrimon xacrimon force-pushed the joel/remove-legacy-sess branch from 6ca54fb to e34dc0e Compare August 9, 2022 14:29
@xacrimon
Copy link
Copy Markdown
Contributor Author

xacrimon commented Aug 9, 2022

@zmb3 Flake fixed.

@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Aug 10, 2022

These could be unrelated, but TestClientDisconnect (which was flaky back in 2021 but then fixed) is failing:

===================================================
OUTPUT github.com/gravitational/teleport/lib/srv/regular.TestClientDisconnect
===================================================
=== RUN   TestClientDisconnect
=== PAUSE TestClientDisconnect
=== CONT  TestClientDisconnect
=== CONT  TestClientDisconnect
    sshserver_test.go:225: 
        	Error Trace:	sshserver_test.go:225
        	            				sshserver_test.go:117
        	            				sshserver_test.go:1398
        	Error:      	Received unexpected error:
        	            	timeout waiting for announce to be sent
        	Test:       	TestClientDisconnect

Similar timeout for TestAdvertiseAddr:

===================================================
OUTPUT github.com/gravitational/teleport/lib/srv/regular.TestAdvertiseAddr
===================================================
=== RUN   TestAdvertiseAddr
=== PAUSE TestAdvertiseAddr
=== CONT  TestAdvertiseAddr
=== CONT  TestAdvertiseAddr
    sshserver_test.go:225: 
        	Error Trace:	sshserver_test.go:225
        	            				sshserver_test.go:111
        	            				sshserver_test.go:482
        	Error:      	Received unexpected error:
        	            	timeout waiting for announce to be sent
        	Test:       	TestAdvertiseAddr

@xacrimon
Copy link
Copy Markdown
Contributor Author

@zmb3 Those seem completely unrelated but we should probably fix them in another PR.

@xacrimon xacrimon force-pushed the joel/remove-legacy-sess branch from 8bb0d47 to a394858 Compare August 12, 2022 11:41
@github-actions github-actions Bot removed the request for review from LKozlowski August 12, 2022 11:41
@xacrimon xacrimon enabled auto-merge (squash) August 12, 2022 16:18
@xacrimon xacrimon merged commit f2dd758 into master Aug 12, 2022
@zmb3 zmb3 deleted the joel/remove-legacy-sess branch August 31, 2022 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants