Remove pkg/controller/ready

[imjasonh]

This package wrote a file to report that the controller was live and ready, which was unnecessary.

This change also removes some unused code in pkg/controller/controller.go

Fixes #732

/kind cleanup

Submitter Checklist

• [n/a] Includes tests if functionality changed/was added
• [n/a] Includes docs if changes are user-facing
• [y] Set a kind label on this PR
• [y] Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

The controller no longer configures a readiness probe or liveness probe.

/assign @SaschaSchwarze0

[SaschaSchwarze0]

Here is an alternative using the metrics endpoint:

        ports:
        - containerPort: 8383
          name: metrics-port
        livenessProbe:
          httpGet:
            path: /metrics
            port: metrics-port
          initialDelaySeconds: 5
          periodSeconds: 10
        readinessProbe:
          httpGet:
            path: /metrics
            port: metrics-port
          initialDelaySeconds: 5
          periodSeconds: 10

We will likely need to use this downstream as we require a probe. It is probably as good as the Tekton solution.

[imjasonh]

Do you require both a liveness probe and a readiness probe?

[SaschaSchwarze0]

I do not know, would need to check. But, you are right, we probably should here only define the liveness probe because there is no service defined. In downstream we have our own deployment yaml anyway, so, if we would have to specify a readiness probe, we can always deviate from what is defined in upstream.

[imjasonh]

If you have your own downstream deployment anyway, I think I'd prefer to remove both probes here for now, until we decide there's real value in adding them. Does that sound okay to you?

[SaschaSchwarze0]

I think I am okay. The risk then is that we use something in downstream (a probe based on the metrics endpoint) that is not "tested" upstream. But, if it would stop working (for whatever reason) and not being repairable, then I think we could fairly quickly introduce a new basic liveness endpoint like Tekton does.

[qu1queee]

We need both probes for some required standards/policies downstream. I will prefer to keep both in upstream so that we can continuously test them, if any vendor downstream does not require them, then they should remove it. PR looks good for me.

[xiujuan95]

Hi, @imjasonh maybe just for your info, I am not sure if liveness/readiness probes can work well for non-leader pods or not when we use metrics endpoint do these two checks.

Tekton-controller is using metrics endpoint to check liveness/readiness and it works normally. This is because tekton is using active/active HA mode for leader-election. That means each tekton controller pod can be the leader, only different pods hold different buckets at the same time.

However, for build side, we are using Leader-with-lease HA mode. And all requests are held by a unique controller pod. Other pods are stand by. Also before we have an issue when we use metrics endpoint to do liveness/readiness probes: #276.

[SaschaSchwarze0]

Hi, @imjasonh maybe just for your info, I am not sure if liveness/readiness probes can work well for non-leader pods or not when we use metrics endpoint do these two checks.

Tekton-controller is using metrics endpoint to check liveness/readiness and it works normally. This is because tekton is using active/active HA mode for leader-election. That means each tekton controller pod can be the leader, only different pods hold different buckets at the same time.

However, for build side, we are using Leader-with-lease HA mode. And all requests are held by a unique controller pod. Other pods are stand by. Also before we have an issue when we use metrics endpoint to do liveness/readiness probes: #276.

@xiujuan95 in our downstream dev environment, the build controller is running with the httpGet based probes since yesterday and there has not been an alert yet = the probes must be working. You may also check the metrics endpoint of non-leading pods in our environment. They are present (they just do not contain build-related metrics which is expected).

[qu1queee]

@xiujuan95 its a good point, I was also wondering why it works now. I think something changed in the controller-runtime pkg that allowed us to serve the /metrics endpoint in both the leader and passive pods.

[qu1queee]

/lgtm

[qu1queee]

@imjasonh @SaschaSchwarze0 whats the state of this PR?

[SaschaSchwarze0]

/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SaschaSchwarze0

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:

• OWNERS [SaschaSchwarze0]

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