-
Notifications
You must be signed in to change notification settings - Fork 54
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
core/services/ccipcapability: add the ccip capability package #1024
Conversation
* add an implementation for the job delegate * ocr instance launcher and blue/green deployment
6e2c9e9
to
26e370d
Compare
LatestState() (RegistryState, error) | ||
} | ||
|
||
type ChainConfig interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is different on the contract, it includes ChainSelector as well.
ds sqlutil.DataSource | ||
peerWrapper *ocrcommon.SingletonPeerWrapper | ||
|
||
isNewlyCreatedJob bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you imagine this is going to be used for? It's currently passed into the oracle creator which effectively makes it a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't spent enough time concentrating on the capabilities docs, so I'm a bit out of depth. But here are some things I noticed. Overall, it seems fairly sensible.
// blueGreenDeployment represents a blue-green deployment of OCR instances. | ||
type blueGreenDeployment struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are capabilities actually deployed? Seems weird to have this built into the node.
Currently it seems like this only manages switching between configs. It might be nice to switch between LOOPPs as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually it'll switch between LOOPPs, that LOOPP or not factor is hidden behind the OracleCreator interface.
Capabilities are created on the capability registry, DONs that support a capability are also created there, so the capability registry is the source of truth on that.
@@ -50,6 +50,7 @@ const ( | |||
Webhook Type = (Type)(pipeline.WebhookJobType) | |||
Workflow Type = (Type)(pipeline.WorkflowJobType) | |||
StandardCapabilities Type = (Type)(pipeline.StandardCapabilitiesJobType) | |||
CCIP Type = (Type)(pipeline.CCIPJobType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that someone was trying to alphabetize these, maybe move this to line 39. Same with the flags below.
} | ||
|
||
func (d *Delegate) JobType() job.Type { | ||
return job.CCIP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if the single job
for both plugins is going to lead to any kind of limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will find out ;-)
|
||
func (d *Delegate) BeforeJobCreated(job.Job) { | ||
// This is only called first time the job is created | ||
d.isNewlyCreatedJob = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems weird (maybe after it starts being used it becomes clear though): for a newly created delegate newJob = false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is just standard chainlink job stuff, other jobs do this as well
} | ||
} | ||
|
||
func (l *launcher) tick() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe rename tick()
to something more meaningful about what happens inside the func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't come up with a good name :-) please suggest
// 1. Create a new oracle (the green instance) and start it. | ||
// 2. Shut down the blue instance, making the green instance the new blue instance. | ||
func (l *launcher) updateDON(don keystone_capability_registry.CapabilityRegistryDONInfo) error { | ||
if !isMemberOfDON(don, l.p2pID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also check if we are running instances for this DON
Motivation
Solution
Architecture