Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[operator] Readiness for Paladins and SCD Mappings #402

Merged

Conversation

onelapahead
Copy link
Contributor

@onelapahead onelapahead commented Nov 7, 2024

Description

Adds a readiness check to the PaladinReconciler based on if the underlying StatefulSet is ready or not, and a controller ref to allow for watching relevant StatefulSet updates.

The SmartContractDeploymentReconciler is then updated to only reconcile a Paladin if the node is Ready and if the contracts refer to it.

Renames the Completed phase to Ready, as a Paladin can now go back to Pending if the StatefulSet every re-reconciles due to config changes.

Results

One set of enhancements which seemed to help SCD reconcile speed decently in efforts to address to #397, but contracts still get stuck even after the Paladin goes back to Ready after a restart, so missing a retry or a readiness check in the right place. And ideally we avoid too many pod restarts at once, but will address that in a follow up PR.

I did have other contracts stuck due to encoding issues I believe will be addressed by #401.

@onelapahead onelapahead marked this pull request as draft November 7, 2024 03:08
Signed-off-by: hfuss <[email protected]>
@onelapahead onelapahead marked this pull request as ready for review November 7, 2024 03:50
Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good step forwards on all fronts, but as we've discussed it's probably not quite the end of the journey to faster deploy times.

I note we don't have service-level readiness checks on our pod defs right now, which might be good to add (although I don't know if we can make them part of the checking process for the SCD/TXInvoke paths)

StatusPhaseFailed StatusPhase = "Failed"
StatusPhaseUnknown StatusPhase = "Unknown"
StatusPhasePending StatusPhase = "Pending"
StatusPhaseReady StatusPhase = "Ready"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this is a lot clearer 👍

@@ -212,12 +214,38 @@ func getContractDeploymentAddress(ctx context.Context, c client.Client, name, na

}

// TODO this is duplicated btwn here and SmartContractDeploymentReconciler, as reconcileAll alludes to, generics
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have some helpers to allow generics. Found it surprisingly hard to support them in k8s data structures, I had to introduce a thing like this to every file:

// allows generic functions by giving a mapping between the types and interfaces for the CR
var TransactionInvokeCRMap = CRMap[corev1alpha1.TransactionInvoke, *corev1alpha1.TransactionInvoke, *corev1alpha1.TransactionInvokeList]{
NewList: func() *corev1alpha1.TransactionInvokeList { return new(corev1alpha1.TransactionInvokeList) },
ItemsFor: func(list *corev1alpha1.TransactionInvokeList) []corev1alpha1.TransactionInvoke {
return list.Items
},
AsObject: func(item *corev1alpha1.TransactionInvoke) *corev1alpha1.TransactionInvoke { return item },
}

@peterbroadhurst peterbroadhurst merged commit e2f35df into LF-Decentralized-Trust-labs:main Nov 7, 2024
3 checks passed
@onelapahead onelapahead deleted the issue-397 branch November 10, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants