Add resource to manage ML datafeed state #1422
Conversation
nick-benoit
left a comment
nick-benoit
left a comment
There was a problem hiding this comment.
Nice. Having the resource to manage the state is a cool idea. Just a few optional questions / thoughts.
| "github.com/hashicorp/terraform-plugin-testing/helper/resource" | ||
| "github.com/hashicorp/terraform-plugin-testing/terraform" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Is there a good way to add test coverage for datafeed_timeout and timeouts? I guess since they aren't written to any particular resource we read back from ES it might be a bit tricky. It would be nice to have some test coverage though.
There was a problem hiding this comment.
Yea, good question. I've added a test to verify the timeouts on the job state resource, but I can't think of a way to do the same on the datafeed (i.e artificially block the datafeed from starting).
ES doesn't return the size as provided, so ensure 1024mb = 1024m = 1g etc
* origin/main: First take at documented coding standards (#1429) chore(deps): update golangci/golangci-lint-action action to v9 (#1431) chore(deps): update golang:1.25.4 docker digest to 6ca9eb0 (#1432) chore(deps): update kibana-openapi-spec digest to 96ffcd7 (#1430) chore(deps): update kibana-openapi-spec digest to a8e3b64 (#1428) chore(deps): update golang docker tag to v1.25.4 (#1423)
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new resource elasticstack_elasticsearch_ml_datafeed_state to manage the operational state (started/stopped) of existing Elasticsearch ML datafeeds. The implementation includes comprehensive testing, timeout handling, and a custom memory size type for improved ML job configuration.
Key changes:
- Added new
elasticstack_elasticsearch_ml_datafeed_stateresource for managing datafeed states - Introduced custom
MemorySizetype with semantic equality checking for ML memory limits - Enhanced timeout handling with descriptive error messages for ML operations
- Refactored datafeed state utilities for reusability
Reviewed Changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| provider/plugin_framework.go | Registered new datafeed state resource |
| internal/elasticsearch/ml/datafeed_state/*.go | Core implementation of datafeed state resource including CRUD operations |
| internal/utils/customtypes/memory_size*.go | New custom type for memory size values with validation and semantic equality |
| internal/models/ml.go | Added datafeed timing stats and search interval models, moved ML job models |
| internal/elasticsearch/ml/datafeed/*.go | Refactored state utilities to be reusable by datafeed state resource |
| internal/elasticsearch/ml/job_state/*.go | Enhanced timeout error messages and refactored state transition logic |
| internal/elasticsearch/ml/anomaly_detection_job/*.go | Updated to use new MemorySize custom type |
| docker-compose.yml, .github/workflows/test.yml | Configured ML memory limits for testing |
| examples/, docs/ | Added documentation and examples for new resource |
|
|
||
| if datafeed.State(datafeedStats.State) == datafeed.StateStarted { | ||
| if datafeedStats.RunningState == nil { | ||
| diags.AddWarning("Running state was empty for a started datafeed", "The Elasticsearch API returned an empty running state for a Datafeed which was successfully started. Ignoring start and end response values.") |
There was a problem hiding this comment.
After checking if RunningState is nil and adding a warning, the code continues to dereference datafeedStats.RunningState on lines 64 and 68 without returning early. This will cause a nil pointer panic. Add an early return after the warning or wrap the subsequent code in an else block.
| diags.AddWarning("Running state was empty for a started datafeed", "The Elasticsearch API returned an empty running state for a Datafeed which was successfully started. Ignoring start and end response values.") | |
| diags.AddWarning("Running state was empty for a started datafeed", "The Elasticsearch API returned an empty running state for a Datafeed which was successfully started. Ignoring start and end response values.") | |
| return &data, diags |
| Node *DatafeedNode `json:"node,omitempty"` | ||
| AssignmentExplanation *string `json:"assignment_explanation,omitempty"` | ||
| RunningState *DatafeedRunning `json:"running_state,omitempty"` | ||
| TimingStats *DatafeedTimingStats `json:"timing_stats"` |
There was a problem hiding this comment.
The TimingStats field is marked as a pointer but doesn't have omitempty in the JSON tag. This is inconsistent with other pointer fields in the struct (like Node, AssignmentExplanation, RunningState) which all have omitempty. This could cause issues when marshaling/unmarshaling JSON if the field is nil. Add omitempty to the JSON tag.
| TimingStats *DatafeedTimingStats `json:"timing_stats"` | |
| TimingStats *DatafeedTimingStats `json:"timing_stats,omitempty"` |
Fixes #813