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

ec_deployment: add support for data tiers #253

Merged
merged 11 commits into from
Mar 11, 2021

Conversation

marclop
Copy link
Contributor

@marclop marclop commented Mar 8, 2021

Description

I've split the pull request in individual commits which should make it
easier to review.

Adds support for node_roles and automatic migration from node_type_*
to node_roles. I will note a few things to make it easier to break
down all of these changes.

f972aa8:
Adds node_roles support by updating the SDK to the latest available
version, this makes the deployment template not work out of the box and
instead our client has to choose whether to use the node_type object
or the node_roles array.
Stack versions >= 7.10.0 can and should use node_roles since that
allows users to leverage the cold tier and autoscaling in the future
as well.
With the introduction of the "id" field, the topology elements needn't
use instance_configuration_id as the unique field, instead, id is
that unique field value.
The instance_configuration_id has been set as computed only and
cannot be set by the user anymore. For now, this change is present on
the Elasticsearch topology only, in the future, as other applications
might also auto scale, the API will change accordingly.
Last, it validates that a set topology element has a size > 0, otherwise
an error will be thrown.

99dfe21:
Another somewhat breaking change is the removal of the version computed
property in the Elasticsearch object. This field is completely redundant
and should not be used, instead the global version declares the version
for all of the deployment's applications. Since we're removing a field
from the schema and previous versions of the provider have saved it in
the state, we're adding a StateUpgradeFunc and bumping the current
schema version to 1. The schema upgrade function will only be run when
the saved state version is 0 and will only run once, then the state
schema version will be bumped to 1.

The rest of commits are updates to the tests, fixtures, documentation
and example files.

Related Issues

Closes #212

How Has This Been Tested?

Thoroughly unit and acceptance tested, additionally I've tested the same scenarios as the acceptance tests manually.

Types of Changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation

Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
@marclop marclop added enhancement New feature or request Team:Delivery labels Mar 8, 2021
@marclop marclop self-assigned this Mar 8, 2021
@marclop marclop requested review from alaudazzi, nrichers and a team as code owners March 8, 2021 16:52
Copy link
Contributor

@alaudazzi alaudazzi left a comment

Choose a reason for hiding this comment

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

Left a few editing suggestions. LGTM for the documentation part.

docs/resources/ec_deployment.md Outdated Show resolved Hide resolved
docs/resources/ec_deployment.md Outdated Show resolved Hide resolved
docs/resources/ec_deployment.md Outdated Show resolved Hide resolved
docs/resources/ec_deployment.md Outdated Show resolved Hide resolved
marclop and others added 2 commits March 9, 2021 08:54
Signed-off-by: Marc Lopez <marc5.12@outlook.com>
Co-authored-by: alaudazzi <46651782+alaudazzi@users.noreply.github.com>
Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

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

This is looking great @marclop 🎉 ! Just wondering if this PR should include an update to the ec_deployment data source as well 🤔 shouldn't be too big of a change I think? Otherwise I guess that should be tackled as a follow up

t.Helper()
requiresAPIConn(t)

deploymentTpl := setDefaultTemplate(region, depTpl)
esIC, kibanaIC, apmIC, essIC, err := setInstanceConfigurations(deploymentTpl)
// esIC is no longer needed
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// esIC is no longer needed
// esIC is no longer needed after the implementation of node roles

resource.TestCheckResourceAttr(ccsResName, "elasticsearch.0.topology.0.node_type_master", ""),
resource.TestCheckResourceAttr(ccsResName, "elasticsearch.0.topology.0.node_type_ml", ""),
resource.TestCheckResourceAttr(ccsResName, "elasticsearch.0.topology.0.id", "hot_content"),
resource.TestCheckResourceAttrSet(ccsResName, "elasticsearch.0.topology.0.node_roles.#"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check that the correct node roles are set in this and all tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but node_roles are likely to evolve over time, if we're too strict in our assertions we'll end up having a lot of maintenance overhead, so I've opted to at least for now check this lightly rather than assert all the node roles for all topology elements.

@marclop
Copy link
Contributor Author

marclop commented Mar 10, 2021

@karencfv I'll add the datasource schema change as well.

Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

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

Thanks for adding this to the data source as well!

@marclop
Copy link
Contributor Author

marclop commented Mar 11, 2021

Tests actually passed, the post destroy tracking is what returned an error, so we're good to merge.

@marclop marclop merged commit af5e016 into elastic:master Mar 11, 2021
@marclop marclop deleted the f/add-data-tiers-support branch March 11, 2021 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ec_deployment: Support node_roles and data tiers
3 participants