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

minReplicas can be manually edited to be set more than maxReplicas, no validations #92

Open
ishanjoshi02 opened this issue Jul 6, 2020 · 8 comments
Labels
bug Something isn't working

Comments

@ishanjoshi02
Copy link

Used k edit wpa -n <context> to open the WPA spec in vim.

Was able to set minReplicas to 10, and maxReplicas to 1 for the same pod.

@alok87 alok87 added the bug Something isn't working label Jul 7, 2020
@alok87
Copy link
Contributor

alok87 commented Jul 8, 2020

Validation needs to be made better for the CRD

@alok87 alok87 changed the title minReplicas can be greater than maxReplicas minReplicas can be manually edit to be more than maxReplicas, no validations Aug 4, 2020
@alok87 alok87 changed the title minReplicas can be manually edit to be more than maxReplicas, no validations minReplicas can be manually edited to be set more than maxReplicas, no validations Aug 4, 2020
@ghost
Copy link

ghost commented Sep 24, 2020

Unable to edit min-replicas in kubernetes v1.18.0; Failing with error:
Invalid value: "": "spec" must validate one and only one schema (oneOf). Found 2 valid alternatives

@alok87
Copy link
Contributor

alok87 commented Sep 24, 2020

Issue is happening because the WPA has got created and the object has both replicasSetName and deploymentName.

spec:
    maxDisruption: null
    maxReplicas: 1
    minReplicas: 0
    queueURI: URI-sample
    replicaSetName: ""
    deploymentName: mailsender
    secondsToProcessOneJob: 0
    targetMessagesPerWorker: 1

cc @matkam

@aleclerc-sonrai
Copy link

I have the same issue as @adityabhatia02 with both deploymentName and replicasSetName having been set, however replicasSetName got defaulted to "" when it was never specified in the yaml. I'm using Helm3 and k8s 1.15

@matkam
Copy link
Contributor

matkam commented Nov 17, 2020

I'm not too familiar with CRD validation rules. Is it possible to set the validation oneOf to be "not empty/blank" instead of required? Here's the CRD currently: https://github.com/practo/k8s-worker-pod-autoscaler/blob/master/artifacts/crd.yaml#L42

Would it make sense to use anyOf instead of oneOf validation? Or to even remove the oneOf requirement and handle incorrect states in the operator code. If both fields exist, use deploymentName first. If invalid, try replicaSetName next. If both values are invalid, then throw an error.

@aleclerc-sonrai
Copy link

To be honest I'm not either, as a temporary measure I did delete the restriction and everything is fine. What I'm trying to figure out and I think is ultimately the problem, is why is replicaName getting defaulted when it's not in my yaml.

@alok87
Copy link
Contributor

alok87 commented Nov 18, 2020

oneOf does work as we expect it to. Here is an example:

$ cat wpa.yaml
apiVersion: k8s.practo.dev/v1
kind: WorkerPodAutoScaler
metadata:
  labels:
    app: voice
  name: testoneof
spec:
    maxDisruption: null
    maxReplicas: 1
    minReplicas: 0
    queueURI: beanstalk://beanstalkd/mail-sender
    replicaSetName: ""
    deploymentName: mailsender
    secondsToProcessOneJob: 0
    targetMessagesPerWorker: 1
$ k create -f wpa.yaml
The WorkerPodAutoScaler "testoneof" is invalid: : Invalid value: "": "spec" must validate one and only one schema (oneOf). Found 2 valid alternatives

oneOf prevents both replicaSetName and deploymetName to get set. As @aleclerc-sonrai we should may be find out how did the value get set? Was the CRD validation updated after the object was created?

@aleclerc-sonrai
Copy link

aleclerc-sonrai commented Nov 18, 2020

No it wasn't. My current theory is that the operator itself, when it is updating the status, is somehow also updating the spec.replicaSetName to ""...my yaml definitely doesn't have the field specified anywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants