-
Notifications
You must be signed in to change notification settings - Fork 41
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
Conversation
Thanks @rgolangh!
Can't you do this in this PR? |
For most of the code path it is easy except the various config maps
factories in workflowproj/operator.go
I can ignore the platform for those as a compromise
…On Thu, 6 Jun 2024 at 22:28 Ricardo Zanini ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In controllers/platform/k8s.go
<#482 (comment)>
:
> + "app.kubernetes.io/name": psh.GetServiceName(),
+ "app.kubernetes.io/component": psh.GetContainerName(),
+ "app.kubernetes.io/part-of": platform.Name,
+ "app.kubernetes.io/managed-by": "sonataflow-operator",
Please add these to
https://github.com/apache/incubator-kie-kogito-serverless-operator/blob/main/api/metadata/annotations.go#L22
------------------------------
In workflowproj/operator.go
<#482 (comment)>
:
> @@ -85,8 +85,12 @@ func GetManagedPropertiesFileName(workflow *operatorapi.SonataFlow) string {
// GetDefaultLabels gets the default labels based on the given workflow.
func GetDefaultLabels(workflow *operatorapi.SonataFlow) map[string]string {
return map[string]string{
- LabelApp: workflow.Name,
- LabelWorkflow: workflow.Name,
+ LabelApp: workflow.Name,
+ LabelWorkflow: workflow.Name,
+ "app.kubernetes.io/name": workflow.Name,
+ "app.kubernetes.io/component": "serverless-workflow",
+ "app.kubernetes.io/part-of": workflow.Status.Services.DataIndexRef.Url,
How hard it's to get the platform name here?
—
Reply to this email directly, view it on GitHub
<#482 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGBYHASWTNTNDT2WMCB55TZGCZ7DAVCNFSM6AAAAABI5J3EHWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBTGA2DANJXGQ>
.
You are receiving this because you were mentioned.Message ID:
<apache/incubator-kie-kogito-serverless-operator/pull/482/review/2103040574
@github.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, 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 |
The |
I set it to wip as it became bit messy. Please give quick feedback if you can on the idea of that refactoring. |
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.
@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.
@rgolangh this PR should ease your work: |
@rgolangh can you rebase and implement based on my PR? |
Thanks @Zanini I rebased on your changes and rolled back most of the previous changes |
@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 can you check the tests? |
oh I now see that `make test` only tests the api/ folder and not the rest.
So I saw everything working locally and overlooked the PR
…On Thu, 20 Jun 2024 at 16:02, Ricardo Zanini ***@***.***> wrote:
@rgolangh <https://github.com/rgolangh> can you check the tests?
—
Reply to this email directly, view it on GitHub
<#482 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGBYHGLYICNIBEBGLEI7SLZILHERAVCNFSM6AAAAABI5J3EHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBQGYZDOMRRGE>
.
You are receiving this because you were mentioned.Message ID:
<apache/incubator-kie-kogito-serverless-operator/pull/482/c2180627211@
github.com>
|
wdym? |
from the Makefile
|
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 |
4140cb5
to
f362560
Compare
@@ -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) |
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.
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.
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'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?
ea018f5
to
d416bfc
Compare
Repeating a discussion we had on slack: |
ok, just to add then we will remove from reconciliation the |
Ack this is probably the way to go. I'll make sure to adapt the code the add to the map and not replace. |
4e32dc3
to
373388c
Compare
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>
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:
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. So I suggest we change our helm installation to avoid this label completely. However the most important labels for now are the |
@wmedvede can you have a look? |
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 Pod for a WF deployed with the dev profile ^ I couldn't check the gitops profile, please verify. Config maps for DI and JS This is the config map for a workflow Managed properties config map Workflow deployed with the dev profile, both config maps was created by the operator: But the managed properties for same WF, no labels. When we create a WF with the dev profile, a route is created for it Labels for builds |
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.
Code looks good, many thanks.
I have left some testing nitpicks.
to summarize the artifacts the operator creates directly or indirectly with no lables:
|
Yes.
|
I think it would be excellent to label the props the user provided, because that really binds a workflow and its dependencies nicely together. |
I created #508 to add tests to cover workflowproj/operator.go |
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>
Motivation
There's a need to select efficiently workflows and their respective
services. Having the common kubernetes labels allow a single selector:
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:
Data index or Jobs services Deployment.v1 labels:
Reference: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#labels
Fixes: #483
Signed-off-by: Roy Golan rgolan@redhat.com