Support specifying annotations for devworkspace pods in the DWOC

dkwon17 commented 3 weeks ago

What does this PR do?

Provides config.workspace.podAnnotations field for DWOC to specify annotations for the devworkspace pods

What issues does this PR fix or reference?

https://github.com/devfile/devworkspace-operator/issues/1270

Is it tested? How?

Deploy DWO by using this catalog source and installing DWO from OperatorHub:

apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
name: devworkspace-operator-catalog
namespace: openshift-marketplace
spec:
sourceType: grpc
image: quay.io/dkwon17/devworkspace-operator-index:annotations-dwoc
publisher: Red Hat
displayName: DevWorkspace Operator Catalog
updateStrategy:
registryPoll:
  interval: 5m

Update the devworkspace-operator-config DWOC by adding an annotation:
```
config:
workspace:
podAnnotations:
  test-annotation: test
```

Create a workspace:

oc apply -f ./samples/plain.yaml -n openshift-operators

Once the workspace pod is created, check the pod annotations and verify that test-annoation: test exists:
```
metadata:
annotations:
test-annotation: test
...
```
PR Checklist

[ ] E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
- [ ] v8-devworkspace-operator-e2e: DevWorkspace e2e test
- [ ] v8-che-happy-path: Happy path for verification integration with Che

AObuchow commented 3 weeks ago

Tested on Minikube and works as expected :+1:

AObuchow commented 3 weeks ago

@dkwon17 I haven't debugged this to verify, but I think the CI is failing because we're now creating a workspace pod that has an empty map[string]string of annotations, which gets converted into nil when the object is created on the cluster. As a result, the SyncObjectWithCluster's diff function for pods is probably seeing that the pod on the cluster has nil annotations, when it expects it to be an empty map[string]string of annotations. This would be why we're seeing printed diffs in the CI logs:

        ... // 2 ignored fields
        Spec: v1.DeploymentSpec{
            Replicas: &1,
            Selector: &{MatchLabels: {"controller.devfile.io/devworkspace_id": "workspace05b48af9f4d14dd4"}},
            Template: v1.PodTemplateSpec{
                ObjectMeta: v1.ObjectMeta{
                    ... // 9 identical fields
                    DeletionGracePeriodSeconds: nil,
                    Labels:                     {"controller.devfile.io/creator": "", "controller.devfile.io/devworkspace_id": "workspace05b48af9f4d14dd4", "controller.devfile.io/devworkspace_name": "test-devworkspace"},
  -                 Annotations:                map[string]string{},
  +                 Annotations:                nil,
                    OwnerReferences:            nil,
                    Finalizers:                 nil,
                    ManagedFields:              nil,
                },

I think this function would be worth checking to see how it's behaving. Looking at its implementation, it doesn't seem like this would cause the diff to conclude that the cluster and spec objects are different, but maybe something subtle is going on.

It might be something else happening here, though. Will need to take a deeper look.

dkwon17 commented 2 weeks ago

Thanks for the feedback, I updated the PR, I learned that before, it was this check always caused the operator to try updating the deployment: https://github.com/devfile/devworkspace-operator/blob/b49427cecf4ba036a7587be4e7319b4dfec42dcf/pkg/provision/sync/diff.go#L58

dkwon17 commented 2 weeks ago

I've squashed some commits and reviewed: https://github.com/devfile/devworkspace-operator/pull/1267

AObuchow commented 2 weeks ago

I've squashed some commits and reviewed: #1267

Thank you :)

Sorry, I should have specified earlier, but it's probably worth updating the commit titles (and maybe descriptions) just to clarify a bit more what these commits change.

For example, b7ccba1 (#1271) could be titled as: "Add config.workspace.podAnnotations field to DWOC".

2bfdcd4 (#1271) could be titled as "Add pod annotations from DWOC & podAdditions to devworkspace pod" and the description could include "Fix #1270" to link the original issue to the commit.

Also.. I just had a second thought about a detail in this PR so please hold off on merging. Will write up my comment shortly.

openshift-ci[bot] commented 2 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AObuchow, dkwon17

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: - ~~[OWNERS](https://github.com/devfile/devworkspace-operator/blob/main/OWNERS)~~ [AObuchow,dkwon17] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

devfile / devworkspace-operator