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

Swapped livenessProbe and readinessProbe endpoints in 1.4.2 #4541

Closed
flozzone opened this issue Feb 16, 2021 · 10 comments
Closed

Swapped livenessProbe and readinessProbe endpoints in 1.4.2 #4541

flozzone opened this issue Feb 16, 2021 · 10 comments
Labels
language/ansible Issue is related to an Ansible operator project

Comments

@flozzone
Copy link
Contributor

flozzone commented Feb 16, 2021

Bug Report

What did you do?

Generated a ansible based operator with operator-sdk 1.4.2

What did you expect to see?

'config/manager/manager.yaml' containing the livenessProbe and readinessProbe from the upgrade guide:
https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.4.0/#for-ansible-based-operators-add-liveness-and-readiness-probe

  livenessProbe:
    httpGet:
      path: /healthz
      port: 6789
    initialDelaySeconds: 15
    periodSeconds: 20
  readinessProbe:
    httpGet:
      path: /readyz
      port: 6789
    initialDelaySeconds: 5
    periodSeconds: 10

What did you see instead? Under which circumstances?

livenessProbe using /readyz endpoint and readinessProbe using /healthz endpoint. Should be the other way around.

  livenessProbe:
    httpGet:
      path: /readyz
      port: 6789
    initialDelaySeconds: 15
    periodSeconds: 20
  readinessProbe:
    httpGet:
      path: /healthz
      port: 6789
    initialDelaySeconds: 5
    periodSeconds: 10

Environment

Operator type:

/language ansible
/language helm

Kubernetes cluster type:
Openshift
Server Version: 4.6.15
Kubernetes Version: v1.19.0+1833054

$ operator-sdk version

operator-sdk version: "v1.4.2", commit: "4b083393be65589358b3e0416573df04f4ae8d9b", kubernetes version: "1.19.4", go version: "go1.15.5", GOOS: "linux", GOARCH: "amd64"

$ kubectl version

Client Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.1", GitCommit:"c4d752765b3bbac2237bf87cf0b1c2e307844666", GitTreeState:"clean", BuildDate:"2020-12-18T12:09:25Z", GoVersion:"go1.15.5", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"19", GitVersion:"v1.19.0+1833054", GitCommit:"183305424c91636d683dfb4a379204c313976be2", GitTreeState:"clean", BuildDate:"2021-01-22T14:47:21Z", GoVersion:"go1.15.5", Compiler:"gc", Platform:"linux/amd64"}

Possible Solution

Swap the two endpoints

Additional context

Added a code review to PR #4326 regarding this.

Please see also the wrong path to the manager resource file in the upgrade guide https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.4.0/#for-ansible-based-operators-add-liveness-and-readiness-probe
config/default/manager/manager.yaml -> 'config/manager/manager.yaml'

@openshift-ci-robot openshift-ci-robot added the language/ansible Issue is related to an Ansible operator project label Feb 16, 2021
@flozzone
Copy link
Contributor Author

flozzone commented Feb 16, 2021

Please add label language/helm since helm operators seem to be affected too.

@camilamacedo86
Copy link
Contributor

WDYT about collab with the PR to fix the docs?

@camilamacedo86 camilamacedo86 added the kind/documentation Categorizes issue or PR as related to documentation. label Feb 16, 2021
@flozzone
Copy link
Contributor Author

Sure. Should I create a separate issue for the documentation changes?

@camilamacedo86 camilamacedo86 removed the kind/documentation Categorizes issue or PR as related to documentation. label Feb 16, 2021
@camilamacedo86
Copy link
Contributor

camilamacedo86 commented Feb 16, 2021

HI @flozzone,

The following is how it is Golang and how it should be for Ansible/Helm:

livenessProbe: -> /healthz
readinessProbe: -> /readyz

By checking the Anible and Helm we can see that it is inverted :

livenessProbe: -> /readyz
readinessProbe: -> /healthz

So, we need to;

  • change the templates to fix that
  • create a changelog fragment with the steps to fix it for projects already scaffolded

Then, we might need to do a pr against the released branches.

WDYT? Could you help on that?

@flozzone
Copy link
Contributor Author

Seems to be quite straight forward. I've already marked the respective parts of the code in #4326. But I will read through your code committing docs before, since this would be my first contribution.

flozzone added a commit to flozzone/operator-sdk that referenced this issue Feb 16, 2021
flozzone added a commit to flozzone/operator-sdk that referenced this issue Feb 16, 2021
Signed-off-by: flozzone <flozzone@gmail.com>
@flozzone
Copy link
Contributor Author

@camilamacedo86 these are the two commits. I've left them apart since they wouldn't fit well in one (and used the same issue number).

It looks like testdata is generated (https://github.com/operator-framework/operator-sdk/blob/master/hack/generate/samples/internal/ansible/memcached.go#L129). But I have no running dev env for this project right now. I've just fixed the testdata manually.

flozzone added a commit to flozzone/operator-sdk that referenced this issue Feb 16, 2021
* created changelog fragment
* fixed scaffold template for manager
* manually fixed testdata

Signed-off-by: flozzone <flozzone@gmail.com>
@flozzone
Copy link
Contributor Author

How should I proceed with the separated commits? squash or create a separate issue for the docs issue?

@estroz
Copy link
Member

estroz commented Feb 16, 2021

@flozzone make 2 separate PR's. The one containing the name change should have a "bugfix" changelog fragment.

flozzone added a commit to flozzone/operator-sdk that referenced this issue Feb 17, 2021
Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
flozzone added a commit to flozzone/operator-sdk that referenced this issue Feb 17, 2021
* created changelog fragment
* fixed scaffold template for the manager resource (ansible/helm)
* fixed testdata (ansible/helm)

Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
@flozzone
Copy link
Contributor Author

Is it ok, having two PRs referencing the same issue? #4545 and #4546

flozzone added a commit to flozzone/operator-sdk that referenced this issue Feb 17, 2021
* created changelog fragment
* fixed scaffold template for the manager resource (ansible/helm)
* fixed testdata (ansible/helm)

Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
flozzone added a commit to flozzone/operator-sdk that referenced this issue Feb 17, 2021
Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
estroz pushed a commit that referenced this issue Feb 17, 2021
Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/operator-sdk that referenced this issue Feb 17, 2021
Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
estroz pushed a commit that referenced this issue Feb 17, 2021
Signed-off-by: Florin Hillebrand <flozzone@gmail.com>

Co-authored-by: Florin Hillebrand <flozzone@gmail.com>
flozzone added a commit to flozzone/operator-sdk that referenced this issue Feb 17, 2021
* created changelog fragment
* fixed scaffold template for the manager resource (ansible/helm)
* fixed testdata (ansible/helm)

Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
flozzone added a commit to flozzone/operator-sdk that referenced this issue Feb 17, 2021
* created changelog fragment
* fixed scaffold template for the manager resource (ansible/helm)
* fixed testdata (ansible/helm)

Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
flozzone added a commit to flozzone/operator-sdk that referenced this issue Feb 18, 2021
* created changelog fragment
* fixed scaffold template for the manager resource (ansible/helm)
* fixed testdata (ansible/helm)

Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
estroz pushed a commit that referenced this issue Feb 18, 2021
* fixed scaffold template for the manager resource (ansible/helm)

Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this issue Feb 20, 2021
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this issue Feb 20, 2021
…perator-framework#4546)

* fixed scaffold template for the manager resource (ansible/helm)

Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
@estroz
Copy link
Member

estroz commented Feb 22, 2021

Fixed by #4546

@estroz estroz closed this as completed Feb 22, 2021
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this issue Feb 28, 2021
… (operator-framework#4545)

Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
Signed-off-by: reinvantveer <reinvantveer@gmail.com>
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this issue Feb 28, 2021
…perator-framework#4546)

* fixed scaffold template for the manager resource (ansible/helm)

Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
Signed-off-by: reinvantveer <reinvantveer@gmail.com>
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this issue Feb 28, 2021
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this issue Feb 28, 2021
…perator-framework#4546)

* fixed scaffold template for the manager resource (ansible/helm)

Signed-off-by: Florin Hillebrand <flozzone@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language/ansible Issue is related to an Ansible operator project
Projects
None yet
Development

No branches or pull requests

4 participants