-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[ML][Fleet] Update Transform installation mechanism to support upgrade paths #142920
[ML][Fleet] Update Transform installation mechanism to support upgrade paths #142920
Conversation
1fe8c56
to
a637796
Compare
…g aliases when first installed
bf16496
to
8e2a6ff
Compare
// @TODO: Remove ignore 400 | ||
await esClient.ingest.deletePipeline({ id }, { ignore: [400] }); |
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 think the way to remove the 400 is to set the index.default_pipeline
setting on the old destination index to the empty string before deleting the pipeline. See elastic/elasticsearch#32286 (comment).
These changes go with those of elastic/kibana#142920. As we formalize the process by which the Fleet package installer will upgrade transforms more operations are required for managing the transforms and the related destination index: 1. Need to be able to add an alias on the transform destination index and adjust which indices it points to when upgrading the transform. 2. Need to be able to remove a default ingest pipeline from the settings of an old transform destination index during an upgrade that deletes the ingest pipeline.
These changes go with those of elastic/kibana#142920. As we formalize the process by which the Fleet package installer will upgrade transforms more operations are required for managing the transforms and the related destination index: 1. Need to be able to add an alias on the transform destination index and adjust which indices it points to when upgrading the transform. 2. Need to be able to remove a default ingest pipeline from the settings of an old transform destination index during an upgrade that deletes the ingest pipeline.
…tes-being-overriden
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.
Core related changes LGTM
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 stepped through all the code/tests (much appreciated on how many tests we've got for this process!) and everything looks good to me overall. I don't have a great way to test this on my end, though, so I'll defer to @kevinlog who I know was testing the Endpoint transforms previously.
I just had a few questions about whether we should be bubbling up granular errors for specific transform failures during installation - but I wouldn't consider those blocking questions here.
} catch (err) { | ||
logger.debug( |
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 this be logged to error
or is this a non-blocking issue?
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 have this same question about making errors visible to the user. It's generally a lot easier to debug with error messages than without!
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.
Updated here e1a0050
(#142920)
logger.debug(`Deleted alias: '${alias}' matching indices ${indicesMatchingAlias}`); | ||
} | ||
} catch (err) { | ||
logger.debug(`Error deleting alias: ${alias}`); |
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.
Same here re: log levels.
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.
Updated here e1a0050
(#142920)
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.
LGTM
@qn895 thank you for preparing this PR! I checked it out and tried it. See some observations below.
|
Changes to support elastic/kibana#142920. Transform destination indices now have a version number appended to their names, so the patterns used to set up the index privileges need to be adjusted accordingly.
…old schema to new
…tes-being-overriden
Thanks Kevin for the thorough testing. I've updated the behavior for
|
@qn895 thank you for the update! I checked it out and tried it again and it works! I addition to the other testing scenarios, the below one now works.
LGTM! |
@elasticmachine merge upstream |
Changes to support elastic/kibana#142920. Transform destination indices now have a version number appended to their names, so the patterns used to set up the index privileges need to be adjusted accordingly.
💚 Build Succeeded
Metrics [docs]Page load bundle
Saved Objects .kibana field count
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @qn895 |
Thanks again for this 🙏 ☀️ 🎉 |
+1 |
Summary
Follow up of #140046. This PR:
Adds index aliases to the destination index for Transforms:
{{package-version}}
name to thedestination_index_name
specified in transform.yml{destination_index_name}.all
that points to all the destination indices from all the previous versions and new version{destination_index_name}.latest
that points to just the destination index of the new versionUpgrading package to a new version no longer deletes the destination index
Downgrading package to an older version (e.g. from v3 to v1) will:
{destination_index_name}.latest
to point to destination index v1.{destination_index_name}.all
and{destination_index_name}.latest
alias.Support installing transforms concurrently and sequentially.
order
is specified in thetransform.yml
's_meta
section, and all the numerical order are unique, transforms will be created and started sequentially. If not, they will be created and started concurrently.Support versioning of transforms. If
fleet_transform_version
is specified intransform.yml
's_meta
section:fleet_transform_version
changed (either incremented or decremented): delete old transform, keep the old destination index, install new index templates, component templates, and transformfleet_transform_version
remains the same: keep old transform, keep the old destination index, do nothing newFixes an issue with the mappings and template not being applied to the destination index correctly when the destination index has an ingest pipeline. Previously, when the transform is associated with an ingest pipeline, we add the ingest pipeline to the settings when calling
PUT index/{transform-destination-index}
. This in turns makes the settings and mappings from the component templates not apply correctly to the destination. This PR changes so that it will add the pipeline to the component template.Technical changes:
ElasticsearchAssetType
forindex
version
forPACKAGES_SAVED_OBJECT_TYPE
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers