isMonitorVPAReady fails if VPA has recommendations for more containers than defined in a "Tortoise"

[Dymanik]

Tortoise should check if containerInVPA is a SuperSet of containerInTortoise.

tortoise/pkg/vpa/service.go

Line 169 in eb8e267

return containerInTortoise.Equal(containerInVPA)

Currently if containers are injected to the pod, Tortoise never sets the VPA as ready as both sets are different, even though VPA has recommendations for all Tortoise defined containers.

[sanposhiho]

if containers are injected to the pod, Tortoise never sets the VPA as ready as both sets are different

VPA should include the recommendation for the injected container, right?

[Dymanik]

Yes, in our case, VPA includes recommendations for istio-proxy container. But in the tortoise definition we can't have istio-proxy because it's not on the deployment.

And additionally, we don't want to scale the istio-proxy container. We want Tortoise to care only about our applications' container.

[sanposhiho]

But in the tortoise definition we can't have istio-proxy because it's not on the deployment.

Do you use "sidecar.istio.io/inject" annotation for the injection? Then, tortoise should allow to have istio-proxy even though the istio-proxy container isn't in the deployment manifest.

And additionally, we don't want to scale the istio-proxy container. We want Tortoise to care only about our applications' container.

You can explicitly disable the autoscaling for the istio container via Off autoscaling type.
https://github.com/mercari/tortoise/blob/main/api/v1beta3/tortoise_types.go#L145

[Dymanik]

Do you use "sidecar.istio.io/inject" annotation for the injection? Then, tortoise should allow to have istio-proxy even though the istio-proxy container isn't in the deployment manifest.

We annotate the Namespace, not the Deployment, nor the Pods.

You can explicitly disable the autoscaling for the istio container via Off autoscaling type. https://github.com/mercari/tortoise/blob/main/api/v1beta3/tortoise_types.go#L145

I'm aware of that, but the ValidationWebhook rejects the Tortoise definition because the Deployment doesn't have a istio-proxy container defined. More specifically, new Tortoises with defined istio-proxy are rejected.

We can create a new one without including istio-proxy, and it will never get ready. the we can modify existing Tortoises to include istio-proxy with autoscaling Off, and it immeadiately gets ready.

[sanposhiho]

We annotate the Namespace, not the Deployment, nor the Pods.

I see.. So, we have to check Namespace too here -
https://github.com/mercari/tortoise/blob/main/api/v1beta3/tortoise_webhook.go#L195

That's the core issue we have to solve.

We can create a new one without including istio-proxy, and it will never get ready. the we can modify existing Tortoises to include istio-proxy with autoscaling Off, and it immeadiately gets ready.

Yeah, that is the workaround for now. We don't validate the containers that the deployment has vs the ones that the tortoise has in an update validation.

[Dymanik]

Yeah, that is the workaround for now. We don't validate the containers that the deployment has vs the ones that the tortoise has in an update validation.

I see.
What I propose that we use superset/subset for comparison between the Tortoise's container set , and VPA container recommendation set, instead of Equals. Checking what the comment says, that all the containers in tortoise have recommendations in the VPA (the VPA can have extra ones added by other mutationwebhooks not just istio). That change will resolve getting stuck as "VPA not ready" even when it has all the recommendations ready.

And I wouldn't have to define a container in Tortoise to set it to Off anyways, and leverage the default behavior defined in the user manual

If policies are defined for some but not all containers or resources, Tortoise will assign a default Off policy to unspecified resources.

[Dymanik]

The Validation webhook check, I see it as a separate Issue, and I believe there is already some work to check the Pods and not the Deployments for that, right?

[sanposhiho]

Okay, +1 to change isMonitorVPAReady to see if containerInVPA is a super set (not equal) of containerInTortoise.
Would you mind sending the PR? That's the easiest way getting it done. (review wise)

The Validation webhook check, I see it as a separate Issue, and I believe there is already some work to check the Pods and not the Deployments for that, right?

Yup, we can regard it as a separate issue.
So, ↓ is the validation webhook for the tortoise, checking the deployment targetted by the tortoise and see if tortoise has useless policy (= policy for non-existing containers) or not.
https://github.com/mercari/tortoise/blob/main/api/v1beta3/tortoise_webhook.go#L195
And, we know the container could be injected, commonly by istio, and then only check the deployment's annotation. But, we should additionally check the namespace. (I didn't know the namespace can have "sidecar.istio.io/inject" annotation)

[Dymanik]

PR sent #415