Match labels #1894

weshayutin · 2025-08-06T18:54:17Z

Why the changes were made

Test use and changes for https://issues.redhat.com/browse/OADP-6248
Depends-On: openshift/hypershift-oadp-plugin#96

How to test the changes made

make test-e2e GINKGO_ARGS="--ginkgo.focus='MySQL-label application CSI'"

recreates hypershift plugin bug where restores fail w/
""Namespace mysql, resource restore error: error preparing secrets/mysql/mysql: rpc error: code = Unknown desc = backup or included namespaces is nil" "

Signed-off-by: Wesley Hayutin <[email protected]>

kaovilai

Variable Declaration Style
(tests/e2e/backup_restore_suite_test.go:167-169):
- Unnecessary variable pre-declaration could be simplified

  // Current
  var nsRequiresResticDCWorkaround bool
  var err error
  nsRequiresResticDCWorkaround, err =
  lib.NamespaceRequiresResticDCWorkaround(...)

  // Better
  nsRequiresResticDCWorkaround, err :=
  lib.NamespaceRequiresResticDCWorkaround(...)

Label Extraction Logic
(tests/e2e/backup_restore_suite_test.go:188-194):
- Only uses first label from map, which may cause unexpected behavior
- Should either support multiple labels or document single-label
limitation
- Consider using metav1.LabelSelector consistently

Signed-off-by: Wesley Hayutin <[email protected]>

weshayutin · 2025-08-13T22:48:20Z

/retest

kaovilai · 2025-08-14T17:51:39Z

Is label test still valid for hcp plugin?

weshayutin · 2025-08-14T20:09:36Z

Is label test still valid for hcp plugin?

@kaovilai the backup via label is NOT FOR hypershift. However I'm sure you agree that backing up apps ( our sample apps ) should work even if the hypershift plugin is configured in the dpa. :)

weshayutin · 2025-08-14T20:12:32Z

/test 4.19-e2e-test-aws

kaovilai · 2025-08-14T22:23:42Z

Note

This comment was generated with Claude

Critical Pod Ownership Conflict Issue

I've identified a critical pod ownership conflict in the MySQL persistent test configuration that could cause issues with pod management.

Problem: In the file mysql-persistent-csi.yaml, both the Deployment and DeploymentConfig resources are using overlapping label selectors:

Both controllers will attempt to manage pods with the same labels
This creates a conflict where multiple controllers compete for ownership of the same pods
Can lead to unpredictable behavior, pod thrashing, and failed deployments

Root Cause: The selectors are not sufficiently unique to distinguish between pods managed by different controller types.

Recommended Fix: Make the selectors more specific by adding distinct labels to differentiate between Deployment and DeploymentConfig managed pods. For example:

Deployment pods: add deployment-type: standard
DeploymentConfig pods: add deployment-type: config

This ensures each controller only manages its intended pods and prevents ownership conflicts.

weshayutin · 2025-08-18T16:01:35Z

/retest

openshift-ci · 2025-08-18T19:45:28Z

@weshayutin: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/4.20-e2e-test-hcp-aws	`5677a21`	link	false	`/test 4.20-e2e-test-hcp-aws`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

kaovilai

Selector Conflict Found - Changes Requested

Note

Responses generated with Claude

Summary

While this PR successfully implements label-based backup functionality, there's a critical selector conflict in mysql-persistent-csi.yaml that needs to be resolved.

Issue

Two controllers have overlapping selectors that can cause race conditions and pod management conflicts:

Deployment mysql: matchLabels: app: todolist
DeploymentConfig todolist: selector: app: todolist, service: todolist

Required Changes

Please update the Deployment selector to be more specific and avoid the overlap. See the inline comment for specific recommendations.

Otherwise

The implementation looks good:

✅ Clean label-based backup functionality
✅ Proper test coverage
✅ Well-structured YAML manifests

Once the selector conflict is resolved, this should be ready to merge!

weshayutin · 2025-08-18T21:20:24Z

@claude, perhaps you are correct.

Two controllers have overlapping selectors that can cause race conditions and pod management conflicts:

Deployment mysql: matchLabels: app: todolist
DeploymentConfig todolist: selector: app: todolist, service: todolist

I don't see why we care in this case. Help me understand why it's important to block this pr. The key/value pair "app: todolist" is used for selectors on the backup and restore. The key/value pair service: todolist is to label the service. Other than the value string, WHY SHOULD WE CARE?

kaovilai

visiack still works from meet

weshayutin · 2025-08-18T21:58:05Z

OK.. so @kaovilai and I jumped on a call to check this.

Deployed the app from the pr w/ 1 replicaset.. no problems
Then go to the deploymentConfig and scale up from 1 to 4.
Results - one pod crashed

Failed to create logfilelogrus.txt
time="2025-08-18T21:53:43Z" level=info msg="Failed: log file /tmp/log/todoapp/app.log create failed" func=main.main file="/build/todolist.go:226"
time="2025-08-18T21:53:43Z" level=info msg="Attempting to connect to: changeme:changeme@tcp(mysql:3306)/todolist" func=main.connectToMariaDBRemote file="/build/todolist.go:71"
2025/08/18 21:53:43 /build/todolist.go:74
[error] failed to initialize database, got error dial tcp 172.30.136.52:3306: connect: connection refused
time="2025-08-18T21:53:43Z" level=error msg="Connection failed" func=main.connectToMariaDBRemote file="/build/todolist.go:76"
time="2025-08-18T21:53:43Z" level=info msg="Attempting to connect to: test:test@tcp(127.0.0.1:3306)/todolist" func=main.connectToMariaDBLocal file="/build/todolist.go:58"
2025/08/18 21:53:43 /build/todolist.go:60
[error] failed to initialize database, got error dial tcp 127.0.0.1:3306: connect: connection refused
time="2025-08-18T21:53:43Z" level=error msg="Connection failed" func=main.connectToMariaDBLocal file="/build/todolist.go:62"
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x2cc9b8]
goroutine 1 [running]:
gorm.io/gorm.(*DB).getInstance(0x1?)
/go/pkg/mod/gorm.io/[email protected]/gorm.go:399 +0x18
gorm.io/gorm.(*DB).Migrator(0x65bec0?)
/go/pkg/mod/gorm.io/[email protected]/migrator.go:12 +0x1c
main.main()
/build/todolist.go:236 +0x1a

So the additional todolist apps are having problems connecting to the db. I don't think that is related to the label though.
¯_(ツ)_/¯

Eventually all the pods can be running. I'm going to investigate the proper way to label an app while also ensuring services etc. work properly.

whayutin@fedora:~/OPENSHIFT/git/OADP/oadp-operator/tests/e2e/sample-applications/mysql-persistent$ oc get pods -n mysql-persistent
NAME                    READY   STATUS      RESTARTS        AGE
mysql-f9cfb9bb7-45vgk   2/2     Running     0               7m40s
todolist-1-7bkcg        1/1     Running     3 (5m57s ago)   6m34s
todolist-1-deploy       0/1     Completed   0               7m40s
todolist-1-jxpt7        1/1     Running     0               7m39s
todolist-1-lprpw        1/1     Running     4 (2m57s ago)   3m59s
todolist-1-mfvd8        1/1     Running     0               6m32s
todolist-1-mlx4x        1/1     Running     0               6m33s
whayutin@fedora:~/OPENSHIFT/git/OADP/oadp-operator/tests/e2e/sample-

whayutin@fedora:~/OPENSHIFT/git/OADP/oadp-operator/tests/e2e/sample-applications/mysql-persistent$ oc get deployments -n mysql-persistent 
NAME    READY   UP-TO-DATE   AVAILABLE   AGE
mysql   1/1     1            1           8m19s
whayutin@fedora:~/OPENSHIFT/git/OADP/oadp-operator/tests/e2e/sample-applications/mysql-persistent$ oc get deploymentconfigs.apps.openshift.io -n mysql-persistent 
Warning: apps.openshift.io/v1 DeploymentConfig is deprecated in v4.14+, unavailable in v4.10000+
NAME       REVISION   DESIRED   CURRENT   TRIGGERED BY
todolist   1          5         5         config

IMHO this is good to land as is and I'll follow up w/ a

backup spec that has a match labels with an AND.. so we match 2 labels
updated labels across the app and mysql db.

kaovilai

visiack still works in meet

openshift-ci · 2025-08-18T22:00:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai, weshayutin

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:

~~tests/e2e/OWNERS~~ [kaovilai,weshayutin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kaovilai · 2025-08-19T03:54:24Z

For 2 ReplicaSets specifying the same .spec.selector but different .spec.template.metadata.labels and .spec.template.spec fields, each ReplicaSet ignores the Pods created by the other ReplicaSet.

https://kubernetes.io/docs/concepts/workloads/controllers/replicaset/

My comments would've applied if there's no ignore mechanism here.

mpryc · 2025-08-19T16:22:35Z

tests/e2e/sample-applications/mysql-persistent/mysql-backup-labels.yaml

+spec:
+  labelSelector:
+    matchLabels:
+      app: todolist


I think the label should be unique across different tests and deployments?

This may have consequences if running in parallel:
https://github.com/openshift/oadp-operator/blob/oadp-dev/tests/e2e/sample-applications/mongo-persistent/mongo-persistent-csi.yaml#L42

Secondly how about also using multiple label selectors implemented by (note this is OR operator):
vmware-tanzu/velero#4650

so mysql specific todolist label?

weshayutin added 4 commits August 6, 2025 10:19

create a test case for match labels

ea413ce

Signed-off-by: Wesley Hayutin <[email protected]>

udpate labels

d1b8ddd

properly break out the label case

ad73588

remove namespaces param from createbackupforlabeles function

d5fb9fb

openshift-ci bot requested review from eemcmullan and shubham-pampattiwar August 6, 2025 18:54

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2025

kaovilai reviewed Aug 6, 2025

View reviewed changes

weshayutin added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 6, 2025

fix label and pvc

72a081c

Signed-off-by: Wesley Hayutin <[email protected]>

weshayutin mentioned this pull request Aug 7, 2025

OADP-6248: fix restore after consult openshift/hypershift-oadp-plugin#96

Merged

4 tasks

missed a few labels

5677a21

Signed-off-by: Wesley Hayutin <[email protected]>

weshayutin removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2025

kaovilai requested changes Aug 18, 2025

View reviewed changes

kaovilai approved these changes Aug 18, 2025

View reviewed changes

mpryc reviewed Aug 19, 2025

View reviewed changes

Match labels #1894

Are you sure you want to change the base?

Match labels #1894

Conversation

weshayutin commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why the changes were made

How to test the changes made

Uh oh!

kaovilai left a comment

Choose a reason for hiding this comment

Uh oh!

weshayutin commented Aug 13, 2025

Uh oh!

kaovilai commented Aug 14, 2025

Uh oh!

weshayutin commented Aug 14, 2025

Uh oh!

weshayutin commented Aug 14, 2025

Uh oh!

kaovilai commented Aug 14, 2025

Critical Pod Ownership Conflict Issue

Uh oh!

weshayutin commented Aug 18, 2025

Uh oh!

openshift-ci bot commented Aug 18, 2025

Uh oh!

kaovilai left a comment

Choose a reason for hiding this comment

Selector Conflict Found - Changes Requested

Summary

Issue

Required Changes

Otherwise

Uh oh!

weshayutin commented Aug 18, 2025

Uh oh!

kaovilai left a comment

Choose a reason for hiding this comment

Uh oh!

weshayutin commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaovilai left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Aug 18, 2025

Uh oh!

kaovilai commented Aug 19, 2025

Uh oh!

mpryc Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

kaovilai Aug 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weshayutin commented Aug 6, 2025 •

edited

Loading

weshayutin commented Aug 18, 2025 •

edited

Loading