-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: Use labels on PVCs for better reconciling #1233
Conversation
I will review this PR |
Name: ownerref.Name, | ||
Namespace: obj.GetNamespace(), | ||
// No need to reconcile PVC, if it doesn't have a label specifying its type | ||
pvcLabel, ok := obj.GetLabels()[constants.DevWorkspacePVCTypeLabel] |
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.
If a user upgrades from an older version of DWO to the latest version, will the older common PVCs get the new label?
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.
@mkuznyetsov this is also my concern. There was an older PR that added labels per-workspace PVCs, but I'm almost certain this only impacted new per-workspace PVCs.
My concern stems from 2 things:
1. Making sure existing PVCs won't be deleted, then re-created to apply the label.
This would cause user data loss. Looking at the sync code, it seems like PVCs are not considered mutable, so the new label would not get applied to existing PVCs. There is however a note that we could still allow for updating labels.
If we want to update existing PVCs with the label, this complicates this PR further: either the sync code needs to be updated to only allow modifying PVCs if the labels are modified, or we need to manually diff
the PVC after calling SyncObjectWithCluster()
, see that the labels are missing and call sync.updateObjectGeneric()
to apply the labels. The former approach is much more clean, though a bit trickier as SyncObjectWithCluster()
is a critical function called throughout DWO.
2. If we decide to not update existing PVCs with the new label, then the dwPVCHandler()
should still work on PVCs that are missing the new label.
This is currently not the case with the following check:
// No need to reconcile, if PVC doesn't have a label specifying its type
pvcLabel, ok := obj.GetLabels()[constants.DevWorkspacePVCTypeLabel]
if !ok {
return []reconcile.Request{}
}
You'd have to change the dwPVCHandler
code to not assume that the constants.DevWorkspacePVCTypeLabel
is applied to the PVC in question. If it does not exist, the behaviour should match the behaviour prior to this PR (i.e. the global common PVC name is assumed to be used, and the original PR bug will be present). If the label does exist, then you can make use of the logic that you've implemented.
In general, I think it's easier as a first step to have this PR not update existing PVCs with the new label (i.e. consider my 2nd concern). A later PR could update the sync code to allow label updating on PVCs.
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.
alright, I will rewrite to account for the use of PVC with missing label, and leave a reference to this PR why this has to be this way
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.
pr rewritten to support old usecase, when there is no new label
for _, workspace := range dwList.Items { | ||
// determin if PVC belongs to the workspace that has to be reconcile by its name | ||
// which comes from the DW operator global, or external config that may be defined | ||
// for this workspace |
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.
I suggest:
Determine workspaces to reconcile that use the current common PVC. Workspaces can either use the common PVC where the PVC name is coming from the global config, or from an external config the workspace might use
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.
comment applied
// for this workspace | ||
var workspacePvcName string; | ||
|
||
if (workspace.Spec.Template.Attributes.Exists(constants.ExternalDevWorkspaceConfiguration)) { |
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.
Should we check if the workspace uses common/per-user storage by checking
storageType := workspace.Spec.Template.Attributes.GetString(constants.DevWorkspaceStorageTypeAttribute, nil)
first?
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.
yes, thanks
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.
@mkuznyetsov Thank you for the great first PR 🥳 Excited to have you contributing to the project. I've made a few initial remarks, but overall things are looking good :)
externalConfig, _ := wkspConfig.ResolveConfigForWorkspace(&workspace, r.Client) | ||
workspacePvcName = externalConfig.Workspace.PVCName | ||
} | ||
if len(workspacePvcName) == 0 { |
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.
We can avoid this if
statement by immediately setting workspacePvcName
to be the default global Config's PVC name. Then, the value of workspacePvcName
will be modified if an external DWOC is used.
So line 84 could be:
workspacePVCName := wkspConfig.GetGlobalConfig().Workspace.PVCName
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.
Also side note: the pattern we tend to use for checking empty strings is if myString == "" {...}
. IMO this makes it a bit more clear that we're checking for an empty string.
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.
fixed
var workspacePvcName string; | ||
|
||
if (workspace.Spec.Template.Attributes.Exists(constants.ExternalDevWorkspaceConfiguration)) { | ||
externalConfig, _ := wkspConfig.ResolveConfigForWorkspace(&workspace, r.Client) |
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.
We can't ignore the error returned by ResolveConfigForWorkspace()
since in error cases, it will return nil, error
. Thus, trying to get the value of externalConfig.Workspace.PVCName
will give a null pointer exception.
If there's an error that occurred, we should not modify the value of workspacePvcName
. Ideally, we should log the error as well, though there isn't a logger set up in this file (and I'm not sure if you can log from an event handler yet?). I can look into this myself.
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.
added logger
Name: ownerref.Name, | ||
Namespace: obj.GetNamespace(), | ||
// No need to reconcile PVC, if it doesn't have a label specifying its type | ||
pvcLabel, ok := obj.GetLabels()[constants.DevWorkspacePVCTypeLabel] |
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.
@mkuznyetsov this is also my concern. There was an older PR that added labels per-workspace PVCs, but I'm almost certain this only impacted new per-workspace PVCs.
My concern stems from 2 things:
1. Making sure existing PVCs won't be deleted, then re-created to apply the label.
This would cause user data loss. Looking at the sync code, it seems like PVCs are not considered mutable, so the new label would not get applied to existing PVCs. There is however a note that we could still allow for updating labels.
If we want to update existing PVCs with the label, this complicates this PR further: either the sync code needs to be updated to only allow modifying PVCs if the labels are modified, or we need to manually diff
the PVC after calling SyncObjectWithCluster()
, see that the labels are missing and call sync.updateObjectGeneric()
to apply the labels. The former approach is much more clean, though a bit trickier as SyncObjectWithCluster()
is a critical function called throughout DWO.
2. If we decide to not update existing PVCs with the new label, then the dwPVCHandler()
should still work on PVCs that are missing the new label.
This is currently not the case with the following check:
// No need to reconcile, if PVC doesn't have a label specifying its type
pvcLabel, ok := obj.GetLabels()[constants.DevWorkspacePVCTypeLabel]
if !ok {
return []reconcile.Request{}
}
You'd have to change the dwPVCHandler
code to not assume that the constants.DevWorkspacePVCTypeLabel
is applied to the PVC in question. If it does not exist, the behaviour should match the behaviour prior to this PR (i.e. the global common PVC name is assumed to be used, and the original PR bug will be present). If the label does exist, then you can make use of the logic that you've implemented.
In general, I think it's easier as a first step to have this PR not update existing PVCs with the new label (i.e. consider my 2nd concern). A later PR could update the sync code to allow label updating on PVCs.
pkg/constants/metadata.go
Outdated
@@ -20,6 +20,9 @@ const ( | |||
// DevWorkspaceIDLabel is the label key to store workspace identifier | |||
DevWorkspaceIDLabel = "controller.devfile.io/devworkspace_id" | |||
|
|||
// DevWorkspacePVCTypeLabel is the label key to identify DeWorkspace common PVCs and their type |
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 comment is slightly inaccurate, as we are using this label for both per-workspace and common/per-user PVC's.
I would rewrite it as:
// DevWorkspacePVCTypeLabel is the label key to identify PVCs used by DevWorkspaces and indicate their storage strategy.
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.
thx, comment applied
@mkuznyetsov sorry about the failing CI tests, that's due to #1242 (not related to your PR). I've opened #1243 to resolve this. |
if obj.GetDeletionTimestamp() == nil { | ||
return []reconcile.Request{} | ||
} | ||
// we can wrap the code below for handling per-workspace PVCs |
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.
Thanks for reporting #1250 -- I think this comment could be now simplified to something along the lines of:
// TODO: Ensure all new and existing PVC's get the `controller.devfile.io/devworkspace_pvc_type` label.
// See: https://github.com/devfile/devworkspace-operator/issues/1250
pkg/constants/metadata.go
Outdated
@@ -20,6 +20,9 @@ const ( | |||
// DevWorkspaceIDLabel is the label key to store workspace identifier | |||
DevWorkspaceIDLabel = "controller.devfile.io/devworkspace_id" | |||
|
|||
// DevWorkspacePVCTypeLabel is the label key to identify PVCs used by DevWorkspaces and indicate their storage strategy. | |||
DevWorkspacePVCTypeLabel = "controller.devfile.io/devworkspace_pvc_type_label" |
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.
I think the actual string value of this label could just be controller.devfile.io/devworkspace_pvc_type
(i.e. remove the _label
part at the end), since it's implied by its usage as a label in the PVC spec, and its name DevWorkspacePVCTypeLabel
in the code.
if workspace.Spec.Template.Attributes.Exists(constants.ExternalDevWorkspaceConfiguration) { | ||
externalConfig, err := wkspConfig.ResolveConfigForWorkspace(&workspace, r.Client) | ||
if err != nil { | ||
r.Log.Info("Couldn't fetch external config for workspace %s, using PVC Name from global config instead", err.Error()) |
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.
We should test this logger works as expected by:
- Creating an external DWOC that uses a custom PVC name
- Using the external DWOC in a workspace that uses the common PVC storage strategy
- Starting up the workspace that uses the external DWOC, wait for it to be in the READY state
- Deleting the external DWOC
- Deleting the common PVC
- Ensure the log appears in DWO's logs
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.
Example logger output, if devworkpace had it external config deleted, when attempting to reconcile
{"level":"info","ts":"2024-04-10T18:39:02+03:00","logger":"controllers.DevWorkspace","msg":"Couldn't resolve PVC name for workspace 'external-plain1' in namespace 'devworkspace-controller', using PVC name 'claim-devworkspace' from global config instead: could not fetch external DWOC with name ec1 in namespace default: DevWorkspaceOperatorConfig.controller.devfile.io \"ec1\" not found."}
@mkuznyetsov when you get a chance, please rebase your PR onto the main branch so that the Validate PR GH Action will pass btw :) |
pkg/provision/storage/shared.go
Outdated
if pvc.Labels == nil { | ||
pvc.Labels = map[string]string{} | ||
} | ||
pvc.Labels[constants.DevWorkspacePVCTypeLabel] = constants.CommonStorageClassType |
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.
Could you change this to constants.PerUserStorageClassType
please? Since "common" is the old/legacy name, and the user-facing name is "per-user"
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.
@mkuznyetsov overall looks good to me :) Manual testing was complicated by #1258, but your PR seems good to me.
Let's wait until I get back from PTO on May 9th before merging this.
Amazing work & congrats on your first (soon to be finally merged!!) PR :D
@mkuznyetsov I think this PR is good to merge :) Before we do so, can you squash your commits with a Additionally, the commit description of |
r.Log.Info(fmt.Sprintf("Couldn't resolve PVC name for workspace '%s' in namespace '%s', using PVC name '%s' from global config instead: %s.", workspace.Name, workspace.Namespace, workspacePVCName, err.Error())) | ||
} else { | ||
storageType := workspace.Spec.Template.Attributes.GetString(constants.DevWorkspaceStorageTypeAttribute, nil) | ||
if storageType == constants.CommonStorageClassType || storageType == constants.PerUserStorageClassType { |
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.
Do we need to also check storageType == ""
here?
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.
@dkwon17 good catch!! Yes, we should be checking this too @mkuznyetsov
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.
Also, should we be doing this check in the first line of this for loop instead?
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.
Because AFAIK we are looking for common/per-user workspaces to reconcile in this for loop. Otherwise, it's possible to have ephemeral and per-workspace workspaces added in this for loop as well
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.
+1 @dkwon17 you're right, the order of checking for workspace storage strategy before iterating over a workspace should have not changed. @mkuznyetsov please restore the 2 lines that were moved for checking the workspace storage strategy, so that they occur before we retrieve the PVC name.
pkg/provision/storage/shared.go
Outdated
@@ -61,7 +61,6 @@ func WorkspaceNeedsStorage(workspace *dw.DevWorkspaceTemplateSpec) bool { | |||
} | |||
|
|||
func getPVCSpec(name, namespace string, storageClass *string, size resource.Quantity) (*corev1.PersistentVolumeClaim, error) { | |||
|
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.
@mkuznyetsov sorry for nit-pick, but could you please also restore this newline so that the diff is a bit cleaner?
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.
If possible, please re-add this newline.
pkg/provision/storage/shared.go
Outdated
@@ -61,7 +61,7 @@ func WorkspaceNeedsStorage(workspace *dw.DevWorkspaceTemplateSpec) bool { | |||
} | |||
|
|||
func getPVCSpec(name, namespace string, storageClass *string, size resource.Quantity) (*corev1.PersistentVolumeClaim, error) { | |||
|
|||
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.
If possible, please remove this tab (unless it's coming from go format)
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.
oops, fixed it in the amended commit
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.
Please run make format
with goimports installed go install golang.org/x/tools/cmd/goimports@latest
Signed-off-by: Mykhailo Kuznietsov <mkuznets@redhat.com>
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.
LGTM :)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AObuchow, mkuznyetsov The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Congrats on having your first PR merged to the project @mkuznyetsov 🎉 Amazing work :) |
What does this PR do?
Add new labels to indicate PVCs that are in use by Devworkspace Operator.
This allows to trigger workspace reconcile, that must be triggered upon PVC deletion.
and if it's a common PVC that had a name configured by external operator config, only those workspaces that would have the same PVC name should be reconciled
What issues does this PR fix or reference?
#920
Is it tested? How?
Steps to test the new & old behavior with sample DevWorkspaces (DW)
https://github.com/devfile/devworkspace-operator/?tab=readme-ov-file#run-controller-locally
Inspect PVC objects, that are created for these repos.
build and run a new version of operator from this pull-request:
export NAMESPACE="devworkspace-controler"
...
make run
Create DWs from step TODO:
Observe a label "controller.devfile.io/devworkspace_pvc_type" added to certain kubernetes PVC objects:
claim-devworkspace
PVC will not have this label, since this PVC was created with old DWO. Even new DWOold-custom-pvc-name
PVC will not have this label, butnew-custom-pvc-name
will have itNow start deleting PVCs one after another with
kubectl delete pvc
And see that workspaces are reconciled respectivelly.
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-path
to trigger)v8-devworkspace-operator-e2e
: DevWorkspace e2e testv8-che-happy-path
: Happy path for verification integration with Che