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.

Sign up for GitHub

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

Limit objects cached by DevWorkspace controller to reduce memory usage #652

Merged
merged 14 commits into from
Nov 5, 2021
Merged

Limit objects cached by DevWorkspace controller to reduce memory usage #652

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3d3732d
Restrict which k8s objects are cached in runtime
amisevsk Oct 14, 2021
d6a12b1
Restrict secrets and configmaps in the cache via new labels
amisevsk Oct 14, 2021
1b2d3b7
Update how workspace metadata configmap is handled
amisevsk Oct 14, 2021
4e4e5b3
Collect sync code into one interface to simplify syncing with cluster
amisevsk Oct 15, 2021
1b4a7e0
Adapt more packages to use sync package for managing objects
amisevsk Oct 15, 2021
0008111
Update devworkspaceRouting controller to use sync package for objects
amisevsk Oct 15, 2021
2130ef1
Add documentation and logging to pkg/provision/sync package
amisevsk Oct 15, 2021
43aae00
Fix cache initialization on Kubernetes
amisevsk Oct 18, 2021
465405e
Use sync package for async storage deployment
amisevsk Oct 18, 2021
8290933
Rework automount package to use ClusterAPI instead of client
amisevsk Oct 18, 2021
299186d
Adapt automount git credentials to use new sync mechanism
amisevsk Oct 18, 2021
76d43d7
Treat updating services specially since we have to copy ClusterIP
amisevsk Oct 19, 2021
6c83e0a
Improve diff logging when updating items
amisevsk Oct 19, 2021
8e07871
Restrict caches for Roles and Rolebindings by name
amisevsk Oct 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions pkg/cache/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
package cache

import (
"fmt"

"github.com/devfile/devworkspace-operator/pkg/constants"
routev1 "github.com/openshift/api/route/v1"
appsv1 "k8s.io/api/apps/v1"
Expand All @@ -33,6 +35,16 @@ func GetCacheFunc() (cache.NewCacheFunc, error) {
return nil, err
}

// We have to treat secrets and configmaps separately since we need to support auto-mounting
secretObjectSelector, err := labels.Parse(fmt.Sprintf("%s=true", constants.DevWorkspaceWatchSecretLabel))
if err != nil {
return nil, err
}
configmapObjectSelector, err := labels.Parse(fmt.Sprintf("%s=true", constants.DevWorkspaceWatchConfigMapLabel))
if err != nil {
return nil, err
}

selectors := cache.SelectorsByObject{
&appsv1.Deployment{}: {
Label: devworkspaceObjectSelector,
Expand All @@ -55,6 +67,12 @@ func GetCacheFunc() (cache.NewCacheFunc, error) {
&routev1.Route{}: {
Label: devworkspaceObjectSelector,
},
&corev1.ConfigMap{}: {
Label: configmapObjectSelector,
},
&corev1.Secret{}: {
Label: secretObjectSelector,
},
}

return cache.BuilderWithOptions(cache.Options{
Expand Down
8 changes: 8 additions & 0 deletions pkg/constants/metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ const (
// DevWorkspaceNameLabel is the label key to store workspace name
DevWorkspaceNameLabel = "controller.devfile.io/devworkspace_name"

// DevWorkspaceWatchConfigMapLabel marks a configmap so that it is watched by the controller. This label is required on all
// configmaps that should be seen by the controller
DevWorkspaceWatchConfigMapLabel = "controller.devfile.io/watch-configmap"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we need a dedicated label for each of objects type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

WDYM? For most objects, the devworkspace_id label is sufficient, but I had to add separate ones for configmaps/secrets since those by default do not have one label they always use. We might run into a similar issue to workaround for Deployments if we continue supporting async storage, but we could fudge that by using devworkspace_id: all or something.

We could use one label for both secrets and configmaps, but then we run into the issue of how to name it -- watch-resource may be unclear as it only applies to configmaps/secrets, and we use different labels for other objects.

Copy link
Member

Choose a reason for hiding this comment

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

I think you got my question)

We could use one label for both secrets and configmaps, but then we run into the issue of how to name it -- watch-resource may be unclear as it only applies to configmaps/secrets, and we use different labels for other objects.

That's exactly what I have in mind, including the concern ) So, then I think it makes sense to leave as is.

Copy link
Member

Choose a reason for hiding this comment

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

One tiny +1 to use common watch annotation - it will allow us to avoid having different articles/section to some docs, like here https://docs.google.com/document/d/1IR78XlxO37VTWXOu-uE-2nKC93D1GNhZomGoRaN518o/edit?usp=sharing

So, we'll just provide a single patch command instead two.

The concern:

We could use one label for both secrets and configmaps, but then we run into the issue of how to name it -- watch-resource may be unclear as it only applies to configmaps/secrets, and we use different labels for other objects.

may be addressed by the following explanation:

DevWorkspace operator watches objects owned by DevWorkspace CR (ones which are in additional labeled with workspace id) or standalone additional objects labeled with watch: true

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that there always is at least some kind of label on the secret/configmap that DWO handles, even if such labels differ depending on the purpose.

I think (I have not tried this out), it should be possible to write an "OR" label selector - if not using the existing code then by implementing a custom labels.Selector.

I personally think requiring 2 labels on a single object for a single purpose is a little bit weird from the UX perspective.

Copy link
Contributor

Choose a reason for hiding this comment

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

an "or" selector is apparently impossible, so please ignore me :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I personally think requiring 2 labels on a single object for a single purpose is a little bit weird from the UX perspective.

The reality is that there are multiple labels that can get applied to configmaps or secrets, and they each serve a different purpose

  1. controller.devfile.io/watch-[secret|configmap]: mark this secret/configmap as "of interest" to the controller; necessary due to caching change
  2. controller.devfile.io/mount-to-devworkspace: mount this resource to the workspace; used by external tools/users to share info across multiple workspaces
  3. controller.devfile.io/git-credential: mark a secret as holding git credentials, which is handled differently from above
  4. controller.devfile.io/devworkspace_id: associate this resource with the workspace with workspace ID specified.

Cases 2, 3 and 4 can exist independently of each other, e.g. a user-defined mounted configmap won't have the devworkspace_id label, and the metadata configmap we provision for workspaces won't have the mount-to-devworkspace label. As a result, there's no label selector we can use here, so we have to add the watch label to cover all use cases. Moreover, there will be cases when there are secrets/configmaps on the cluster that we're interested in that only have the controller.devfile.io/watch-[secret|configmap] label and no others.

The concern [...] may be addressed by the following explanation:

DevWorkspace operator watches objects owned by DevWorkspace CR (ones which are in additional labeled with workspace id) or standalone additional objects labeled with watch: true

Potentially, but it's still somewhat unclear: we watch PVCs without the label applied, and secrets/configmaps become invisible if the label is removed, even if it has the workspace ID label. I'm open to using one label for both, but I'm not sure it's a huge gain in documentation burden.


// DevWorkspaceWatchSecretLabel marks a secret so that it is watched by the controller. This label is required on all
// secrets that should be seen by the controller
DevWorkspaceWatchSecretLabel = "controller.devfile.io/watch-secret"

// DevWorkspaceMountLabel is the label key to store if a configmap or secret should be mounted to the devworkspace
DevWorkspaceMountLabel = "controller.devfile.io/mount-to-devworkspace"

Expand Down
6 changes: 4 additions & 2 deletions pkg/provision/storage/asyncstorage/configmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"
"strings"

"github.com/devfile/devworkspace-operator/pkg/constants"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand All @@ -37,8 +38,9 @@ func getSSHAuthorizedKeysConfigMapSpec(namespace string, authorizedKeys []byte)
Name: sshAuthorizedKeysConfigMapName,
Namespace: namespace,
Labels: map[string]string{
"app.kubernetes.io/name": "async-storage", // TODO
"app.kubernetes.io/part-of": "devworkspace-operator",
"app.kubernetes.io/name": "async-storage", // TODO
"app.kubernetes.io/part-of": "devworkspace-operator",
constants.DevWorkspaceWatchConfigMapLabel: "true",
},
},
Data: map[string]string{
Expand Down
6 changes: 4 additions & 2 deletions pkg/provision/storage/asyncstorage/secret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"

"github.com/devfile/devworkspace-operator/pkg/constants"
wsprovision "github.com/devfile/devworkspace-operator/pkg/provision/workspace"
)

Expand All @@ -40,8 +41,9 @@ func getSSHSidecarSecretSpec(workspace *dw.DevWorkspace, privateKey []byte) *cor
Name: GetSSHSidecarSecretName(workspace.Status.DevWorkspaceId),
Namespace: workspace.Namespace,
Labels: map[string]string{
"app.kubernetes.io/name": "async-storage", // TODO
"app.kubernetes.io/part-of": "devworkspace-operator",
"app.kubernetes.io/name": "async-storage", // TODO
"app.kubernetes.io/part-of": "devworkspace-operator",
constants.DevWorkspaceWatchSecretLabel: "true",
},
},
Data: map[string][]byte{
Expand Down
10 changes: 6 additions & 4 deletions pkg/provision/workspace/automount/git-credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,9 @@ func getGitSecret(secretName string, namespace string, config string) *corev1.Se
Name: secretName,
Namespace: namespace,
Labels: map[string]string{
"app.kubernetes.io/name": "git-config-secret",
"app.kubernetes.io/part-of": "devworkspace-operator",
"app.kubernetes.io/name": "git-config-secret",
"app.kubernetes.io/part-of": "devworkspace-operator",
constants.DevWorkspaceWatchSecretLabel: "true",
},
},
Data: map[string][]byte{
Expand Down Expand Up @@ -234,8 +235,9 @@ func getGitConfigMap(configMapName string, namespace string, config string) *cor
Name: configMapName,
Namespace: namespace,
Labels: map[string]string{
"app.kubernetes.io/name": "git-config-secret",
"app.kubernetes.io/part-of": "devworkspace-operator",
"app.kubernetes.io/name": "git-config-secret",
"app.kubernetes.io/part-of": "devworkspace-operator",
constants.DevWorkspaceWatchConfigMapLabel: "true",
},
},
Data: map[string]string{
Expand Down