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

Fix resource update reconciling, increase log level for users reconciler #727

Merged
merged 7 commits into from
Feb 26, 2024

Conversation

evheniyt
Copy link
Contributor

@evheniyt evheniyt commented Feb 13, 2024

Description

Fixes #633
Fixes #731

Based on investigations here and here it was found that a discrepancy between <nil slice> and <[] slice> is causing the operator to constantly update User, Role, IndexTemplate, etc.

Also, the current log level for User reconciles produces too many logs, so increasing its level may be a good idea here.

Because of the issue with comparing nil and empty slices, I have replaced reflect.DeepEqual with comp.Equal. Also, added a helper function that will sort nested json keys in cases when API returns unsorted keys which is casing comparing to return diff.

Issues Resolved

Closes #633
Closes #731

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Yevhenii Tiutiunnyk <evheniytyutyunnik@gmail.com>
@evheniyt
Copy link
Contributor Author

The reason why tests are broken (and why they were working before) is that in tests userResponse OpendistroSecurityRoles and BackendRoles are nil, but in reality, they are an empty slice
Appreciate it if someone could help with this

@prudhvigodithi
Copy link
Collaborator

Hey @evheniyt can you please pull from upstream to fix the failing tests ? Here is the recent PR that can fix the tests #725.
Thanks

@evheniyt
Copy link
Contributor Author

Hi @prudhvigodithi, thank you for your reply. I did a sync with upstream, but I guess in my case the issue is different, please see my comment above that describes why tests are failing for this PR.

Signed-off-by: Yevhenii Tiutiunnyk <evheniytyutyunnik@gmail.com>
Signed-off-by: Yevhenii Tiutiunnyk <evheniytyutyunnik@gmail.com>
Signed-off-by: Yevhenii Tiutiunnyk <evheniytyutyunnik@gmail.com>
Signed-off-by: Yevhenii Tiutiunnyk <evheniytyutyunnik@gmail.com>
@evheniyt evheniyt changed the title Fix users and role reconciling, increase log level for users reconciler Fix resource update reconciling, increase log level for users reconciler Feb 26, 2024
@evheniyt
Copy link
Contributor Author

@prudhvigodithi could you please review or add someone from active reviewers?

@prudhvigodithi
Copy link
Collaborator

Thanks @evheniyt for your contribution, the PR LGTM!.
Adding @salyh @pchmielnik @swoehrl-mw @jochenkressin
@bbarani

@prudhvigodithi prudhvigodithi merged commit 913bc04 into opensearch-project:main Feb 26, 2024
6 checks passed
swoehrl-mw pushed a commit to MaibornWolff/opensearch-operator that referenced this pull request Apr 25, 2024
…ler (opensearch-project#727)

### Description
Fixes opensearch-project#633
Fixes opensearch-project#731

Based on investigations
[here](opensearch-project#633 (comment))
and
[here](opensearch-project#731 (comment))
it was found that a discrepancy between `<nil slice>` and `<[] slice>`
is causing the operator to constantly update User, Role, IndexTemplate,
etc.

Also, the current log level for User reconciles produces too many logs,
so increasing its level may be a good idea here.

Because of the issue with comparing `nil` and empty slices, I have
replaced `reflect.DeepEqual` with `comp.Equal`. Also, added a helper
function that will sort nested json keys in cases when API returns
unsorted keys which is casing comparing to return diff.
### Issues Resolved
Closes opensearch-project#633
Closes opensearch-project#731

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Yevhenii Tiutiunnyk <evheniytyutyunnik@gmail.com>
Signed-off-by: Sebastian Woehrl <sebastian.woehrl@maibornwolff.de>
swoehrl-mw pushed a commit that referenced this pull request May 13, 2024
#809)

### Description
This fixes the same issue that previously existed with
`OpenSearchIndexTemplate` resources (#731), which was fixed in #727 but
did not apply the fix to `OpenSearchComponentTemplate` resources.

### Issues Resolved
Fixes #799 

### Check List
- [x] Commits are signed per the DCO using --signoff 
- [ ] Unittest added for the new/changed functionality and all unit
tests are successful
- [ ] Customer-visible features documented
- [x] No linter warnings (`make lint`)

If CRDs are changed:
- [ ] CRD YAMLs updated (`make manifests`) and also copied into the helm
chart
- [ ] Changes to CRDs documented

Please refer to the [PR
guidelines](https://github.com/opensearch-project/opensearch-k8s-operator/blob/main/docs/developing.md#submitting-a-pr)
before submitting this pull request.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

Signed-off-by: Zack Brenton <zack@imposium.com>
swoehrl-mw pushed a commit to swoehrl-mw/opensearch-k8s-operator that referenced this pull request May 16, 2024
opensearch-project#809)

### Description
This fixes the same issue that previously existed with
`OpenSearchIndexTemplate` resources (opensearch-project#731), which was fixed in opensearch-project#727 but
did not apply the fix to `OpenSearchComponentTemplate` resources.

### Issues Resolved
Fixes opensearch-project#799

### Check List
- [x] Commits are signed per the DCO using --signoff
- [ ] Unittest added for the new/changed functionality and all unit
tests are successful
- [ ] Customer-visible features documented
- [x] No linter warnings (`make lint`)

If CRDs are changed:
- [ ] CRD YAMLs updated (`make manifests`) and also copied into the helm
chart
- [ ] Changes to CRDs documented

Please refer to the [PR
guidelines](https://github.com/opensearch-project/opensearch-k8s-operator/blob/main/docs/developing.md#submitting-a-pr)
before submitting this pull request.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

Signed-off-by: Zack Brenton <zack@imposium.com>
(cherry picked from commit f38eb26)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] OpensearchIndexTemplate always requires update Operator is constantly trying to update OpensearchUser
2 participants