ClusterPool-Inventory enhancement Status.Inventory

[abraverm]

x-ref: https://issues.redhat.com/browse/HIVE-1367

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abraverm
To complete the pull request process, please assign akhil-rane after the PR has been reviewed.
You can assign the PR to them by writing /assign @akhil-rane in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[akhil-rane]

I still feel this is a bit of stretch storing status in the cluster.status.inventory. Let's list down a couple of problems that culminated in this design

1. Openstack PSI installations do not succeed at first but require repeated attempts to succeed

• I think we should mark ClusterDeploymentCustomization as Invalid only when json patch process fails. If the cluster installation fails after patching, we can maintain field called LastFailedApplyTime (Needs a better name) on customization and if the last install with this customization recently failed clusterpool controller should move on to the next customization. This way we are not stuck failing on single customization for a long time. The LastFailedAppyTime should be cleared when we update the customization.

2. It might happen that for a specific clusterpool with its custom install config template, the customization will never work on the cloud - in short a bad patch

• Should be a no-op on the customization side and the user should be able to figure out the issue from the clusterdeployment condition. This is practically an issue with the install config. The clusterpool controller will ignore the customization till cooloff period is reached (calculated from LastFailedApplyTime) and try again.

3. What if inventory resource is missing

• clusterdeployment should use default install config and add a status condition to the clusterpool resource.

@2uasimojo Any inputs from you here?

[akhil-rane]

This statement looks ambiguous. Can we have something that mentions these fields are on clusterpool.status?

[akhil-rane]

The status in the clusterpool.status.inventory is checked before doing GET to fetch ClusterDeploymentCustomization

[akhil-rane]

Suggested change

	- `Version` - Inventory resoruce last seen version.
	- `Version` - Inventory resource last seen version.

[akhil-rane]

Suggested change

	Add a field to `ClusterPool.Stauts` called `Inventory`.
	Add a field to `ClusterPool.Status` called `Inventory`.

[akhil-rane]

Suggested change

	- Created ClusterDeployment `spec.ClusterPoolReference.ClusterDeploymentCustomizationRef` with reference to the inventory resource.
	- Created ClusterDeployment will have field`spec.ClusterPoolReference.ClusterDeploymentCustomizationRef` populated with reference to the inventory resource.

[akhil-rane]

We also need to mention that clusterpool.status.inventory will be updated with a right status

[abraverm]

I still feel this is a bit of stretch storing status in the cluster.status.inventory

I think that having a custom resource for cluster deployment customization is the right thing to do. The status need to be stored somewhere. So the only alternative I can think of is having another custom resource and controller that will handle this process.

3. What if inventory resource is missing

• clusterdeployment should use default install config and add a status condition to the clusterpool resource.

The entire inventory or just a specific resource is missing? If inventory is defined, then install config must be customized because the user expects that the default install config is not good without customization.

[2uasimojo]

@2uasimojo Any inputs from you here?

My initial thought is that, while I like the ideas behind these changes, I would like to see them adjusted to conform better to a) k8s best practices, and b) design principles elsewhere in the hive project. For example:

• The k8s-ish way to handle To Be Deleted is with deletionTimestamp (set when the resource is Delete()ed) and one or more finalizers to hold it until the relevant cleanup has been done.
• Rather than mirroring the ClusterPool.spec.inventory list in ClusterPool.status.inventory, I would like to see most of that status information held within each individual ClusterDeploymentCustomization.status instead.
• The only thing ⬆️ doesn't work for is Missing. For this we should use a (new) ClusterPool status condition, something like InventoryHealthy. In a spirit similar to the way the ClusterDeployment Ready status condition lists machines that are not yet running in the Message, InventoryHealthy could list the inventory entries that don't exist. It could also list (or just count) the entries that are broken. Or we could have two separate conditions, AllInventoryPresent and InventoryHealthy.
• Keeping track of pending updates for To Be Updated sounds really hard. We should do one of two things here:
  • Disallow updates to in-use ClusterDeploymentCustomizations via webhook.
  • Do nothing special -- simply allow the updates. If the CDC is in use, we will have already applied it to the CD, so the updates won't have an effect until that CD is deleted and the CDC gets pulled in for the next one. I get that this makes it tough to correlate and debug, so I would actually be in favor of the webhook.
• Attempts: I don't see where the initial number of attempts is set. Is it configurable? If we're going to do this, I would prefer it to gel with existing hive concepts, such as ClusterDeployment.spec.installAttemptsLimit <=> ClusterDeployment.status.installRestarts. a) This is a count up rather than a count down; and b) there's a .spec field to initialize/customize the retry count. So in this case something like ClusterDeploymentCustomization.spec.applyAttemptsLimit and ClusterDeploymentCustomization.status.applyAttempts. If spec.applyAttemptsLimit is unset, we try forever; if set, we only try up to that many times before declaring the CDC broken. However, there would be some devils in the details for this. How do we tell that the CDC has been updated so we can reset status.applyAttempts? I don't think resourceVersion works, since that gets updated any time anything changes, including labels/annotations and status. Instead I think we have to store an annotation with a hash of .spec. I think something like this is worth doing, though; otherwise we could end up selecting the same busted CDC over and over again and never getting a healthy CD out of it. With the way the pool tries to refill itself, this could lead to starvation. (We've seen a similar problem since checking cluster operator health and only claiming running clusters.)

[abraverm]

I'm starting from the end because it helps me build my claims:

• Attempts: I don't see where the initial number of attempts is set. Is it configurable?

Yes, I forgot to mention this in the enhancment, will do.

If we're going to do this, I would prefer it to gel with existing hive concepts, such as ClusterDeployment.spec.installAttemptsLimit <=> ClusterDeployment.status.installRestarts. a) This is a count up rather than a count down; and b) there's a .spec field to initialize/customize the retry count. So in this case something like ClusterDeploymentCustomization.spec.applyAttemptsLimit and ClusterDeploymentCustomization.status.applyAttempts. If spec.applyAttemptsLimit is unset, we try forever; if set, we only try up to that many times before declaring the CDC broken.

Sounds good to me

However, there would be some devils in the details for this. How do we tell that the CDC has been updated so we can reset status.applyAttempts? I don't think resourceVersion works, since that gets updated any time anything changes, including labels/annotations and status. Instead I think we have to store an annotation with a hash of .spec.

That what I think I did in the implementation (hash calculation of spec), see #1672. But the version is monitored by ClusterPool. Here I'm struggling with the request to move most of the monitoring to CDC. Version must be in ClusterPool, because user can delete and create the same CDC, in that case the attemtps will reset but the version is technically the same.

I think something like this is worth doing, though; otherwise we could end up selecting the same busted CDC over and over again and never getting a healthy CD out of it. With the way the pool tries to refill itself, this could lead to starvation. (We've seen a similar problem since checking cluster operator health and only claiming running clusters.)

I liked @akhil-rane suggestions on using LastFailedApplyTime for cool off period.

• Keeping track of pending updates for To Be Updated sounds really hard. We should do one of two things here:
  • Disallow updates to in-use ClusterDeploymentCustomizations via webhook.

This option is not user friendly as one will be in a weird position of trying to catch a window to update CDC, or going the overhead of disabling in all CPs first.

• Do nothing special -- simply allow the updates. If the CDC is in use, we will have already applied it to the CD, so the updates won't have an effect until that CD is deleted and the CDC gets pulled in for the next one. I get that this makes it tough to correlate and debug, so I would actually be in favor of the webhook.

"this makes it tough to correlate and debug" - I agree but I believe that UX is more important and I think that this option corrolates better with "Operator" idea of reconciling.
There for I have added the CD reference in the CP inventory status.

-The only thing arrow_up doesn't work for is Missing. For this we should use a (new) ClusterPool status condition, something like InventoryHealthy. In a spirit similar to the way the ClusterDeployment Ready status condition lists machines that are not yet running in the Message, InventoryHealthy could list the inventory entries that don't exist. It could also list (or just count) the entries that are broken. Or we could have two separate conditions, AllInventoryPresent and InventoryHealthy.

I'm struggling with the idea of InventoryHealthy. If CP needs a CDC, then it can't just pick "Available" CDC, it also has to check if it is already marked as "Broken" and other non "Reserved" statuses. I don't see why should "Broken" be in CDC status as it is relevant to a specific CP, so IIUC it will be listed in InventoryHealthy. Condition message is a string, so it means it will become a stringified data stracture, and here I'm lost on how is it better than having it in CP inventory status.

• Rather than mirroring the ClusterPool.spec.inventory list in ClusterPool.status.inventory, I would like to see most of that status information held within each individual ClusterDeploymentCustomization.status instead

I partially agree on this point. I don't like duplication of data like the state of Available and reference to a CD, but CDC is a shared resource that only one CP can use it at a time and I want the user to be able to update it whenever wishes for UX, these two make it a volotule source of truth. I think that the cost of additional verification and data duplication worth the UX. Also see my previous comment about InventoryHealthy.

• The k8s-ish way to handle To Be Deleted is with deletionTimestamp (set when the resource is Delete()ed) and one or more finalizers to hold it until the relevant cleanup has been done.

Agree

[abraverm]

https://coreos.slack.com/archives/CE3ETN3J8/p1645814399944789

[2uasimojo]

@abraverm Do you intend to open a new PR with the design changes we discussed in slack?

[abraverm]

Yes, this PR design is fundamentally different from the one discussed in slack.