search

syself / cluster-api-provider-hetzner

Cluster API Provider Hetzner 🚀 Kubernetes Infrastructure as Software 🔧 Terraform/Kubespray/kOps alternative for running Kubernetes on Hetzner
https://caph.syself.com
Apache License 2.0
539 stars 51 forks source link

HetznerBareMetalHost status is part of spec #1333

Open simonostendorf opened 3 weeks ago

simonostendorf commented 3 weeks ago

/kind bug

What steps did you take and what happened:

If you describe a HetznerBareMetalHost you can see, that the .status fields are placed under .spec.status.

What did you expect to happen:

Placing status as .status and not as .spec.status is the "normal" way. Are there any reasons why this is placed under .spec.status here?

Anything else you would like to add:

You can see the same structure in the code here.

Environment:

janiskemper commented 3 weeks ago

Yes there is. The status in Kubernetes is something that can be lost and the reproduced. What you see in spec.status cannot be reproduced in some parts and therefore cannot be put into the Kubernetes status, at least parts.

This is why we had the options of putting it under spec (what we did) or in the annotations in some compact json string. We decided to put it in the spec because it is more stable than the annotations that have a size limit.

If you have another perspective how to achieve this (to not lose this data because we cannot just re-create it in any reconcile loop), I'm open for it!

simonostendorf commented 3 weeks ago

Thank you for your quick and detailed reply @janiskemper!

I think the decision is understandable and I have no further perspective or questions.

The whole decision (or this issue) could be written/referenced as code comment to avoid future questions and to document the decision process.

janiskemper commented 3 weeks ago

makes sense! will do!