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.

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

Set recommended kubernetes labels on services and workflows #482

Merged
merged 1 commit into from
Jul 25, 2024

Conversation

rgolangh
Copy link
Contributor

@rgolangh rgolangh commented Jun 6, 2024

Motivation

There's a need to select efficiently workflows and their respective
services. Having the common kubernetes labels allow a single selector:

   podSelector:
     matchExpressions:
       - { key: app.kubernetes.io/component, operator: In, values:
         ["data-index-service", "jobs-service", "serverless-workflow"] }

Modification:
Make the v1.Deployment for services and the deployment or knative
services to contain at common labels

Result:
A workflow deployment or knative serving labels:

    app.kubernetes.io/name: ${workflow name}
    app.kubernetes.io/component: serverless-workflow
    app.kubernetes.io/part-of: ${platform url set by status}
    app.kubernetes.io/managed-by: sonataflow-operator

Data index or Jobs services Deployment.v1 labels:

    app.kubernetes.io/name: ${service name}
    app.kubernetes.io/component: data-index-service|jobs-service
    app.kubernetes.io/part-of: ${platform name}
    app.kubernetes.io/managed-by: sonataflow-operator

Reference: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels

Fixes: #483
Signed-off-by: Roy Golan rgolan@redhat.com

@ricardozanini
Copy link
Member

Thanks @rgolangh!

A future improvement will be to pass the platform name to the workflow
so app.kubernetes.io/part-of will have the platform name so we can
select all component for a platform.

Can't you do this in this PR?

controllers/platform/k8s.go Outdated Show resolved Hide resolved
workflowproj/operator.go Outdated Show resolved Hide resolved
@rgolangh
Copy link
Contributor Author

rgolangh commented Jun 7, 2024 via email

Copy link
Contributor

@wmedvede wmedvede left a comment

Choose a reason for hiding this comment

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

LGTM, just the same comment about te part-of

@rgolangh
Copy link
Contributor Author

rgolangh commented Jun 7, 2024

LGTM, just the same comment about te part-of

i would have to get the active platform set in the workflow reconciler and then pass it on to the rest of the state ensurers
some of them already do it i think
wdyt?

@ricardozanini
Copy link
Member

@rgolangh

For most of the code path it is easy except the various config maps
factories in workflowproj/operator.go

The workflowproj won't care about the platform AFAIK, so what we can do is to add a Platform argument to the constructors so a client can pass the platform (e.g. kn cli) when generating the manifests.

@rgolangh rgolangh changed the title Set recommended kubernetes labels on services and workflows WIP: Set recommended kubernetes labels on services and workflows Jun 7, 2024
@rgolangh
Copy link
Contributor Author

rgolangh commented Jun 7, 2024

I set it to wip as it became bit messy. Please give quick feedback if you can on the idea of that refactoring.
I think I want to leave only a single ensurer that has the platform . same for the object creator. that should remove some uneeded code
I had a place or two that are used just to generate the workflowproj so not in real deployment time. like @ricardozanini mentioned

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

@rgolangh I'll refactor the code to add the platform reference to the SonataFlow object. Then we can return to this feature, wdyt? That will ease your implementation and we can avoid passing the platform reference everywhere.

controllers/profiles/common/ensurer.go Outdated Show resolved Hide resolved
@ricardozanini
Copy link
Member

ricardozanini commented Jun 11, 2024

@rgolangh this PR should ease your work:

@ricardozanini
Copy link
Member

@rgolangh can you rebase and implement based on my PR?

@rgolangh
Copy link
Contributor Author

Thanks @Zanini I rebased on your changes and rolled back most of the previous changes

@ricardozanini
Copy link
Member

@rgolangh can you please check the unit tests? Also, before sending the PR do:

make generate-all
make vet fmt

I need to update the CONTRIB guide.

@rgolangh rgolangh changed the title WIP: Set recommended kubernetes labels on services and workflows Set recommended kubernetes labels on services and workflows Jun 19, 2024
@ricardozanini
Copy link
Member

@rgolangh can you check the tests?

@rgolangh
Copy link
Contributor Author

rgolangh commented Jun 20, 2024 via email

@ricardozanini
Copy link
Member

oh I now see that make test only tests the api/ folder and not the rest.

wdym? make test runs everything, including the api module.

@rgolangh
Copy link
Contributor Author

oh I now see that make test only tests the api/ folder and not the rest.

wdym? make test runs everything, including the api module.

from the Makefile

test: manifests generate envtest vet fmt test-api ## Run tests.
...
test-api:
	cd api && make test

@ricardozanini
Copy link
Member

Yes, it runs everything.

@rgolangh
Copy link
Contributor Author

Yes, it runs everything.

it stays on the api folder after the cd I think

@ricardozanini
Copy link
Member

Yes, it runs everything.

it stays on the api folder after the cd I think

It does not, just tested here:

make test
test -s /Users/ricferna/go/src/github.com/apache/incubator-kie-kogito-serverless-operator/bin/controller-gen || GOBIN=/Users/ricferna/go/src/github.com/apache/incubator-kie-kogito-serverless-operator/bin go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.15.0
/Users/ricferna/go/src/github.com/apache/incubator-kie-kogito-serverless-operator/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./api/..." paths="./container-builder/api/..."
/Users/ricferna/go/src/github.com/apache/incubator-kie-kogito-serverless-operator/bin/controller-gen rbac:roleName=manager-role crd:allowDangerousTypes=true webhook paths="./api/..." paths="./internal/controller/..." output:crd:artifacts:config=config/crd/bases
go vet ./...
./hack/goimports.sh
api/v1alpha08/zz_generated.deepcopy.go
api/zz_generated.deepcopy.go
container-builder/api/zz_generated.deepcopy.go
go work sync
go: downloading github.com/apache/incubator-kie-kogito-serverless-operator/workflowproj v0.0.0
go mod tidy
go fmt ./...
cd api && make test
go test github.com/apache/incubator-kie-kogito-serverless-operator/api github.com/apache/incubator-kie-kogito-serverless-operator/api/metadata github.com/apache/incubator-kie-kogito-serverless-operator/api/v1alpha08 -coverprofile cover.out
        github.com/apache/incubator-kie-kogito-serverless-operator/api          coverage: 0.0% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/api/metadata 0.378s  coverage: 57.1% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/api/v1alpha08        0.959s  coverage: 4.7% of statements
KUBEBUILDER_ASSETS="/Users/ricferna/go/src/github.com/apache/incubator-kie-kogito-serverless-operator/bin/k8s/1.26.1-darwin-amd64" go test github.com/apache/incubator-kie-kogito-serverless-operator/cmd github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/builder github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/cfg github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/clusterplatform github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/discovery github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/knative github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/openshift github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/platform github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/platform/services github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/common github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/common/constants github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/common/persistence github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/common/properties github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/common/variables github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/dev github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/factory github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/gitops github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/preview github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/workflowdef github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/workflows github.com/apache/incubator-kie-kogito-serverless-operator/log github.com/apache/incubator-kie-kogito-serverless-operator/test github.com/apache/incubator-kie-kogito-serverless-operator/utils github.com/apache/incubator-kie-kogito-serverless-operator/utils/kubernetes github.com/apache/incubator-kie-kogito-serverless-operator/utils/openshift github.com/apache/incubator-kie-kogito-serverless-operator/utils/resources github.com/apache/incubator-kie-kogito-serverless-operator/version -coverprofile cover.out
        github.com/apache/incubator-kie-kogito-serverless-operator/cmd          coverage: 0.0% of statements
        github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/common/constants                coverage: 0.0% of statements
        github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/clusterplatform          coverage: 0.0% of statements
        github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/knative          coverage: 0.0% of statements
        github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/openshift                coverage: 0.0% of statements
?       github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/common/variables        [no test files]
        github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/factory         coverage: 0.0% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller  3.878s  coverage: 44.1% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/builder  1.437s  coverage: 34.9% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/cfg      3.947s  coverage: 50.0% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/discovery        2.529s  coverage: 74.1% of statements
?       github.com/apache/incubator-kie-kogito-serverless-operator/log  [no test files]
        github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/workflows                coverage: 0.0% of statements
        github.com/apache/incubator-kie-kogito-serverless-operator/test         coverage: 0.0% of statements
        github.com/apache/incubator-kie-kogito-serverless-operator/utils/resources              coverage: 0.0% of statements
        github.com/apache/incubator-kie-kogito-serverless-operator/version              coverage: 0.0% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/platform 6.859s  coverage: 5.0% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/platform/services        5.003s  coverage: 39.6% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles 5.922s  coverage: 20.6% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/common  7.762s  coverage: 19.5% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/common/persistence      6.642s  coverage: 37.7% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/common/properties       7.096s  coverage: 45.8% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/dev     7.839s  coverage: 58.0% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/gitops  6.500s  coverage: 20.8% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/profiles/preview 7.993s  coverage: 45.7% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/internal/controller/workflowdef      7.046s  coverage: 23.3% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/utils        7.208s  coverage: 11.5% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/utils/kubernetes     7.508s  coverage: 15.9% of statements
ok      github.com/apache/incubator-kie-kogito-serverless-operator/utils/openshift      7.894s  coverage: 40.0% of statements

@rgolangh rgolangh force-pushed the common-labels branch 3 times, most recently from 4140cb5 to f362560 Compare June 25, 2024 13:29
@@ -76,6 +76,7 @@ const (
// It serves as a basis for a basic Quarkus Java application, expected to listen on http 8080.
func DeploymentCreator(workflow *operatorapi.SonataFlow, plf *operatorapi.SonataFlowPlatform) (client.Object, error) {
lbl := workflowproj.GetMergedLabels(workflow)
delete(lbl, workflowproj.LabelK8SPartOF)
Copy link
Member

Choose a reason for hiding this comment

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

Instead, can we have a GetLabelsForSelectors() and remove the items we don't want or create a new map for the items we want (preferred).

Then we use it only on the selectors field. Having the default merged labels in the objects is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do that and in addition I think it is the best to also use the platform object we have there and not use the platform from the status of the workflow because it is not there - hence the errors in the controller log while trying to update the deployment selector
wdyt?

controllers/profiles/gitops/profile_gitops_test.go Outdated Show resolved Hide resolved
@rgolangh rgolangh force-pushed the common-labels branch 2 times, most recently from ea018f5 to d416bfc Compare June 25, 2024 15:18
@rgolangh
Copy link
Contributor Author

Repeating a discussion we had on slack:
Since the selector labels are immutable we would constantly fail to reconcile existing deployments.
This means that existing deployments would fall out of the network policies based on the labels set here.

@ricardozanini
Copy link
Member

ok, just to add then we will remove from reconciliation the labels from the selector. Labels from other objects won't be replaced by the object management but MERGED instead. This way we avoid this kind of problem when migrating from different versions.

@rgolangh
Copy link
Contributor Author

ok, just to add then we will remove from reconciliation the labels from the selector. Labels from other objects won't be replaced by the object management but MERGED instead. This way we avoid this kind of problem when migrating from different versions.

Ack this is probably the way to go. I'll make sure to adapt the code the add to the map and not replace.

@rgolangh rgolangh force-pushed the common-labels branch 2 times, most recently from 4e32dc3 to 373388c Compare June 30, 2024 11:13
Motivation:
There's a need to select efficiently workflows and their respective
services. Having the common kubernetes labels allow a single selector:

   podSelector:
     matchExpressions:
       - { key: app.kubernetes.io/component, operator: In, values:
         ["data-index-service", "jobs-service", "serverless-workflow"] }

Modification:
Make the v1.Deployment for services and the deployment or knative
services to contain at common labels

Result:
A workflow deployment or knative serving labels:
    app.kubernetes.io/name: ${workflow name}
    app.kubernetes.io/component: serverless-workflow
    app.kubernetes.io/part-of: ${platform url set by status}
    app.kubernetes.io/managed-by: sonataflow-operator

Data index or Jobs services Deployment.v1 labels:

    app.kubernetes.io/name: ${service name}
    app.kubernetes.io/component: data-index-service|jobs-service
    app.kubernetes.io/part-of: ${platform name}
    app.kubernetes.io/managed-by: sonataflow-operator

Reference: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels

Signed-off-by: Roy Golan <rgolan@redhat.com>
@rgolangh
Copy link
Contributor Author

rgolangh commented Jun 30, 2024

e2e test fixed and are green now.

one last note: installing sonata using an helm for example, would take over this label on all installed artifacts:

app.kubernetes.io/part-of: Helm

which ruins the nice query we thought about to get all the artifacts of a platform.

Fortunately according to helm documentation this label usage is optional and not mandatory.
https://helm.sh/docs/chart_best_practices/labels/

So I suggest we change our helm installation to avoid this label completely. However the most important labels for now are the app.kunernetes.io/component with the values of serverless-workflow, data-index-service, jobs-service

@rgolangh
Copy link
Contributor Author

rgolangh commented Jul 3, 2024

@wmedvede can you have a look?

@rgolangh rgolangh requested a review from wmedvede July 15, 2024 10:29
@wmedvede
Copy link
Contributor

Hi @rgolangh , great work, many thanks.

Here goes some nitpicks I could find doing piece of testing.

Pod for a WF deployed with preview profile

Part-of-preview-Screenshot from 2024-07-19 14-49-09

Pod for a WF deployed with the dev profile

Part-of-devmode-Screenshot from 2024-07-19 14-49-31

^ I couldn't check the gitops profile, please verify.

Config maps for DI and JS

Config-map-for-sevices-Screenshot from 2024-07-19 14-55-05

This is the config map for a workflow

Config-maps-workflow-1-Screenshot from 2024-07-19 14-56-54

Managed properties config map

Config-maps-workflow2-Screenshot from 2024-07-19 15-00-07

Workflow deployed with the dev profile, both config maps was created by the operator:

Config-map-created-by-operator-Screenshot from 2024-07-19 15-08-01

But the managed properties for same WF, no labels.
Config-map-created-by-operator-Screenshot from 2024-07-19 15-10-25

When we create a WF with the dev profile, a route is created for it

Route-dev-mode-Screenshot from 2024-07-19 15-04-23

Labels for builds
When a WF is created in Openshift with the preview mode, the following objects are created.

A-build-config-Screenshot from 2024-07-19 15-26-36

A-potential-build-Screenshot from 2024-07-19 15-28-27

A-potential-image-stream-Screenshot from 2024-07-19 15-30-45

A-pod-build-Screenshot from 2024-07-19 15-36-18

Copy link
Contributor

@wmedvede wmedvede left a comment

Choose a reason for hiding this comment

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

Code looks good, many thanks.
I have left some testing nitpicks.

@rgolangh
Copy link
Contributor Author

rgolangh commented Jul 22, 2024

to summarize the artifacts the operator creates directly or indirectly with no lables:

  • the build pod
  • route in dev prof without part-of
  • managedProps
  • pod in dev prof without part-of

@wmedvede
Copy link
Contributor

wmedvede commented Jul 24, 2024

to summarize the artifacts the operator creates directly or indirectly with no lables:

  • the build pod
  • route in dev prof without part-of
  • managedProps
  • pod in dev prof without part-of

Yes.
Then, one thing that will look weird is to have the managed props, created by the operator, labeled, and the "standard" props (user provided) not labeled. Also, regarding the "standard" props, we have two situations.

  1. the user never provided the "standard" props. In that case, the operator still creates an empty one, this is good for consistency. In this case, the operator can label them.

  2. the user has provided the "standard" props. Here, the operator should label them?
    From the user perspective might look weird to see the config map he provided to gain some labels automatically after some time 🤔 , but on the other hand, for consistence we must label them.

@rgolangh
Copy link
Contributor Author

I think it would be excellent to label the props the user provided, because that really binds a workflow and its dependencies nicely together.

@wmedvede
Copy link
Contributor

@ricardozanini

I'm approving and merging this with the gaps stated in the screenshots above to not block another feature that is being developed by @rgolangh on top of this labels.

@rgolangh Would you mind open an issue to keep track of the gaps that are going be left so this can be tackled at some point.

@wmedvede wmedvede merged commit a6a954a into apache:main Jul 25, 2024
4 checks passed
@rgolangh
Copy link
Contributor Author

@wmedvede created #509

@rgolangh
Copy link
Contributor Author

rgolangh commented Jul 25, 2024

I created #508 to add tests to cover workflowproj/operator.go

rgdoliveira pushed a commit to rgdoliveira/kogito-serverless-operator that referenced this pull request Aug 6, 2024
Motivation:
There's a need to select efficiently workflows and their respective
services. Having the common kubernetes labels allow a single selector:

   podSelector:
     matchExpressions:
       - { key: app.kubernetes.io/component, operator: In, values:
         ["data-index-service", "jobs-service", "serverless-workflow"] }

Modification:
Make the v1.Deployment for services and the deployment or knative
services to contain at common labels

Result:
A workflow deployment or knative serving labels:
    app.kubernetes.io/name: ${workflow name}
    app.kubernetes.io/component: serverless-workflow
    app.kubernetes.io/part-of: ${platform url set by status}
    app.kubernetes.io/managed-by: sonataflow-operator

Data index or Jobs services Deployment.v1 labels:

    app.kubernetes.io/name: ${service name}
    app.kubernetes.io/component: data-index-service|jobs-service
    app.kubernetes.io/part-of: ${platform name}
    app.kubernetes.io/managed-by: sonataflow-operator

Reference: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels

Signed-off-by: Roy Golan <rgolan@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Place common labels on workflows and platform services
3 participants