Open scothis opened 4 years ago
My concern about this is what the "base" looks like (i.e. do I always have to list those opinions), and how I override an opinion that someone else has opted to apply (e.g. central IT).
I don't think /actuator/health
is a good default for readiness. From what I've seen including external resources in a readiness check is considered by some to be a dangerous. For example:
If the readiness probe is verifying a dependency that is exclusive to the container—a private cache or database—then you can be more aggressive in failing the readiness probe, with the assumption that container dependencies are independent. However, if the readiness probe is verifying a shared dependency—like a common service used for authentication, authorization, metrics, logging, or metadata—you should be very conservative in failing the readiness probe.
Given that we don't know what health indicators a user has in their app, I think the default should err on the side of caution and not use the entirety of /health
for readiness. We'll have something dedicated to readiness in Boot 2.3, but for 2.2.x you might want to explore automatically configuring a health endpoint group that only contains indicators for local or exclusive-use dependencies.
My concern about this is what the "base" looks like
The Deployment before injection and the SpringBootContainer resources are what the user would provide. The .spec.template.spec
in the Deployment and the previous SpringBootApplication resources is identical. The only difference in .spec.template
is that a new label is added for the deployment to manage replicas. But that extra label wouldn't be necessary for resources other than Deployment.
(i.e. do I always have to list those opinions)
The list of opinions is on the SpringBootContainer .status
, you wouldn't specify that as a user, the reconciler is reporting what it did. It's also present on SpringBootApplication today.
how I override an opinion that someone else has opted to apply (e.g. central IT).
Same way as you do today. Set the value you want directly on the PodTemplateSpec and the opinions should back off. The difference is that the PodTemplateSpec where you define those settings has moved from SpringBootApplication to "Deployment".
@wilkinsona thanks for the feedback. These opinions are still quite naive and untested. We can codify the boot team's opinions. So far we're trying to prove that we can have and inject meaningful opinions; and not yet that those opinions are actually correct.
Let's find a time next week to flesh out more robust opinions.
OK, I'd missed the bit of those being on the status
and not the spec
.
I would still also like to be able the list the opinions I am interested in via a spec (not just the status). Especially if I have an image without the BOM metadata.
Spiked this idea over in https://github.com/projectriff/bindings/pull/16. In short, it's an abomination that should not be adopted.
The fundamental issue is that we're updating our source of truth. Because the opinions change based on the content of the image and the existing resource, the previously applied opinions become part of the input to the new opinions. This would effectively break the demo where we upgrade from boot 2.2 to 2.3 and see the new capabilities come into play. The previously applied opinions will be locked in place. This is ok'ish for upgrading boot 2.2 -> 2.3, but would completely break downgrading from 2.3 -> 2.2 as the probe endpoints wouldn't exist putting the pod into a crash loop.
It might be possible to work around this by cacheing the original state of the resource when binding and then doing a three-way diff for all updates to the resource. I don't have much faith in the ability to safely do a diff without ever hitting a conflict. Moreover, there are crucial semantics in the diff based on object and not just individual fields. For example, a probe is either defined or not depending on whether the field is nil or an object. Within the object individual fields are either set or left to the default value. Distinguishing between object semantics and field semantics will require a semantically aware diff (which doesn't exist, and largely depends on the semantics of the opinions).
A workaround for this issue was to target the resource created by the parent resource rather than the user defined resource directly. For example, if a user defines a Deployment, k8s will create ReplicaSets and in turn Pods. If we target the ReplicaSet we can apply the opinions while preserving the original state. This fails because the Deployment controller will stamp out a new ReplicaSet when it sees the desired state drift. This has the effect of creating a flood a of new replicasets for the deployment (about once per minute, or more). Each new replica set will create new pods, effectively restarting the application every minute.
The SpringBootApplication resource encapsulates out knowledge and opinions about running a Spring Boot application as a container on k8s. It does this by hosting a PodTemplateSpec on its spec and creating a Deployment with the original PodTemplateSpec injected with opinions. This has a few issues:
We can invert the relationship between the SpringBootApplication and a "Deployment", and still be able to inject the same opinions. Introducing the SpringBootContainer:
Deployment before injection:
Deployment after injection:
application properties ConfigMap:
By the SpringBootContainer defining a reference to the Deployment, we can interact with any resource the hosts a PodTemplateSpec, while providing the same high level experience. The downside is that users will need to specify the apiVersion, kind and name of the resource they want to target. In the future we might be able to provide a more cross-cutting mechanism to apply opinions.
To implement this behavior, we need to switch to the Knative Binding reconcile framework. This is only a compile time dependency within the controller, not a dependency on Knative at runtime. Unfortunately, it is incompatible with the controller-runtime foundation we currently use, so this is an invasive change.