#Opensearch: missing fields during update cluster configuration

[acharviakou]

Issue #2219

Description of changes:
Set missing fields during domain update operation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[ack-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: acharviakou
Once this PR has been reviewed and has the lgtm label, please assign a-hilaly for approval by writing /assign @a-hilaly in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ack-prow]

Hi @acharviakou. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[michaelhtm]

Thank you for the changes @acharviakou
/ok-to-test

[acharviakou]

As I can see one test failed
According to job history https://prow.ack.aws.dev/job-history/s3/ack-prow-logs/pr-logs/directory/opensearchservice-verify-attribution
Previous build failed because of the same error
Job link: https://prow.ack.aws.dev/view/s3/ack-prow-logs/pr-logs/pull/aws-controllers-k8s_opensearchservice-controller/77/opensearchservice-verify-attribution/1844225177622155264
Changes: #77

Should I change something?

[michaelhtm]

This is great @acharviakou, thank you for contributing!
Left two inline comment.
Can you also make the PR title the same as the issue title? and move the issue number to the PR description, thanks!

[michaelhtm]

Can we have a simple update test checking these updates work? Thanks!

[acharviakou]

Thanks for comment
Should I add e2e tests like here in PR? https://github.com/aws-controllers-k8s/opensearchservice-controller/pull/67/files#diff-aee6724aa306271cdd04ef6ed757ce3f297dd5b23d0b198d5012299779261960
Should I add patch commands in the same method or create separate one?

[acharviakou]

/retest

[ack-prow]

@acharviakou: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
opensearchservice-verify-attribution	c6bff12	link	false	/test opensearchservice-verify-attribution
opensearchservice-kind-e2e	c6bff12	link	true	/test opensearchservice-kind-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[acharviakou]

Hi @michaelhtm
I have issues with using default method k8s.wait_on_condition(https://github.com/aws-controllers-k8s/test-infra/blob/main/src/acktest/k8s/resource.py#L337)
Controller logs from test

025-01-17T06:33:22.520Z	INFO	ackrt	desired resource state has changed	{"kind": "Domain", "namespace": "default", "name": "my-os-domain2-xocsac", "account": "095708837592", "role": "", "region": "us-west-2", "is_adopted": false, "generation": 4, "diff": [{"Path":{"Parts":["Spec","AutoTuneOptions","DesiredState"]},"A":"ENABLED","B":null}]}

Controller is always trying to update OpenSearch cluster because of diff in state. It means 'ACK.ResourceSynced' condition is always 'False'
Checking code deeper
https://github.com/aws-controllers-k8s/opensearchservice-controller/blob/v1.0.1/pkg/resource/domain/sdk.go#L191
https://docs.aws.amazon.com/opensearch-service/latest/APIReference/API_AutoTuneOptionsOutput.html
This means controller have missing AutoTune DesiredState from AWS response

Behaviour a bit explained in issue description aws-controllers-k8s/community#2219
Just to sum up:
When I am creating OpenSearch domain first time using

autoTuneOptions:
    desiredState: DISABLED
    useOffPeakWindow: false

Controller create domain in AWS, but removing autoTuneOptions.desiredState from k8s manifest. This is the logic of method to create cluster.
If I am going to update cluster parameters controller will perform changes, but autoTuneOptions.desiredState will not be removed from manifest
https://github.com/aws-controllers-k8s/opensearchservice-controller/blob/v1.0.1/pkg/resource/domain/hooks.go#L181

I would suggest provide nil in DesiredState update method. In such case changes will be applied to AWS resource, but field will be removed from k8s manifest to eliminate diff.

ko.Spec.AutoTuneOptions = &v1alpha1.AutoTuneOptionsInput{
			DesiredState:         nil,
			MaintenanceSchedules: maintSchedules,
		}

This option can help in one more case. Some instances t2,t3 does not support AutoTune in general.
It means I can not set "autoTuneOptions.desiredState: DISABLED"
https://docs.aws.amazon.com/opensearch-service/latest/developerguide/supported-instance-types.html

Reply from AWS Support

This cluster is configured with a t3.small instance type, which does not support Autotune. [1]
When you submitted a request to disable Autotune, the operation failed and resulted in an ERROR state. [2] 
This behaviour is not an issue but an expected behaviour due to the instance type limitation. 

Few more info. To create OpenSearch cluster using t3 instance I have to specify "autoTuneOptions.desiredState: DISABLED" to avoid validation error. But if I am applying the same manifest one more time this field is not required

Please let me know if you have another suggestion.
Thanks in advance.

[michaelhtm]

Hey @acharviakou it seems like AutoTuneOptionsInput field is called DesiredState, and in output they call it state. This confused the generator, which is why it does not set it during the code-generation. Here's how you can do that:

https://aws-controllers-k8s.github.io/community/docs/contributor-docs/code-generator-config/#renames-renaming-things
https://aws-controllers-k8s.github.io/community/docs/contributor-docs/code-generator-config/#renaming-things-with-different-names-in-input-and-output-shapes

The example is not there right now, but we would appreciate if you can send a PR with the example from this PR you made 🙂.