[234] Add the ability to configure the Readiness, Liveness and Startup probes

[yersan]

Fixes #234

[jbliznak]

Thanks Yeray for fixing my concern, I have no more objections.

[tommaso-borgato]

@yersan I don't understand what triggers the usage of HTTP probes here; IIUC default (no bootableJar or http setting) should be exec probes

[yersan]

Hi @tommaso-borgato , the env variables SERVER_LIVENESS_SCRIPT and SERVER_READINESS_SCRIPT are not configured for this test.

[tommaso-borgato]

@yersan shouldn't we expect exec probes like e.g.

        exec:
          command:
            - /bin/bash
            - '-c'
            - >-
              if [ -f '/opt/eap/bin/livenessProbe.sh' ]; then
              /opt/eap/bin/livenessProbe.sh; else curl --fail
              http://127.0.0.1:9990/health/live; fi

in this case? I mean, I don't see neither bootableJar=true nor http=true configured here....

[yersan]

Hi @tommaso-borgato , the intention of this test is to verify the creation of probes that use pure HTTP request, not the Exec commands. I used the presence of the aforementioned environment variables to force the switch that triggers it.

Would it get clearer if we use bootableJar=true? That would be more close to the reality fro the product side, although we do not define them upstream.

The verification of the Exec probes is done in the TestWildFlyServerWithProbesScript

[tommaso-borgato]

@yersan what I don't understand is why in TestWildFlyServerWithHttpProbes, a CRD like:

func TestWildFlyServerWithHttpProbes(t *testing.T) {
	ctrl.SetLogger(zap.New(zap.UseDevMode(true)))
	assert := testifyAssert.New(t)

	// First verify the default values are created when there is not
	// any Probe configuration
	wildflyServer := &wildflyv1alpha1.WildFlyServer{
		ObjectMeta: metav1.ObjectMeta{
			Name:      name,
			Namespace: namespace,
		},
		Spec: wildflyv1alpha1.WildFlyServerSpec{
			ApplicationImage: applicationImage,
			Replicas:         replicas,
		},
	}

which hasn't either bootableJar or http set, when processed by the operator produces HTTP Probes instead of exec probes;

I expected something like:

wildflyServer := &wildflyv1alpha1.WildFlyServer{
		ObjectMeta: metav1.ObjectMeta{
			Name:      name,
			Namespace: namespace,
		},
		Spec: wildflyv1alpha1.WildFlyServerSpec{
			ApplicationImage: applicationImage,
			Replicas:         replicas,
			BootableJar:      true,
		},
	}

to produce HTTP probes,

or something like:

readinessProbe := &wildflyv1alpha1.ProbeSpec{
		InitialDelaySeconds: 20,
		TimeoutSeconds:      245,
		PeriodSeconds:       25,
		SuccessThreshold:    21,
		FailureThreshold:    29,
		Http: true,
	}
wildflyServer.Spec.LivenessProbe = livenessProbe

to produce HTTP probes: I mean a CRD with BootableJar: true or Http: true

[yersan]

which hasn't either bootableJar or http set, when processed by the operator produces HTTP Probes instead of exec probes;

Because the env variables SERVER_LIVENESS_SCRIPT and SERVER_READINESS_SCRIPT are not configured for this test. If they are not configured (for example the upstream operator does not configure them), then we configure the probes using HTTP get requests.

[tommaso-borgato]

@yersan Thank you for the explanation, now I see it

[yersan]

@tommaso-borgato This is now ready for your review accordingly whit what we have been discussing by removing the HTTP field and exposing the Exec and HTTPGet

[tommaso-borgato]

@yersan what about adding yet another test to check that, when StartupProbe alone is added, the other probes still default to the correct values? that would prove that the StartupProbe code path doesn't interfere with the logic already in place...

[yersan]

Hello @tommaso-borgato I don't see it necessary since there is no interaction between those conditions in the source code. Each probe uses its own method, and the existence of one vs another does not affect to any logic.

We always invoke each of the methods See

wildfly-operator/pkg/resources/statefulsets/statefulset.go

Lines 110 to 114 in a4764d0

	LivenessProbe: createLivenessProbe(w),
	// Readiness Probe is optional
	ReadinessProbe: createReadinessProbe(w),
	// StartupProbe Probe is optional
	StartupProbe: createStartupProbe(w),

Anyway, since tests are quick and the change is easy, we can add it, but it is unnecessary from my point of view.

[tommaso-borgato]

ok, no need to if they are totally independent code paths

[tommaso-borgato]

@yersan I see the bundle/manifests/wildfly.org_wildflyservers.yaml file is not updated

[yersan]

@yersan I see the bundle/manifests/wildfly.org_wildflyservers.yaml file is not updated

@tommaso-borgato We don't need to update it. It is confusing, so I guess the best would be to remove it from Git. This is the CRD generated when you are creating a bundle for the Operator. It has nothing to do with this PR.