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

Code Refactor to update the API's to opensearch.org (removing opster reference) #698

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

prudhvigodithi
Copy link
Member

@prudhvigodithi prudhvigodithi commented Jan 13, 2024

Description

Please note this PR is for refactoring the code to update the packages and remove Opster reference. Once this is merged I will raise a new PR to update the GitHub workflows and release process to do a minimal release.

On my local I have tested with make install ; make run and was able to deploy the operator.

Issues Resolved

Part of #674

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: Prudhvi Godithi <[email protected]>
@prudhvigodithi
Copy link
Member Author

Adding @swoehrl-mw @bbarani @peterzhuamazon @dbason @idanl21 @ido-opster @dblock to please take a look.

@prudhvigodithi prudhvigodithi changed the title Code Refactor Code Refactor to update the API's to opensearch.org (removing opster reference) Jan 16, 2024
@prudhvigodithi prudhvigodithi changed the title Code Refactor to update the API's to opensearch.org (removing opster reference) Code Refactor to update the API's to opensearch.org (removing opster reference) Jan 16, 2024
@prudhvigodithi prudhvigodithi merged commit 1c72422 into opensearch-project:main Jan 17, 2024
6 checks passed
@swoehrl-mw
Copy link
Collaborator

@prudhvigodithi These changes completely break the operator. By renaming the apigroup of all the CRDs they will be incompatible and users will not be able to update. They would have to basically destroy their opensearch clusters by removing all CRs, update the operator, change their yamls to use the new apigroup, then reinstall everything.
This change should not be done, or at least not without due discussion and consideration.

@prudhvigodithi
Copy link
Member Author

Hey @swoehrl-mw it wont break, the user has to re-install the operator that will have the new API groups, from there one can control the cluster settings with the new API group.

@swoehrl-mw
Copy link
Collaborator

@prudhvigodithi I disagree.
In the best case all users have to migrate their cluster yamls which relies on the users doing extra manual and coordinated tasks (if they even read about having to do this).
In the wort case reinstalling the operator will delete the old CRDs (which happens because they are part of the helm chart so will be deleted if they are not part of the new version), meaning the deletion of all clusters (deleting a CRD will delete all resources of that type). It can be avoided, but it hinges on manual and specific action by the users, which is very risky.

@prudhvigodithi
Copy link
Member Author

@swoehrl-mw yes as I mentioned it can be avoided and the cluster need not be destroyed, but do you suggest to support both the opensearch.opster.io/v1 and opensearch.org/v1?

@swoehrl-mw
Copy link
Collaborator

@prudhvigodithi I suggest to not rename the CRDs at all and just keep the old names, due to the risks and incompatibility. It wouldn't be the first operator to have an apigroup no longer matching the name or org.

@prudhvigodithi
Copy link
Member Author

prudhvigodithi commented Jan 17, 2024

So you suggest continue with opensearch.opster.io/v1 for all upcoming future releases?
@bbarani @peterzhuamazon @dtaivpp can you add your thoughts here based on @swoehrl-mw suggestion?
or we can rename and have clear steps with the user migration guide to opensearch.org/v1?

@bbarani
Copy link
Member

bbarani commented Jan 17, 2024

@prudhvigodithi @swoehrl-mw Can we incorporate this change and release a new major version to support this breaking change? We might not be able to continue using the name Opster in opensearch.opster.io/v1 for all upcoming future releases. Is there any other alternatives?

@prudhvigodithi
Copy link
Member Author

prudhvigodithi commented Jan 17, 2024

I feel we should not stop releasing the operator with new API opensearch.org/v1. As without this, will stop new users from onboarding to OpenSearch via the operator with opensearch.org/v1. We should also come back and provide some steps on how to migrate to the new API for the existing users.

@sergeiwaigant
Copy link

Could you plan to compile and release the operator along with the helm chart?
I need to setup a test cluster for a customer and would be able to perform some tests and write a migration document.

@prudhvigodithi
Copy link
Member Author

Hey @bbarani @swoehrl-mw,

An alternate way is to use the conversion web-hook with hub and spoke model in the operator to manage both the API's opensearch.org/v1, opensearch.opster.io/v1. What will happen with this is even though we use the opensearch.opster.io/v1, the backend will be converted to opensearch.org/v1. But long term still we need to stop the API opensearch.opster.io/v1 and allow the users to migrate opensearch.org/v1.

I have created 2.x branch that can continue to support (the patches) with API opensearch.opster.io/v1. From main branch we can do a 3.x release that only supports API opensearch.org/v1.

For existing users, the migration process from operator 2.x (with API's opensearch.opster.io/v1) to 3.x (opensearch.org/v1) should be written clearly. At high level following are steps to migrate to opensearch.org/v1 (tested on my local and works).

  • Backup the cluster and persistent volumes (a safety measure to limit the data lose).

  • Delete the opensearch.yaml custom resource with apiVersion: opensearch.opster.io/v1 by running kubectl delete -f opensearch.yaml.

Note: This wont delete the PV and PVC's created where the actual cluster data is stored. It will just delete the other resources bought up by the CRD OpenSearchCluster.

  • Now delete the old operator and install the latest 3.x version using helm chart.

  • Update the all existing PVC labels to

  labels:
    opensearch.org/opensearch-cluster: my-first-cluster
    opensearch.org/opensearch-nodepool: masters

This is to ensure the new cluster stateful set pods use the existing claims where the actual data resides.

  • Now Once the operator pod is up and running, update the opensearch.yaml to apiVersion: opensearch.org/v1 and run kubectl apply -f opensearch.yaml.

This should bring back the cluster mounting to the same persistent volumes and now the using the new API's opensearch.org/v1.

@swoehrl-mw
Copy link
Collaborator

@prudhvigodithi Your steps require users to completely shutdown their opensearch clusters during the upgrade (as all pods will be deleted). Not good for anyone running the operator in production, especially as the operator normally does everything in a rolling fashion.

Why not stay with the old apigroup name until there is a pressing (legal) need to change it or a big redesign of the CRDs happens?

Changing the name just to get rid of all opster references and because maybe potentially Elastic might not like the operator continuing to use the opster name in technical naming is IMO too big a disruption for little advantage.

@prudhvigodithi
Copy link
Member Author

Hey Everyone, at this point working with legal team to get the confirmation on using the existing Opster API's for the operator, myself or @bbarani will post details as soon as we have some updates.
Thank you

prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 26, 2024
@bbarani
Copy link
Member

bbarani commented Jan 26, 2024

We will revert this PR for now and work with the community to come up with a plan along with a roadmap. CC: @swoehrl-mw @prudhvigodithi

prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 26, 2024
prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 26, 2024
prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 26, 2024
prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 26, 2024
prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 26, 2024
prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 26, 2024
prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 26, 2024
prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 26, 2024
prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 26, 2024
prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 26, 2024
prudhvigodithi added a commit to prudhvigodithi/opensearch-k8s-operator that referenced this pull request Jan 27, 2024
…ng `opster` reference) (opensearch-project#698)"

This reverts commit 1c72422.

Signed-off-by: Prudhvi Godithi <[email protected]>
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.

5 participants