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.

Sign up for GitHub

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

Jump to bottom

fix(oci): clean dead code #290

Merged
merged 2 commits into from
Sep 18, 2022
Merged

fix(oci): clean dead code #290

merged 2 commits into from
Sep 18, 2022

Conversation

jycamier
Copy link
Contributor

Referring to the issue #281

Helmfile supports the OCI format but executes the helm pull command twice, resulting in :

  • slow down the use of Helmfile when using multiple registries supporting OCI format
  • impact the possible rate limits of a registry

How

  • deletion of a "pull chanel" which never really used as the destination path isn't shared with the other commands (like the template command)
  • separation of concerns about chartPull and chartExport

@yxxhero
Copy link
Member

yxxhero commented Aug 15, 2022

@jycamier Please fix DCO issue.

@yxxhero yxxhero linked an issue Aug 15, 2022 that may be closed by this pull request
OCI charts are pull 2 times #281
Closed
tuananh
tuananh reviewed Aug 16, 2022
View reviewed changes
pkg/state/state.go
@@ -3339,22 +3328,3 @@ func (st *HelmState) getOCIChart(pullChan chan PullCommand, release *ReleaseSpec

return &chartPath, nil
}

// Pull charts one by one to prevent concurrent pull problems with Helm
Copy link

Choose a reason for hiding this comment

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

I'm just curious why this part is no longer needed? does helm fix it upstream?

Copy link
Contributor Author

@jycamier jycamier Aug 16, 2022 •
edited

Choose a reason for hiding this comment

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

In the worker which manipulate the pullChan channel is pulling charts only for the getOCIChart function and the charts pulled are never used : this worker runs for nothing.

I don't know why this code was write but today, if it works, I imagine the previous helm issue you speak about is fixed.

Copy link
Member

Choose a reason for hiding this comment

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

@jycamier Can we maintain the original functional logic, reform it, or give a better reason to clean it up?

Copy link
Contributor

@mumoshu mumoshu Sep 18, 2022 •
edited

Choose a reason for hiding this comment

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

For your reference, here's the original issue that reported the helm pull issue:
roboll/helmfile#1705

Copy link
Contributor

@mumoshu mumoshu Sep 18, 2022 •
edited

Choose a reason for hiding this comment

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

From my limited test result, I can say that this is no longer an issue since helm 3.8.

Previsouly helm chart pull pulled to the chart into a local registry cache maintained by helm and apparently it was subject to a race condition.

However since Helm 3.8, the command has been merged into helm fetch and renamed to helm pull, and the new implementation seems to pull into the local directory. Naturally, there will be no place for race conditions as long as you don't pull the same chart multiple times while reading the chart before all other chart pull operations complete. Helmfile does wait for all the chart pull operations to finish before doing subsequent operations like chartify, diff, template, and so on. Everything should work ok.

That said, I think it's fine to remove this legacy code in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

@mumoshu ok. got it. Thanks very much.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can say that this is no longer an issue since helm 3.8.

Let me correct this. I meant to say "helm 3.7"

@yxxhero yxxhero force-pushed the fix/oci-clean-dead-code branch 2 times, most recently from 98b5caa to 2a5ed80 Compare August 30, 2022 11:07
@jycamier @yxxhero
fix(oci): remove dead code
303a449 
Signed-off-by: Jean-Yves CAMIER <jycamier@gmail.com>
@yxxhero
Copy link
Member

yxxhero commented Sep 16, 2022

ping

mumoshu
mumoshu reviewed Sep 18, 2022
View reviewed changes
pkg/helmexec/exec.go
_ = os.RemoveAll(tempDir)
}()
helmArgs = []string{"fetch", ociChartURL, "--version", ociChartTag, "--destination", tempDir}
helmArgs = []string{"pull", ociChartURL, "--version", ociChartTag, "--destination", path}
} else {
helmArgs = []string{"chart", "pull", chart}
Copy link
Contributor

@mumoshu mumoshu Sep 18, 2022 •
edited

Choose a reason for hiding this comment

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

@yxxhero FYI, this is the legacy code path for helm 3.7.x or less. We no longer support 3.7.x since #286 and this should probably be safe enough to be removed in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

yes. you are right. @mumoshu

mumoshu
mumoshu reviewed Sep 18, 2022
View reviewed changes
pkg/helmexec/exec.go
helmArgs = []string{"chart", "export", chart}
// in the 3.7.0 version, the chart export has been removed
// https://github.com/helm/helm/releases/tag/v3.7.0
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we'd better panic here as we shouldn't reach this line in practice?

@mumoshu
Merge branch 'main' into fix/oci-clean-dead-code
fd786c0 
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@mumoshu
Copy link
Contributor

mumoshu commented Sep 18, 2022

@yxxhero Fixed the conflict in fd786c0. I believe this is good to go once the CI build passes.

@yxxhero
Copy link
Member

yxxhero commented Sep 18, 2022

@mumoshu yes. I think so.

mumoshu
mumoshu approved these changes Sep 18, 2022
View reviewed changes
Copy link
Contributor

@mumoshu mumoshu 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 a lot for investigating and fixing it @jycamier!

@mumoshu mumoshu merged commit b8cf0f1 into helmfile:main Sep 18, 2022
@toVersus toVersus mentioned this pull request Sep 18, 2022
Merged
@jycamier
Copy link
Contributor Author

Your welcome @mumoshu. This is my first golang contribution 🍼

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2022
@jycamier jycamier deleted the fix/oci-clean-dead-code branch September 23, 2022 16:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tuananh tuananh tuananh left review comments

@yxxhero yxxhero yxxhero left review comments

@mumoshu mumoshu mumoshu approved these changes

@itscaro itscaro Awaiting requested review from itscaro itscaro is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OCI charts are pull 2 times
4 participants
@jycamier @yxxhero @mumoshu @tuananh