NTHv2 core functionality #612
Conversation
* Add SQS parameters to Terminator spec * Add drain parameters to Terminator spec * Fetch messages from SQS queue * Cordon and drain nodes targeted in SQS messages
src/charts/aws-node-termination-handler-2/templates/node.k8s.aws_terminators.yaml
Outdated
Show resolved
Hide resolved
src/charts/aws-node-termination-handler-2/templates/node.k8s.aws_terminators.yaml
Outdated
Show resolved
Hide resolved
* "Url" to "URL" * "Aws" to "AWS" * "Asg" to "ASG" * "Ec2" to "EC2" * "Sqs" to "SQS"
* "Id" to "ID" * "Arn" to "ARN"
There was a problem hiding this comment.
I feel like the preemptive versioning in the event pkg (v0, v1, v2) is unnecessary and makes the project structure more confusing. My understanding is if we didn't include these versions now, then when/IF we ever release a v2 then we would need to spend additional time to update module versions (potentially logic). Assuming this was the intent, then I don't think the cost is worth the potential savings.
I think it'd be clearer to remove versions altogether. Ex: pkg/event/rebalancerecommendation/event.go
Lmk if this assumption is wrong or if I'm missing context
| kubectlcordondrainer.DefaultCordoner, | ||
| kubectlcordondrainer.DefaultDrainer, | ||
| eventParser := event.NewAggregatedParser( | ||
| asgterminateeventv1.Parser{ASGLifecycleActionCompleter: asgClient}, |
There was a problem hiding this comment.
what do the versions (v0, v1, v2) correspond to? Is it necessary to have both asgterminateeventv1 and asgterminateeventv2?
There was a problem hiding this comment.
It corresponds to the value of the "version" field from the EventBridge message (stored in AWSMetadata.Version). Each event type has a versioned schema defined in EventBridge, e.g. here.
There was a problem hiding this comment.
Ahh gotcha, I feel like I'm getting lost in the versions here: CRD/API version, app version, module/pkg version, and now EventBridge schema version.
With that said, I'm still leaning towards removing the version (v0, v1) from the project directories/file paths. I don't think the schema version needs to be exposed in the project, but instead handled within an event's Parse logic. For example with asgterminate event, Parse would try to unmarshal into v1 struct first.. if that fails try v2.
- Another idea (related to my other comment) is a "super" struct
AsgTerminatethat is a superset of all fields, unmarshal into that, and then check field(s) for version.
Last concern with breaking out by schema version is if v3 is released tomorrow with very minimal changes, then we need to create a new directory and set of files that would be mostly copy/paste which I don't think is necessary.
There was a problem hiding this comment.
if v3 is released tomorrow with very minimal changes [emphasis mine]
It is also possible that new versions would include new and important data fields, or require new API calls.
I think the question is: what should NTH do if it encounters a newer version number of a known event type?
- Ignore it
- This is the currently implemented behavior; an error is logged
- Handle it the same as the previous version
- Should probably log a warning about the unknown version in case there is a failure later
There was a problem hiding this comment.
I think the question is: what should NTH do if it encounters a newer version number of a known event type?
- Ignore it
This is the currently implemented behavior; an error is logged- Handle it the same as the previous version
Should probably log a warning about the unknown version in case there is a failure later
I'd go with number 2 and make it a best effort to handle it (or maybe configurable?). However, I think regardless of how we implement unknown version handling we don't need to expose the version at the package level.
* "awsevent.go" to "unmarshal.go" * "event.go" to "handler.go"
Issue #, if available:
#556 Initial NTH v2 design proposal
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.