-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 Log II: ws-manager state machine #4351
Conversation
5417007
to
5a2d63b
Compare
5a2d63b
to
2922d1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes by and large make sense to me.
Only the many fixture tests I did not understand. Many (most) of them do not seem to exercise prebuild specific paths.
components/ws-manager/pkg/manager/testdata/status_prebuildFail_CREATING00.json
Outdated
Show resolved
Hide resolved
components/ws-manager/pkg/manager/testdata/status_prebuildFail_PENDING00.golden
Outdated
Show resolved
Hide resolved
components/ws-manager/pkg/manager/testdata/status_prebuildFail_INITIALIZING00.golden
Outdated
Show resolved
Hide resolved
/werft run 👍 started the job as gitpod-build-gpl-headless-log-wsman.31 |
I first started the prebuild when ws-daemon wasn't ready yet, hence the build failed. Now I'm left in a permanent "prebuild is running" state.
Once ws-daemon was up and running things worked as expected. |
I really like this change. It finally does away with that fickle headless stop mechanism and replaces it with a much stronger signal! |
d94e366
to
d5d7481
Compare
7f32420
to
aceb552
Compare
Codecov Report
@@ Coverage Diff @@
## main #4351 +/- ##
==========================================
+ Coverage 36.38% 41.33% +4.95%
==========================================
Files 14 37 +23
Lines 3881 9288 +5407
==========================================
+ Hits 1412 3839 +2427
- Misses 2349 5145 +2796
- Partials 120 304 +184
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
f5afc4f
to
95e9442
Compare
Now we also have integration tests. ✔️ |
8ce0b5f
to
b0ca0f2
Compare
1d2ec37
to
1edf05c
Compare
@csweichel Finally rebased and deployed again. After that and fixing the nits the codecov/patch is red now 🙈 |
Code does look good now indeed. After running the integration test I had a look at the ws-manager logs and saw this:
Even though the functionality itself seems to be there (snapshot is created and attached to the workspace), these logs are far too loud and might be hiding other failure modes. Unfortunately these issues would need to be fixed before merging this. If we were not to merge this PR prior to deploying, would that break prebuilds? |
Hm, I'm not sure what to do about this. The PR has not touched that area, at all. The only thing I can think of is to switch the
No, there is no dependency here. Though we should double check on staging, of course. |
Ok, after looking at the code there is one distinct line where we try to be loud about things we deem urgent. I introduced a condition @aledbf Do you have time to review? |
/werft run 👍 started the job as gitpod-build-gpl-headless-log-wsman.64 |
844cc1e
to
811b412
Compare
bffd808
to
1f06391
Compare
/werft run 👍 started the job as gitpod-build-gpl-headless-log-wsman.71 |
…eir container stops
1f06391
to
1565bfd
Compare
/werft run 👍 started the job as gitpod-build-gpl-headless-log-wsman.74 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one took way too long to review/rework/approve - I am very sorry for that :/
/werft run 👍 started the job as gitpod-build-gpl-headless-log-wsman.75 |
/werft run 👍 started the job as gitpod-build-gpl-headless-log-wsman.76 |
Note: don't merge before #4262
This PR is part of the "Revamp Headless Logs" effort: https://www.notion.so/gitpod/Revamp-Headless-Logs-90d9d08d12e344a49bce442cad89200b#065b7b935c4d4d2d86dd8bd1aa3b840e
It changes the "done" signal for headless workspaces from "the last line of the log is
DONE
" to "the container has stopped".This contains:
STOPPING
now does not imply "container has already been stopped" but is enforced bymonitor.go
now0
return222
as exit codeTest
Unit tests are passing ✔️
These Integration tests are passing ✔️
Start a prebuild which succeeds
PREBUILD_PARAMS: 100_500ms_1m_success
d_b_prebuild
)Start a prebuild which fails
go to https://gpl-headless-log-wsman.staging.gitpod-dev.com/variables and enter variable
PREBUILD_PARAMS: 100_500ms_1m_fail
start a prebuild: https://gpl-headless-log-wsman.staging.gitpod-dev.com/#prebuild/https://github.com/gitpod-io/gitpod/tree/gpl/test-prebuild
wait until it fails (ws-manager evts, DB
d_b_prebuild
)/werft with-clean-slate-deployment