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

Add a new cli option --ignore-already-migrated to convert command #182

Merged
merged 2 commits into from Jun 8, 2021

Conversation

sergenyalcin
Copy link
Contributor

@sergenyalcin sergenyalcin commented Feb 27, 2021

Idempotencey is a significant point for the Kubernetes environment. It is very important to provide rerunable and idempotent processes in order to manage Kubernetes' batch operations (eg. Kubernetes Jobs) more properly and consistently. In this context, it becomes critical for the client-tools to provide idempotency. Because managing rerunability and idempotency can be very complicated in Jobs environment.

In the context of the general framework I tried to summarize above, I will give a few examples about the idempotency of the 2to3 tool. Because of the removal of Helm v2 support, the migration to Helm v3 has become mandatory. For this reason, 2to3 tool has features that make the migration very easy. However, as far as I have observed, the general working principle of the tool is not idempotent. In order to explain this issue better, I would like to give two examples:

1. Migrating all versions of a single release by directly running the 2to3 tool

As we all know, a helm release can have more than one revision. In this context, all of these release versions can be migrated with the 2to3 tool (default to the last 10 versions). All versions of the Release are migrated respectively. However, in a scenario like the following, the release's migration process may encounter a problem.

Scenario: For example, we have a Helm2 release called test-scenario-1. And there are 5 versions of this release: v1, v2, v3, v4 and v5. The migration of these release is started by running the following command:

$ helm 2to3 convert test-scenario-1

Let's imagine that there is a problem in our local internet connection after the test-scenario-1.v3 version is migrated. Or another problem may have been encountered at this time. For example, there may be a connection problem with the K8s API, network problems that the cluster itself may experience instantly, a problem with the Kubernetes client while creating the helmv3 secret of test-scenario-1.v4.

As a result, the point I want to come to is that the migration process was interrupted. In other words, the v1, v2 and v3 versions of the release were successfully migrated, but before the v4 and v5 versions were migrated, the 2to3 tool exited with an error due to the problems experienced.

In this case, when the 2to3 convert command is run again for the same release, while the v1 version is migrating, 2to3 tells us that this is already a migrated release and exits with 1 status code.

Although the release has not been fully migrated yet, the tool does not continue the migration in the next run. In this case, it is necessary to intervene manually to delete the migrated release versions (v1, v2 and v3).

In summary for Scenario-1, the fact that the 2to3 convert command is not idempotent.

2. Migrating all releases by using a Kubernetes Job that runs the 2to3 tool

Scenario: Suppose we use a batch operation (Kubernetes Job) to manage migration process centrally in a Kubernetes environment. The Kubernetes Job runs a command/script as described in the 2to3 document:

$ kubectl get [configmap | secret] -n <tiller_namespace> \
 -l "OWNER = TILLER" | awk '{print $ 1}' | grep -v NAME | cut -d '.' -f1 | uniq | xargs -n1 helm 2to3 convert

For example, in this scenario, there is only a version of all the releases, but we have 10 releases. And again, for any reason as above, after the migration of the 6th release, the 2to3 tool will exit with an error. When this job runs repeatedly until it reaches the retry count, it will exit 1 with the already exists error when trying to migrate the 1st release each time. This will cause Job to fail. In this case, it will be necessary to delete the migrated releases mentioned in Scenario 1 by manual intervention. Or you will need a complex process such as handling the situation of the cluster in the script that is inside Kubernetes Job.

At the end of this scenario, I come to the same point, the convert command is not idempotent and exits with 1 status code for the migrated release or release versions. This may reduce our power in the Kubernetes environment.


After these two scenarios, I hope to be able to explain my concern. Of course, by default, it may be a preference that the convert command is not idempotent. However, this idempotency can be achieved with a command line parameter. With this change, the --ignore-already-migrated cli option will be added to the convert command and it will provide this idempotency. In order for the change to be backward compatible and not affecting the current functioning of the tool, the default value of the new cli option is false.

If you review this change, please continue to discuss. Your opinions are very important to me. If you are interested in this change and include it in the tool, I hope it will be useful to me and also to the people doing jobs in this environment.

Thanks

@hickeyma
Copy link
Collaborator

@sergenyalcin Do you mind signing the DCO. Refer to https://helm.sh/blog/helm-dco/ for more details.

… for convert command

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
@sergenyalcin sergenyalcin force-pushed the ignore-already-migrated-option branch from 8e51a72 to 0e9edec Compare May 14, 2021 15:52
@sergenyalcin
Copy link
Contributor Author

@sergenyalcin Do you mind signing the DCO. Refer to https://helm.sh/blog/helm-dco/ for more details.

Signed, thank you.

Copy link
Collaborator

@hickeyma hickeyma 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 pushing PR @sergenyalcin. Some comments inline to look at.

cmd/convert.go Outdated Show resolved Hide resolved
cmd/convert.go Outdated Show resolved Hide resolved
cmd/convert.go Outdated Show resolved Hide resolved
cmd/convert.go Outdated Show resolved Hide resolved
cmd/convert.go Outdated Show resolved Hide resolved
@marckhouzam
Copy link
Member

@sergenyalcin could you update the completion.yaml file to include this new flag please?

@helm-bot helm-bot added size/M and removed size/S labels Jun 3, 2021
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
@sergenyalcin sergenyalcin force-pushed the ignore-already-migrated-option branch from 23cb910 to 24b1bc9 Compare June 3, 2021 20:42
@sergenyalcin
Copy link
Contributor Author

sergenyalcin commented Jun 3, 2021

@sergenyalcin could you update the completion.yaml file to include this new flag please?

@marckhouzam Updated, thanks!

@sergenyalcin
Copy link
Contributor Author

sergenyalcin commented Jun 3, 2021

Thanks for pushing PR @sergenyalcin. Some comments inline to look at.

@hickeyma Thank you very much for your comments. I have addressed them.

Copy link
Collaborator

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding the feature @sergenyalcin

@hickeyma hickeyma added the enhancement New feature or request label Jun 8, 2021
@hickeyma hickeyma merged commit 62fcef2 into helm:main Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants