volcano-sh / volcano

A Cloud Native Batch System (Project under CNCF)
https://volcano.sh
Apache License 2.0
4.13k stars 953 forks source link

Admission webhook - hierarchical queues can not have hierarchy parts that are a substring [0:i] of other queue hierarchy. #3759

Open PigNatovsky opened 5 days ago

PigNatovsky commented 5 days ago

Description

If I create two queues like these:

---
apiVersion: scheduling.volcano.sh/v1beta1
kind: Queue
metadata:
  name: root-namewithsuffix
  annotations:
    "volcano.sh/hierarchy": "root/namewithsuffix"
    "volcano.sh/hierarchy-weights": "1/1"
spec:
  reclaimable: true
---
apiVersion: scheduling.volcano.sh/v1beta1
kind: Queue
metadata:
  name: root-namewith
  annotations:
    "volcano.sh/hierarchy": "root/namewith"
    "volcano.sh/hierarchy-weights": "1/1"
spec:
  reclaimable: true

Admission webhook throws:

Error from server: error when creating "./local/queues-hdrf.yaml": admission webhook "validatequeue.volcano.sh" denied the request: requestBody.metadata.annotations: Invalid value: "root/namewith": root/namewith is not allowed to be in the sub path of root/namewithsuffix of queue root-namewithsuffix

Steps to reproduce the issue

  1. Create queue with an annotation: "volcano.sh/hierarchy": "root/namewithsuffix"
  2. Try to create a queue with an annotation: "volcano.sh/hierarchy": "root/namewith"

Describe the results you received and expected

As mentioned, the result is a queue revocation done by the admission webhook. My expectation is that I should be able to create queues like root/someproject and root/someprojectdev.

What version of Volcano are you using?

1.9.0

Any other relevant information

I can pick up this issue, because currently I'm working on admission webhook in our internal env.

PigNatovsky commented 5 days ago

/assign

PigNatovsky commented 5 days ago

I think that issue is here: https://github.com/volcano-sh/volcano/blob/639a4ba73f9dc71fd95f6d5937f0a78144659648/pkg/webhooks/admission/queues/validate/validate_queue.go#L157

We're just checking if our annotation hierarchy is a substring of other hierarchy, but it's not always a good check, because hierarchy may be at the same length, but with name that contains substring.

PigNatovsky commented 5 days ago

Simple solution: just add a "/" at the end of hierarchy before check.

Example 1: Queue in the cluster: "root/somename/suffixdev" (because first one was a dev env for this project) New queue: "root/somename/suffix" (time for a prd queue)

hierarchyInTree: "root/somename/suffixdev" hierarchy: "root/somename/suffix"

How it works now? strings.HasPrefix("root/somename/suffixdev", "root/somename/suffix") gives true which throws an error.

How it will work? strings.HasPrefix("root/somename/suffixdev", "root/somename/suffix/") gives false and accepts queue.

Example 2 (from code comment): Queue in the cluster: "root/sci/dev" New queue: "root/sci"

hierarchyInTree: "root/sci/dev" hierarchy: "root/sci"

How it works now? strings.HasPrefix("root/sci/dev", "root/sci") gives true and throws an error.

How it will work? strings.HasPrefix("root/sci/dev", "root/sci") gives true and throws an error (and it should).