Skip to content

Commit 905be0a

Browse files
sagor999roboquat
authored andcommitted
address PR feedback
1 parent 9920417 commit 905be0a

File tree

7 files changed

+58
-59
lines changed

7 files changed

+58
-59
lines changed

components/ws-daemon/pkg/internal/session/workspace.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -280,9 +280,9 @@ func (s *Workspace) UpdateGitSafeDirectory(ctx context.Context) (err error) {
280280
}
281281

282282
// UpdateGitStatus attempts to update the LastGitStatus from the workspace's local working copy.
283-
func (s *Workspace) UpdateGitStatus(ctx context.Context, persistent_volume_claim bool) (res *csapi.GitStatus, err error) {
283+
func (s *Workspace) UpdateGitStatus(ctx context.Context, persistentVolumeClaim bool) (res *csapi.GitStatus, err error) {
284284
var loc string
285-
if persistent_volume_claim {
285+
if persistentVolumeClaim {
286286
loc = filepath.Join(s.ServiceLocDaemon, "prestophookdata")
287287
stat, err := git.GitStatusFromFiles(ctx, loc)
288288
if err != nil {

components/ws-manager/cmd/run.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ import (
3232
imgbldr "github.com/gitpod-io/gitpod/image-builder/api"
3333
"github.com/gitpod-io/gitpod/ws-manager/pkg/manager"
3434
"github.com/gitpod-io/gitpod/ws-manager/pkg/proxy"
35-
volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
35+
volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
3636
)
3737

3838
// serveCmd represents the serve command

components/ws-manager/go.mod

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ require (
2020
github.com/google/uuid v1.3.0
2121
github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
2222
github.com/imdario/mergo v0.3.12
23+
github.com/kubernetes-csi/external-snapshotter/client/v4 v4.2.0
2324
github.com/opentracing/opentracing-go v1.2.0
2425
github.com/prometheus/client_golang v1.12.1
2526
github.com/sirupsen/logrus v1.8.1
@@ -74,8 +75,6 @@ require (
7475
github.com/inconshreveable/mousetrap v1.0.0 // indirect
7576
github.com/json-iterator/go v1.1.12 // indirect
7677
github.com/klauspost/cpuid v1.3.1 // indirect
77-
github.com/klauspost/pgzip v1.2.5 // indirect
78-
github.com/kubernetes-csi/external-snapshotter/client/v6 v6.0.0-rc4 // indirect
7978
github.com/matttproud/golang_protobuf_extensions v1.0.2-0.20181231171920-c182affec369 // indirect
8079
github.com/minio/md5-simd v1.1.0 // indirect
8180
github.com/minio/minio-go/v7 v7.0.11 // indirect

components/ws-manager/go.sum

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -408,10 +408,8 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ=
408408
github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI=
409409
github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
410410
github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
411-
github.com/linuxkit/virtsock v0.0.0-20201010232012-f8cee7dfc7a3/go.mod h1:3r6x7q95whyfWQpmGZTu3gk3v2YkMi05HEzl7Tf7YEo=
412-
github.com/kubernetes-csi/external-snapshotter/client/v6 v6.0.0-rc4 h1:uspy64y8fTrwchSk4dKx/CAndt0y2V70BFM2C9/jTIE=
413-
github.com/kubernetes-csi/external-snapshotter/client/v6 v6.0.0-rc4/go.mod h1:tnHiLn3P10N95fjn7O40QH5ovN0EFGAxqdTpUMrX6bU=
414-
github.com/magiconair/properties v1.8.0/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ=
411+
github.com/kubernetes-csi/external-snapshotter/client/v4 v4.2.0 h1:nHHjmvjitIiyPlUHk/ofpgvBcNcawJLtf4PYHORLjAA=
412+
github.com/kubernetes-csi/external-snapshotter/client/v4 v4.2.0/go.mod h1:YBCo4DoEeDndqvAn6eeu0vWM7QdXmHEeI9cFWplmBys=
415413
github.com/magiconair/properties v1.8.1/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ=
416414
github.com/magiconair/properties v1.8.5 h1:b6kJs+EmPFMYGkow9GiUyCyOvIwYetYJ3fSaWak/Gls=
417415
github.com/magiconair/properties v1.8.5/go.mod h1:y3VJvCyxH9uVvJTWEGAELF3aiYNyPKd5NZ3oSwXrF60=
@@ -858,6 +856,7 @@ golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
858856
golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
859857
golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
860858
golang.org/x/time v0.0.0-20191024005414-555d28b269f0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
859+
golang.org/x/time v0.0.0-20200416051211-89c76fbcd5d1/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
861860
golang.org/x/time v0.0.0-20210220033141-f8bda1e9f3ba/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
862861
golang.org/x/time v0.0.0-20210723032227-1f47c861a9ac/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
863862
golang.org/x/time v0.0.0-20220411224347-583f2d630306 h1:+gHMid33q6pen7kv9xvT+JRinntgeXO2AeZVd0AWD3w=

components/ws-manager/pkg/manager/create.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import (
3333
regapi "github.com/gitpod-io/gitpod/registry-facade/api"
3434
"github.com/gitpod-io/gitpod/ws-manager/api"
3535
config "github.com/gitpod-io/gitpod/ws-manager/api/config"
36+
volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
3637
)
3738

3839
// Protobuf structures often require pointer to boolean values (as that's Go's best means of expression optionallity).
@@ -259,8 +260,8 @@ func (m *Manager) createPVCForWorkspacePod(startContext *startWorkspaceContext)
259260
PVC.Spec.StorageClassName = &PVCConfig.StorageClass
260261
}
261262

262-
if startContext.VolumeSnapshot.VolumeSnapshotName != "" {
263-
snapshotApiGroup := "snapshot.storage.k8s.io"
263+
if startContext.VolumeSnapshot != nil && startContext.VolumeSnapshot.VolumeSnapshotName != "" {
264+
snapshotApiGroup := volumesnapshotv1.GroupName
264265
PVC.Spec.DataSource = &corev1.TypedLocalObjectReference{
265266
APIGroup: &snapshotApiGroup,
266267
Kind: "VolumeSnapshot",
@@ -907,10 +908,12 @@ func (m *Manager) newStartWorkspaceContext(ctx context.Context, req *api.StartWo
907908
workspaceClassLabel: clsName,
908909
}
909910

910-
var volumeSnapshot workspaceVolumeSnapshotStatus
911+
var volumeSnapshot *workspaceVolumeSnapshotStatus
911912
if req.Spec.VolumeSnapshot != nil {
912-
volumeSnapshot.VolumeSnapshotName = req.Spec.VolumeSnapshot.VolumeSnapshotName
913-
volumeSnapshot.VolumeSnapshotHandle = req.Spec.VolumeSnapshot.VolumeSnapshotHandle
913+
volumeSnapshot = &workspaceVolumeSnapshotStatus{
914+
VolumeSnapshotName: req.Spec.VolumeSnapshot.VolumeSnapshotName,
915+
VolumeSnapshotHandle: req.Spec.VolumeSnapshot.VolumeSnapshotHandle,
916+
}
914917
}
915918

916919
return &startWorkspaceContext{

components/ws-manager/pkg/manager/manager.go

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -68,17 +68,17 @@ type Manager struct {
6868
}
6969

7070
type startWorkspaceContext struct {
71-
Request *api.StartWorkspaceRequest `json:"request"`
72-
Labels map[string]string `json:"labels"`
73-
CLIAPIKey string `json:"cliApiKey"`
74-
OwnerToken string `json:"ownerToken"`
75-
IDEPort int32 `json:"idePort"`
76-
SupervisorPort int32 `json:"supervisorPort"`
77-
WorkspaceURL string `json:"workspaceURL"`
78-
TraceID string `json:"traceID"`
79-
Headless bool `json:"headless"`
80-
Class *config.WorkspaceClass `json:"class"`
81-
VolumeSnapshot workspaceVolumeSnapshotStatus `json:"volumeSnapshot"`
71+
Request *api.StartWorkspaceRequest `json:"request"`
72+
Labels map[string]string `json:"labels"`
73+
CLIAPIKey string `json:"cliApiKey"`
74+
OwnerToken string `json:"ownerToken"`
75+
IDEPort int32 `json:"idePort"`
76+
SupervisorPort int32 `json:"supervisorPort"`
77+
WorkspaceURL string `json:"workspaceURL"`
78+
TraceID string `json:"traceID"`
79+
Headless bool `json:"headless"`
80+
Class *config.WorkspaceClass `json:"class"`
81+
VolumeSnapshot *workspaceVolumeSnapshotStatus `json:"volumeSnapshot"`
8282
}
8383

8484
func (swctx *startWorkspaceContext) ContainerConfiguration() config.ContainerConfiguration {

components/ws-manager/pkg/manager/monitor.go

Lines changed: 32 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ import (
3939
"github.com/gitpod-io/gitpod/ws-manager/api"
4040
"github.com/gitpod-io/gitpod/ws-manager/pkg/manager/internal/workpool"
4141

42-
volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
42+
volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v4/apis/volumesnapshot/v1"
4343
)
4444

4545
const (
@@ -870,13 +870,13 @@ func (m *Monitor) finalizeWorkspaceContent(ctx context.Context, wso *workspaceOb
870870
deletedPVC bool
871871
pvcFeatureEnabled bool
872872
markVolumeSnapshotAnnotation bool
873-
pvcVolumeSnapshotName string
873+
// volume snapshot name is 1:1 mapped to workspace id
874+
pvcVolumeSnapshotName string = workspaceID
874875
pvcVolumeSnapshotContentName string
875876
pvcVolumeSnapshotClassName string
876877
)
877878
if wso.Pod != nil {
878879
_, pvcFeatureEnabled = wso.Pod.Labels[pvcWorkspaceFeatureAnnotation]
879-
pvcVolumeSnapshotName = workspaceID
880880
wsClassName := ""
881881
if _, ok := wso.Pod.Labels[workspaceClassLabel]; ok {
882882
wsClassName = wso.Pod.Labels[workspaceClassLabel]
@@ -929,16 +929,14 @@ func (m *Monitor) finalizeWorkspaceContent(ctx context.Context, wso *workspaceOb
929929
}
930930

931931
if pvcFeatureEnabled {
932+
// pvc was created with the name of the pod. see createDefiniteWorkspacePod()
932933
pvcName := wso.Pod.Name
933934
if !createdVolumeSnapshot {
934935
// create snapshot object out of PVC
935936
volumeSnapshot := &volumesnapshotv1.VolumeSnapshot{
936937
ObjectMeta: metav1.ObjectMeta{
937938
Name: pvcVolumeSnapshotName,
938939
Namespace: m.manager.Config.Namespace,
939-
Labels: map[string]string{
940-
"workspaceID": workspaceID,
941-
},
942940
},
943941
Spec: volumesnapshotv1.VolumeSnapshotSpec{
944942
Source: volumesnapshotv1.VolumeSnapshotSource{
@@ -949,7 +947,7 @@ func (m *Monitor) finalizeWorkspaceContent(ctx context.Context, wso *workspaceOb
949947
}
950948

951949
err = m.manager.Clientset.Create(ctx, volumeSnapshot)
952-
if err != nil {
950+
if err != nil && !k8serr.IsAlreadyExists(err) {
953951
err = xerrors.Errorf("cannot create volumesnapshot: %v", err)
954952
return true, nil, err
955953
}
@@ -963,6 +961,7 @@ func (m *Monitor) finalizeWorkspaceContent(ctx context.Context, wso *workspaceOb
963961
Jitter: 0.1,
964962
Cap: 10 * time.Minute,
965963
}
964+
log = log.WithField("VolumeSnapshot.Name", pvcVolumeSnapshotName)
966965
err = wait.ExponentialBackoff(backoff, func() (bool, error) {
967966
var volumeSnapshot volumesnapshotv1.VolumeSnapshot
968967
err := m.manager.Clientset.Get(ctx, types.NamespacedName{Namespace: m.manager.Config.Namespace, Name: pvcVolumeSnapshotName}, &volumeSnapshot)
@@ -971,37 +970,38 @@ func (m *Monitor) finalizeWorkspaceContent(ctx context.Context, wso *workspaceOb
971970
// volumesnapshot doesn't exist yet, retry again
972971
return false, nil
973972
}
974-
log.WithError(err).WithField("VolumeSnapshot.Name", pvcVolumeSnapshotName).Error("was unable to get volume snapshot")
973+
log.WithError(err).Error("was unable to get volume snapshot")
975974
return false, err
976975
}
977976
if volumeSnapshot.Status != nil {
978-
if volumeSnapshot.Status.ReadyToUse != nil && *(volumeSnapshot.Status.ReadyToUse) {
977+
if volumeSnapshot.Status.ReadyToUse != nil && *(volumeSnapshot.Status.ReadyToUse) && volumeSnapshot.Status.BoundVolumeSnapshotContentName != nil {
979978
pvcVolumeSnapshotContentName = *volumeSnapshot.Status.BoundVolumeSnapshotContentName
980979
return true, nil
981980
}
982981
if volumeSnapshot.Status.Error != nil {
983982
if volumeSnapshot.Status.Error.Message != nil {
984983
err = xerrors.Errorf("error during volume snapshot creation: %s", *volumeSnapshot.Status.Error.Message)
985-
log.WithError(err).WithField("VolumeSnapshot.Name", pvcVolumeSnapshotName).Error("unable to create volume snapshot")
984+
log.WithError(err).Error("unable to create volume snapshot")
986985
return false, err
987986
}
988-
log.WithField("VolumeSnapshot.Name", pvcVolumeSnapshotName).Error("unknown error during volume snapshot creation")
987+
log.Error("unknown error during volume snapshot creation")
989988
return false, xerrors.Errorf("unknown error during volume snapshot creation")
990989
}
991990
}
992991
return false, nil
993992
})
994993
if err != nil {
995-
log.WithError(err).WithField("VolumeSnapshot.Name", pvcVolumeSnapshotName).Errorf("failed while waiting for volume snapshot to get ready")
994+
log.WithError(err).Errorf("failed while waiting for volume snapshot to get ready")
996995
return true, nil, err
997996
}
998997
readyVolumeSnapshot = true
999998
}
1000999
if readyVolumeSnapshot && !markVolumeSnapshotAnnotation {
1000+
log = log.WithField("VolumeSnapshotContent.Name", pvcVolumeSnapshotContentName)
10011001
var volumeSnapshotContent volumesnapshotv1.VolumeSnapshotContent
10021002
err := m.manager.Clientset.Get(ctx, types.NamespacedName{Namespace: "", Name: pvcVolumeSnapshotContentName}, &volumeSnapshotContent)
10031003
if err != nil {
1004-
log.WithError(err).WithField("VolumeSnapshotContent.Name", pvcVolumeSnapshotContentName).Error("was unable to get volume snapshot content")
1004+
log.WithError(err).Error("was unable to get volume snapshot content")
10051005
return true, nil, err
10061006
}
10071007

@@ -1038,38 +1038,36 @@ func (m *Monitor) finalizeWorkspaceContent(ctx context.Context, wso *workspaceOb
10381038
},
10391039
},
10401040
)
1041-
if pvcErr != nil {
1041+
if pvcErr != nil && !k8serr.IsNotFound(pvcErr) {
10421042
log.WithError(pvcErr).Errorf("failed to delete pvc `%s`", pvcName)
10431043
}
10441044
deletedPVC = true
10451045
}
1046-
} else {
1047-
if doSnapshot {
1048-
// if this is a prebuild take a snapshot and mark the workspace
1049-
var res *wsdaemon.TakeSnapshotResponse
1050-
res, err = snc.TakeSnapshot(ctx, &wsdaemon.TakeSnapshotRequest{Id: workspaceID})
1046+
} else if doSnapshot {
1047+
// if this is a prebuild take a snapshot and mark the workspace
1048+
var res *wsdaemon.TakeSnapshotResponse
1049+
res, err = snc.TakeSnapshot(ctx, &wsdaemon.TakeSnapshotRequest{Id: workspaceID})
1050+
if err != nil {
1051+
tracing.LogError(span, err)
1052+
log.WithError(err).Warn("cannot take snapshot")
1053+
err = xerrors.Errorf("cannot take snapshot: %v", err)
1054+
err = m.manager.markWorkspace(ctx, workspaceID, addMark(workspaceExplicitFailAnnotation, err.Error()))
1055+
if err != nil {
1056+
log.WithError(err).Warn("was unable to mark workspace as failed")
1057+
}
1058+
}
1059+
1060+
if res != nil {
1061+
err = m.manager.markWorkspace(context.Background(), workspaceID, addMark(workspaceSnapshotAnnotation, res.Url))
10511062
if err != nil {
10521063
tracing.LogError(span, err)
1053-
log.WithError(err).Warn("cannot take snapshot")
1054-
err = xerrors.Errorf("cannot take snapshot: %v", err)
1064+
log.WithError(err).Warn("cannot mark headless workspace with snapshot - that's one prebuild lost")
1065+
err = xerrors.Errorf("cannot remember snapshot: %v", err)
10551066
err = m.manager.markWorkspace(ctx, workspaceID, addMark(workspaceExplicitFailAnnotation, err.Error()))
10561067
if err != nil {
10571068
log.WithError(err).Warn("was unable to mark workspace as failed")
10581069
}
10591070
}
1060-
1061-
if res != nil {
1062-
err = m.manager.markWorkspace(context.Background(), workspaceID, addMark(workspaceSnapshotAnnotation, res.Url))
1063-
if err != nil {
1064-
tracing.LogError(span, err)
1065-
log.WithError(err).Warn("cannot mark headless workspace with snapshot - that's one prebuild lost")
1066-
err = xerrors.Errorf("cannot remember snapshot: %v", err)
1067-
err = m.manager.markWorkspace(ctx, workspaceID, addMark(workspaceExplicitFailAnnotation, err.Error()))
1068-
if err != nil {
1069-
log.WithError(err).Warn("was unable to mark workspace as failed")
1070-
}
1071-
}
1072-
}
10731071
}
10741072
}
10751073

0 commit comments

Comments
 (0)