ws-manager: Provide more detail of DiposalStatus. #11950
Conversation
|
/werft with-preview 👎 unknown command: with-preview |
|
/werft run with-preview 👍 started the job as gitpod-build-to-ds.12 |
|
/werft with-preview 👎 unknown command: with-preview |
|
The unit tests fail. |
|
/werft run with-preview 👍 started the job as gitpod-build-to-ds.15 |
|
/werft run with-preview 👍 started the job as gitpod-build-to-ds.17 |
|
@jenting PTAL |
|
started the job as gitpod-build-to-ds.19 because the annotations in the pull request description changed |
|
started the job as gitpod-build-to-ds.21 because the annotations in the pull request description changed |
|
started the job as gitpod-build-to-ds.24 because the annotations in the pull request description changed |
|
started the job as gitpod-build-to-ds.26 because the annotations in the pull request description changed |
|
/werft run 👍 started the job as gitpod-build-to-ds.27 |
|
/werft run 👍 started the job as gitpod-build-to-ds.29 |
|
/werft run 👍 started the job as gitpod-build-to-ds.33 |
|
/unhold |
| // startedDisposalAnnotation sets to true when finalizeWorkspaceContent is called to prevent finalize from | ||
| // being called more then once, which can happen due to race between disposalStatusAnnotation update and actOnPodEvent | ||
| startedDisposalAnnotation = "gitpod.io/startedDisposal" | ||
|
|
There was a problem hiding this comment.
It's nice to see we could minimize the number of annotations affecting the state change. ❤️
| if disposalStatus.Status == DisposalEmpty { | ||
| span.LogKV("disposalStatus", "empty") | ||
| return | ||
| } |
There was a problem hiding this comment.
We could consider checking if the disposalStatus be updated or not.
If not, return it. Otherwise, call the markDisposalStatus.
For example
disposalStatus := &workspaceDisposalStatus{}
existingDisposalStatus := disposalStatus
if reflect.DeepEqual(existingDisposalStatus, disposalStatus) {
return
}
err := m.manager.markDisposalStatus(ctx, workspaceID, disposalStatus)
if err != nil {
tracing.LogError(span, err)
log.WithError(err).Error("was unable to update pod's disposal status - this will break someone's experience")
}There was a problem hiding this comment.
Or rather, should we have it updated every time I make a change rather than mark it with a defer?
There was a problem hiding this comment.
Or rather, should we have it updated every time I make a change rather than mark it with a defer?
It depends on our code convention.
I'd vote to update the status with defer function because it makes the code simple and we only have to update it once before we exit the function. Otherwise, we might update the status within a function more than once.
There was a problem hiding this comment.
I don't know the conventions of the Go language, but is it correct to use defer that way? mark should be reflected immediately, and it seemed natural to have the code in a set with the status structure changes. Could you please tell me what the convention is?
There was a problem hiding this comment.
Sorry, it's not the Go language code convention, it's more like the code convention within Gitpod, and I think we don't have the convention on when to update the status. Never mind for the defer function discussion 😄
But, it's good to check whether the resource needs to be updated before calling the kubernetes client API to update it, ref https://github.com/kubernetes/kubernetes/blob/a1128e380c2cf1c2d7443694673d9f1dd63eb518/pkg/controller/volume/persistentvolume/pv_controller.go#L794
There was a problem hiding this comment.
Good point. I'm not sure I felt we should avoid using DeepEqual if possible, right?
But, it's good to check whether the resource needs to be updated before calling the kubernetes client API to update it, ref
I wonder In what scenarios would DeepEqual be needed? I believe that the current comparison is sufficient. If you have a case in mind, please let me know. Thanks for the learning opportunity.
roboquat
added
deployed: workspace
Description
The
StartDisposalannotation so far has only shown that it has started, so we will provide more detailed information.Related Issue(s)
Fixes #
How to test
Release Notes
Documentation
Werft options: