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.

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

Headless Logs III: Upload and retrieve headless logs after the workspaces has stopped #4439

Merged
merged 3 commits into from
Jul 15, 2021

Conversation

geropl
Copy link
Member

@geropl geropl commented Jun 8, 2021

Test

Run prebuild and download log

ToDo

@geropl geropl changed the title [content-service] Introduce logs [content-service] Headless Logs III: Offline logs Jun 8, 2021
@geropl geropl changed the title [content-service] Headless Logs III: Offline logs Headless Logs III: Upload and retrieve headless logs after the workspaces has stopped Jun 9, 2021
@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #4439 (1a04547) into main (974ce0a) will increase coverage by 35.96%.
The diff coverage is 9.41%.

❗ Current head 1a04547 differs from pull request most recent head 823952c. Consider uploading reports for the commit 823952c to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##           main    #4439       +/-   ##
=========================================
+ Coverage      0   35.96%   +35.96%     
=========================================
  Files         0       78       +78     
  Lines         0    15481    +15481     
=========================================
+ Hits          0     5568     +5568     
- Misses        0     9429     +9429     
- Partials      0      484      +484     
Flag Coverage Δ
components-content-service-app 14.35% <11.02%> (?)
components-content-service-lib 14.35% <11.02%> (?)
components-ee-ws-scheduler-app 62.84% <ø> (?)
components-image-builder-api-go-lib ∅ <ø> (?)
components-image-builder-app 34.44% <ø> (?)
components-supervisor-app 36.37% <66.66%> (?)
components-workspacekit-app ∅ <ø> (?)
components-ws-daemon-api-go-lib ∅ <ø> (?)
components-ws-daemon-app 22.29% <0.00%> (?)
components-ws-daemon-nsinsider-app ∅ <ø> (?)
components-ws-manager-api-go-lib ∅ <ø> (?)
components-ws-manager-app 36.63% <0.00%> (?)
components-ws-proxy-app 65.84% <ø> (?)
dev-loadgen-app ∅ <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/content-service/pkg/storage/gcloud.go 3.11% <0.00%> (ø)
components/content-service/pkg/storage/minio.go 0.00% <0.00%> (ø)
components/content-service/pkg/storage/noop.go 0.00% <0.00%> (ø)
components/content-service/pkg/storage/storage.go 7.69% <0.00%> (ø)
components/ws-daemon/pkg/content/initializer.go 0.00% <0.00%> (ø)
components/ws-daemon/pkg/content/service.go 0.00% <0.00%> (ø)
components/ws-manager/pkg/manager/monitor.go 0.00% <0.00%> (ø)
...ontent-service/pkg/service/headless-log-service.go 24.13% <24.13%> (ø)
components/supervisor/pkg/supervisor/tasks.go 47.98% <66.66%> (ø)
components/ws-manager/pkg/manager/metrics.go 11.26% <0.00%> (ø)
... and 77 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 974ce0a...823952c. Read the comment docs.

@geropl geropl force-pushed the gpl/headless-log-content branch 5 times, most recently from 23a6b88 to e1c780f Compare July 2, 2021 12:48
@geropl
Copy link
Member Author

geropl commented Jul 5, 2021

/werft run

👍 started the job as gitpod-build-gpl-headless-log-content.20

@geropl geropl marked this pull request as ready for review July 6, 2021 08:53
@geropl geropl requested a review from a team as a code owner July 6, 2021 08:53
@geropl geropl requested review from a team, JanKoehnlein and aledbf and removed request for a team July 6, 2021 08:53
Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

This is in excellent shape! I have yet to try things out, and would much prefer more tests, but we're really rather close here :)

components/content-service/pkg/logs/logs.go Outdated Show resolved Hide resolved
components/server/src/workspace/headless-log-service.ts Outdated Show resolved Hide resolved
components/content-service-api/headless-log.proto Outdated Show resolved Hide resolved
)

// HeadlessLogService implements LogServiceServer
type HeadlessLogService struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

unit tests for the HeadlessLogService would be awfully nice - e.g. by mocking PresignedAccess

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test for ListLogs. Not super convinced that the test alone justifies the LOC of the mock... but maybe useful for the other services as well.

Comment on lines +22 to +23
legacyTerminalStoreLocation = "/workspace"
legacyPrebuildLogFilePrefix = ".prebuild-log-"
Copy link
Contributor

Choose a reason for hiding this comment

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

We've made the switch from this "legacy" naming convention to what we use today about nine months ago - I reckon we could get rid of it.

Not necessarily as part of this PR, only if it makes you life/this code easier.

@geropl geropl requested a review from csweichel July 13, 2021 15:11
@geropl
Copy link
Member Author

geropl commented Jul 14, 2021

Not 100% this PR, but closely related:
@svenefftinge Discovered yesterday that the current implementation still needs quite some time (e.g., 1 db-sync cycle after the pod got created) to be able to connect to a running prebuild because we need to know the ideURL to derive the supervisor URL from it.

We could try to improve a bit by storing the URL from the StartWorkspaceResponse we already get directly into the DB (here) but still depend on a sync cycle.

Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

Thank you for sticking with this PR for so long. Let's get this in

@geropl geropl merged commit bb5f229 into main Jul 15, 2021
@geropl geropl deleted the gpl/headless-log-content branch July 15, 2021 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants