kanisterio / kanister

An extensible framework for application-level data management on Kubernetes
https://kanister.io
Apache License 2.0
764 stars 155 forks source link

Find a UID match for pods with multiple ownerReferences #3209

Closed Alveel closed 2 weeks ago

Alveel commented 3 weeks ago

Change Overview

Currently, Kanister does not see any pods that have more than one item in their ownerReferences field as healthy. This PR aims to fix that.

Pull request type

Please check the type of change your PR introduces:

Issues

Test Plan

I am not a go developer (unfortunately), but I tried my best. I think the basics are there and should work as expected, though I understand the static strings for Kind may be less than desirable.

Test/verify:

  1. Create a Deployment/StatefulSet/etc.
  2. Edit the pod(s) from the deployment, and add an item to ownerReferences
  3. Create an ActionSet targeting this deployment
  4. It should proceed without issues
Alveel commented 3 weeks ago

I ran the tests in VSCodium, make golint and tried make test but make install-minio didn't work for me:

❯ make install-minio
make[1]: Entering directory '/home/alwyn/git/kanister'
running CMD in the containerized build environment
Running command within build container...
Deploying minio...
"minio" has been added to your repositories
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "minio" chart repository
Update Complete. ⎈Happy Helming!⎈
The connection to the server localhost:8080 was refused - did you specify the right host or port?
real 0.47
user 0.14
sys 0.03
real 0,69
user 0,00
sys 0,00
make[1]: *** [Makefile:217: run] Error 1
make[1]: Leaving directory '/home/alwyn/git/kanister'
make: *** [Makefile:266: install-minio] Error 2
pavannd1 commented 3 weeks ago

@Alveel Thank you for your contribution! 🎉 The team will review this soon. We'll look into the minio issue. Added @hairyhum and @viveksinghggits.

hairyhum commented 3 weeks ago

@Alveel any particular reason why ownerKind is there? Since we're using UID, it should not be necessary for matching (UIDs are global and not per resource kind). It's a change in the public API of the library so it's a breaking change. The change itself looks good, but it should be possible to implement without adding ownerKind.

hairyhum commented 3 weeks ago

Also tests are failing because we don't set the owner reference kind in unit tests.

Alveel commented 3 weeks ago

You're right that the UID check is stringent enough. I'll remove it!

Alveel commented 3 weeks ago

I've separated the check into a new function in utils and applied it to FetchReplicationController, FetchReplicaSet and FetchPods so they all have the same fix.

Let me know if this is okay!

Alveel commented 3 weeks ago

@hairyhum @viveksinghggits done!